Bug 102560

Created attachment 178577 [details] Patch

Created attachment 178578 [details] Patch

Comment on attachment 178578 [details] Patch please have a look at this CL

Created attachment 178787 [details] illustration of the impact on the form controls elements

Comment on attachment 178578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178578&action=review First pass review comments. > Source/WebCore/rendering/TextAutosizer.cpp:126 > +bool TextAutosizer::contentHeightIsConstrained(const RenderBlock* container) Can you rename this method, now that it's not just dealing with constrained height, e.g. to containerShouldBeAutosized? Also it was at jchaffraix's suggestion that this function was given static linkage in TextAutosizer.cpp and not exposed in the header. Though it's probably not worth fretting about this right now, as I think avayvod has a patch in the works to give a whole bunch of these functions static linkage and move them out of the header, so he can keep this consistent when he does that. > Source/WebCore/rendering/TextAutosizer.cpp:131 > + QualifiedName tags[4] = {inputTag, textareaTag, buttonTag, formTag}; What about selectTag? And formTag is a tricky one; it probably does make sense to exclude it; but I suspect some sites will misuse this and wrap their entire page in a form element... > Source/WebCore/rendering/TextAutosizer.cpp:132 > + if (containerContainsTags(container, tags, 4)) It's not clear that the 4 depends on the length of the list of tags. I'd suggest at least using sizeof to avoid hardcoding this. Kenneth/Julien can probably best advise whether it would be better to use a Vector or whether this is fine.

Comment on attachment 178578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178578&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:126 >> +bool TextAutosizer::contentHeightIsConstrained(const RenderBlock* container) > > Can you rename this method, now that it's not just dealing with constrained height, e.g. to containerShouldBeAutosized? > > Also it was at jchaffraix's suggestion that this function was given static linkage in TextAutosizer.cpp and not exposed in the header. Though it's probably not worth fretting about this right now, as I think avayvod has a patch in the works to give a whole bunch of these functions static linkage and move them out of the header, so he can keep this consistent when he does that. done >> Source/WebCore/rendering/TextAutosizer.cpp:131 >> + QualifiedName tags[4] = {inputTag, textareaTag, buttonTag, formTag}; > > What about selectTag? > And formTag is a tricky one; it probably does make sense to exclude it; but I suspect some sites will misuse this and wrap their entire page in a form element... done, also removed form tag >> Source/WebCore/rendering/TextAutosizer.cpp:132 >> + if (containerContainsTags(container, tags, 4)) > > It's not clear that the 4 depends on the length of the list of tags. I'd suggest at least using sizeof to avoid hardcoding this. Kenneth/Julien can probably best advise whether it would be better to use a Vector or whether this is fine. done, using Vector now

Created attachment 179060 [details] Patch

Comment on attachment 179060 [details] Patch latest patch with the fixed comments from JohnM

Comment on attachment 179060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179060&action=review A couple more comments (some of which I intended to submit earlier, but somehow got missed out). > Source/WebCore/rendering/TextAutosizer.cpp:233 > + if (parent->isRenderButton() || parent->isTextControl() || parent->isMenuList()) Looking at HTMLSelectElement::createRenderer, <select> elements allowing multiple selection can also be rendered as a RenderListBox (instead of a RenderMenuList). And this still doesn't seem exhaustive. For example <input type="file"> creates a RenderFileUploadControl which contains text (the filename) alongside a button (see FileInputType::createRenderer). Ultimately listing all the render classes corresponding to the different input types doesn't seem sustainable. Can you instead do a tagname-based check? e.g.: Node* parentNode = renderer->parent() ? renderer->parent()->generatingNode() : 0; if (parentNode && parentNode->isElement() && s_inputTagNames.contains(toElement(parentNode)->tagQName())) return false; > Source/WebCore/rendering/TextAutosizer.cpp:297 > +bool TextAutosizer::containerContainsTags(const RenderObject* container, Vector<QualifiedName>& tags) Nit: If it's a container, it's guaranteed to be a RenderBlock, so please pass it in as such to make things clearer. > Source/WebCore/rendering/TextAutosizer.cpp:299 > + const Node* containerNode = container->node(); I don't think you need to special-case the container. Just do: const RenderObject* object = container; while (object) { ... object = nextInPreOrderSkippingDescendantsOfContainers(descendant, container); } > Source/WebCore/rendering/TextAutosizer.cpp:311 > + bool validCandidate = descendantNode && descendantNode->isHTMLElement() && !descendantNode->isShadowRoot(); Can you add a comment explaining why you need to check isShadowRoot? > LayoutTests/fast/text-autosizing/form-controls-boosting-button-textarea-input-elements.html:84 > + <th align="right"><label for="Bugzilla_login">Login:</label></th> This test is much too complicated. For example it's clearly irrelevant whether this label refers to "Bugilla_login" or some other field. Please reduce it to a minimal test-case[1]. [1]: https://wiki.mozilla.org/QA/Minimal_Test_Cases > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element-expected.html:15 > + #boosted { I prefer to leave the relevant* styles inline in Text Autosizing layout tests. While it's a good idea to separate presentation (CSS) from content (HTML) when building a website, for layout tests since they're supposed to be very simple, it's usually easier to have the styles inline so you can see immediately how everything behaves. *: (i.e. not generic things applied to the body, but the properties whose interactions are being tested - in this case just the boosted font size) > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element-expected.html:16 > + font-size: 2.05rem; Huh - 2.05x is a strange multiplier. Normally in these tests the multiplier is 800/320 = 2.5x. What's happening? > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:11 > + font-family: Verdana, sans-serif; The font-family shouldn't make a difference, so can be removed. > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:12 > + font-size: small; Please specify a pixel font size (e.g. 16px), then in the text below you can make a statement, like "This text should be autosized to 40px computed font-size (16 * 800/320)." > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:30 > +This tests the boosting of check boxes. This paragraph should be boosted, while Please don't talk about boosting. The WebKit term for this is Text Autosizing, so for example instead of "paragraph foo has been boosted" write "paragraph foo has been autosized", and instead of "paragraph foo has been boosted more than paragraph bar" write "paragraph foo has been autosized with a larger multiplier than paragraph bar". Ditto elsewhere :) > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:32 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ It's best to use Lorem ipsum text like the other tests, as a long string of +'s like this could behave stranglely since it can't be wrapped. > LayoutTests/fast/text-autosizing/form-controls-boosting-select-element.html:39 > + <option value=option1>Option 1</option> Please always use double quotes around attributes, i.e. value="default". In fact, you shouldn't even need to specify the value here (see minimal test cases earlier).

Comment on attachment 179060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179060&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:233 >> + if (parent->isRenderButton() || parent->isTextControl() || parent->isMenuList()) > > Looking at HTMLSelectElement::createRenderer, <select> elements allowing multiple selection can also be rendered as a RenderListBox (instead of a RenderMenuList). > > And this still doesn't seem exhaustive. For example <input type="file"> creates a RenderFileUploadControl which contains text (the filename) alongside a button (see FileInputType::createRenderer). > > Ultimately listing all the render classes corresponding to the different input types doesn't seem sustainable. Can you instead do a tagname-based check? e.g.: > Node* parentNode = renderer->parent() ? renderer->parent()->generatingNode() : 0; > if (parentNode && parentNode->isElement() && s_inputTagNames.contains(toElement(parentNode)->tagQName())) > return false; right, testing for tags is probably better and more consistent done >> Source/WebCore/rendering/TextAutosizer.cpp:297 >> +bool TextAutosizer::containerContainsTags(const RenderObject* container, Vector<QualifiedName>& tags) > > Nit: If it's a container, it's guaranteed to be a RenderBlock, so please pass it in as such to make things clearer. done >> Source/WebCore/rendering/TextAutosizer.cpp:299 >> + const Node* containerNode = container->node(); > > I don't think you need to special-case the container. Just do: > > const RenderObject* object = container; > while (object) { > ... > object = nextInPreOrderSkippingDescendantsOfContainers(descendant, container); > } done >> Source/WebCore/rendering/TextAutosizer.cpp:311 >> + bool validCandidate = descendantNode && descendantNode->isHTMLElement() && !descendantNode->isShadowRoot(); > > Can you add a comment explaining why you need to check isShadowRoot? actually removed testing for isShadowRoot at this point probably not necessary >> LayoutTests/fast/text-autosizing/form-controls-boosting-button-textarea-input-elements.html:84 >> + <th align="right"><label for="Bugzilla_login">Login:</label></th> > > This test is much too complicated. For example it's clearly irrelevant whether this label refers to "Bugilla_login" or some other field. Please reduce it to a minimal test-case[1]. > > [1]: https://wiki.mozilla.org/QA/Minimal_Test_Cases done >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element-expected.html:15 >> + #boosted { > > I prefer to leave the relevant* styles inline in Text Autosizing layout tests. While it's a good idea to separate presentation (CSS) from content (HTML) when building a website, for layout tests since they're supposed to be very simple, it's usually easier to have the styles inline so you can see immediately how everything behaves. > > *: (i.e. not generic things applied to the body, but the properties whose interactions are being tested - in this case just the boosted font size) done >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element-expected.html:16 >> + font-size: 2.05rem; > > Huh - 2.05x is a strange multiplier. Normally in these tests the multiplier is 800/320 = 2.5x. What's happening? this is because I used {font-size: small} for the body, so after removing this the factor is 2.5 >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:11 >> + font-family: Verdana, sans-serif; > > The font-family shouldn't make a difference, so can be removed. done >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:12 >> + font-size: small; > > Please specify a pixel font size (e.g. 16px), then in the text below you can make a statement, like "This text should be autosized to 40px computed font-size (16 * 800/320)." removed. done. >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:30 >> +This tests the boosting of check boxes. This paragraph should be boosted, while > > Please don't talk about boosting. The WebKit term for this is Text Autosizing, so for example instead of "paragraph foo has been boosted" write "paragraph foo has been autosized", and instead of "paragraph foo has been boosted more than paragraph bar" write "paragraph foo has been autosized with a larger multiplier than paragraph bar". Ditto elsewhere :) ok. done. >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:32 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > It's best to use Lorem ipsum text like the other tests, as a long string of +'s like this could behave stranglely since it can't be wrapped. ok. done. >> LayoutTests/fast/text-autosizing/form-controls-boosting-select-element.html:39 >> + <option value=option1>Option 1</option> > > Please always use double quotes around attributes, i.e. value="default". In fact, you shouldn't even need to specify the value here (see minimal test cases earlier). removed. done.

Created attachment 179285 [details] Patch

Comment on attachment 179285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179285&action=review I've reviewed the code - just some minor nits. I'll look at the tests next. > Source/WebCore/rendering/TextAutosizer.cpp:54 > + s_inputTags.append(inputTag); You shouldn't initialize a static member from a constructor - if multiple TextAutosizer objects are created (which isn't unlikely, since there's one per document) you will end up with multiple copies of each tag in s_inputTags! In theory you'd use a static initializer for this; though I think they're frowned upon in WebKit (and Chromium). So instead perhaps try the following pattern, which I've seen a few times: #include <wtf/StdLibExtras.h> static const Vector<QualifiedName>& formInputTags() { DEFINE_STATIC_LOCAL(Vector<QualifiedName>, formInputTags, ()); if (formInputTags.isEmpty()) { formInputTags.append(inputTag); formInputTags.append(textareaTag); formInputTags.append(buttonTag); formInputTags.append(selectTag); } return formInputTags; } No guarantees that Julien/Kenneth will like that though :) > Source/WebCore/rendering/TextAutosizer.cpp:132 > +bool TextAutosizer::containerShouldBeAutosized(const RenderBlock* container) You'll need to rebase this now http://wkbug.com/104925 has landed (sorry!). > Source/WebCore/rendering/TextAutosizer.cpp:232 > + // avoid creating containers for text within text controls, buttons, or selection menus. Nits: s/avoid/Avoid/ and s/selection menus/<select> dropdowns/ > Source/WebCore/rendering/TextAutosizer.cpp:234 > + if (parentNode && parentNode->isHTMLElement() && s_inputTags.contains(toElement(parentNode)->tagQName())) Nit: I think parentNode->isElementNode() should be sufficient, instead of isElementNode->isHTMLElement(); ditto below in containerContainsTags. > Source/WebCore/rendering/TextAutosizer.cpp:299 > + const RenderObject* runningObject = container; The term "running" is a little confusing here. It's probably fine to call it either "renderer" or "object". > Source/WebCore/rendering/TextAutosizer.cpp:360 > + // Equalize the depths if containerShouldBeAutosized necessary. Only one of the while loops below will get executed. s/if containerShouldBeAutosized necessary/if necessary/ > LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:29 > +the check boxes below should not be autosized. Lorem ipsum dolor sit amet, consectetur Nit: s/check boxes below/check boxes below and the surrounding text/ > LayoutTests/fast/text-autosizing/form-controls-boosting-radio-input-element.html:29 > +the radio buttons below should not be autosized. Lorem ipsum dolor sit amet, consectetur Nit: s/radio buttons below/radio buttons below and the surrounding text/

Comment on attachment 179285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179285&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:54 >> + s_inputTags.append(inputTag); > > You shouldn't initialize a static member from a constructor - if multiple TextAutosizer objects are created (which isn't unlikely, since there's one per document) you will end up with multiple copies of each tag in s_inputTags! > > In theory you'd use a static initializer for this; though I think they're frowned upon in WebKit (and Chromium). So instead perhaps try the following pattern, which I've seen a few times: > > #include <wtf/StdLibExtras.h> > > static const Vector<QualifiedName>& formInputTags() > { > DEFINE_STATIC_LOCAL(Vector<QualifiedName>, formInputTags, ()); > if (formInputTags.isEmpty()) { > formInputTags.append(inputTag); > formInputTags.append(textareaTag); > formInputTags.append(buttonTag); > formInputTags.append(selectTag); > } > return formInputTags; > } > > No guarantees that Julien/Kenneth will like that though :) oops, done. >> Source/WebCore/rendering/TextAutosizer.cpp:132 >> +bool TextAutosizer::containerShouldBeAutosized(const RenderBlock* container) > > You'll need to rebase this now http://wkbug.com/104925 has landed (sorry!). ack >> Source/WebCore/rendering/TextAutosizer.cpp:232 >> + // avoid creating containers for text within text controls, buttons, or selection menus. > > Nits: s/avoid/Avoid/ and s/selection menus/<select> dropdowns/ done >> Source/WebCore/rendering/TextAutosizer.cpp:234 >> + if (parentNode && parentNode->isHTMLElement() && s_inputTags.contains(toElement(parentNode)->tagQName())) > > Nit: I think parentNode->isElementNode() should be sufficient, instead of isElementNode->isHTMLElement(); ditto below in containerContainsTags. done >> Source/WebCore/rendering/TextAutosizer.cpp:299 >> + const RenderObject* runningObject = container; > > The term "running" is a little confusing here. It's probably fine to call it either "renderer" or "object". done >> Source/WebCore/rendering/TextAutosizer.cpp:360 >> + // Equalize the depths if containerShouldBeAutosized necessary. Only one of the while loops below will get executed. > > s/if containerShouldBeAutosized necessary/if necessary/ done >> LayoutTests/fast/text-autosizing/form-controls-boosting-checkbox-input-element.html:29 >> +the check boxes below should not be autosized. Lorem ipsum dolor sit amet, consectetur > > Nit: s/check boxes below/check boxes below and the surrounding text/ done >> LayoutTests/fast/text-autosizing/form-controls-boosting-radio-input-element.html:29 >> +the radio buttons below should not be autosized. Lorem ipsum dolor sit amet, consectetur > > Nit: s/radio buttons below/radio buttons below and the surrounding text/ done

Created attachment 179476 [details] Patch

Comment on attachment 179476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179285&action=review Thanks for the update. A couple more comments on the tests. > LayoutTests/fast/text-autosizing/form-controls-boosting-button-textarea-input-elements.html:14 > + font-size: 85%; This test is still far too complicated. For example the following are almost certainly irrelevant: - font-size: 85%; - "<span>| </span>\n" should be equivalent to just "|\n" - <form> <div> ... </div> </form> should be equivalent to just <div> ... </div> - id="quicksearch_top" - id="quicksearch" - name="quicksearch" - id="find" - class="txt" - class="btn" - <p><div> ... </div></p> should be equivalent to <p> ... </p> But that's just the low-level details. Ultimately even once you strip out the redundant information, this test will still be too complicated, because it's trying to do too many things at once. It might be easiest to just delete this test and create new ones in the style of form-controls-boosting-checkbox-input-element.html which just test text inputs and buttons respectively (when testing buttons, please make sure to test a variety of different button types, i.e. <input type="submit" value="Text">, <button>Text</button>, <input type="button" value="Text"> and <input type="reset" value="Text">; and similarly for text inputs, include type="text", type="email", type="number", type="password", type="search" and type="url" - all in their own divs of course so they don't affect each other). > LayoutTests/fast/text-autosizing/form-controls-boosting-radio-input-element.html:27 > +<div> Please indent your html tags (with spaces) so it's easier to see the tree structure. Ditto in other tests.

Comment on attachment 179476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179476&action=review > Source/WebCore/rendering/TextAutosizer.cpp:36 > +#include <wtf/StdLibExtras.h> Nit: most other code in WebKit includes <algorithm> before <wtf/*>. Probably because of http://www.webkit.org/coding/coding-style.html#include-others (i.e. 'a' is before 'w' when sorted). > Source/WebCore/rendering/TextAutosizer.cpp:52 > + // returns the tags for the form input elements http://www.webkit.org/coding/coding-style.html#comments-sentences > Source/WebCore/rendering/TextAutosizer.cpp:140 > +bool TextAutosizer::containerShouldBeAutosized(const RenderBlock* container) You still need to rebase this now http://wkbug.com/104925 has landed. > Source/WebCore/rendering/TextAutosizer.cpp:240 > + // Avoid creating containers for text within text controls, buttons, or <select> dropdowns Nit: full stop at end http://www.webkit.org/coding/coding-style.html#comments-sentences

Created attachment 179492 [details] Patch Work in progress.

Comment on attachment 179476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179476&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:36 >> +#include <wtf/StdLibExtras.h> > > Nit: most other code in WebKit includes <algorithm> before <wtf/*>. Probably because of http://www.webkit.org/coding/coding-style.html#include-others (i.e. 'a' is before 'w' when sorted). done >> Source/WebCore/rendering/TextAutosizer.cpp:52 >> + // returns the tags for the form input elements > > http://www.webkit.org/coding/coding-style.html#comments-sentences done >> Source/WebCore/rendering/TextAutosizer.cpp:140 >> +bool TextAutosizer::containerShouldBeAutosized(const RenderBlock* container) > > You still need to rebase this now http://wkbug.com/104925 has landed. done >> Source/WebCore/rendering/TextAutosizer.cpp:240 >> + // Avoid creating containers for text within text controls, buttons, or <select> dropdowns > > Nit: full stop at end http://www.webkit.org/coding/coding-style.html#comments-sentences done >> LayoutTests/fast/text-autosizing/form-controls-boosting-button-textarea-input-elements.html:14 >> + font-size: 85%; > > This test is still far too complicated. For example the following are almost certainly irrelevant: > - font-size: 85%; > - "<span>| </span>\n" should be equivalent to just "|\n" > - <form> <div> ... </div> </form> should be equivalent to just <div> ... </div> > - id="quicksearch_top" > - id="quicksearch" > - name="quicksearch" > - id="find" > - class="txt" > - class="btn" > - <p><div> ... </div></p> should be equivalent to <p> ... </p> > > But that's just the low-level details. Ultimately even once you strip out the redundant information, this test will still be too complicated, because it's trying to do too many things at once. It might be easiest to just delete this test and create new ones in the style of form-controls-boosting-checkbox-input-element.html which just test text inputs and buttons respectively (when testing buttons, please make sure to test a variety of different button types, i.e. <input type="submit" value="Text">, <button>Text</button>, <input type="button" value="Text"> and <input type="reset" value="Text">; and similarly for text inputs, include type="text", type="email", type="number", type="password", type="search" and type="url" - all in their own divs of course so they don't affect each other). done >> LayoutTests/fast/text-autosizing/form-controls-boosting-radio-input-element.html:27 >> +<div> > > Please indent your html tags (with spaces) so it's easier to see the tree structure. Ditto in other tests. done

Created attachment 179503 [details] Patch

Kenneth/Julien, we had a couple of iterations with John on this bug. Could you have a quick look please?

Comment on attachment 179503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179503&action=review > Source/WebCore/rendering/TextAutosizer.cpp:40 > #include <algorithm> > > +#include <wtf/StdLibExtras.h> > + Why is that include grouped separately? > Source/WebCore/rendering/TextAutosizer.cpp:53 > + > +static const Vector<QualifiedName>& formInputTags() > +{ > + // Returns the tags for the form input elements. Btw did you look into content-editable? What about input forms like date, email, etc? > Source/WebCore/rendering/TextAutosizer.cpp:216 > + // Avoid creating containers for text within text controls, buttons, or <select> dropdowns. > + Node* parentNode = renderer->parent() ? renderer->parent()->generatingNode() : 0; > + if (parentNode && parentNode->isElementNode() && formInputTags().contains(toElement(parentNode)->tagQName())) > + return false; What abut platforms that doesnt handle select dropdown inline? There is some kind of define for that > Source/WebCore/rendering/TextAutosizer.cpp:311 > +bool TextAutosizer::containerContainsTags(const RenderBlock* container, const Vector<QualifiedName>& tags) Name is confusing. so "contains tags" means all tags? or just one of them?

Comment on attachment 179503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179503&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:40 >> + > > Why is that include grouped separately? not sure what the policies regarding includes are in WebKit, but I've deleted one line now so it's grouped under <algorithm> hope that's ok >> Source/WebCore/rendering/TextAutosizer.cpp:53 >> + // Returns the tags for the form input elements. > > Btw did you look into content-editable? > > What about input forms like date, email, etc? content-editable is a good point, however I think it is best to handle it in a separate bug, because this bug explicitly addresses form input controls input fields like date, and other should be fine, in fact they are included in the tests with this patch. >> Source/WebCore/rendering/TextAutosizer.cpp:216 >> + return false; > > What abut platforms that doesnt handle select dropdown inline? There is some kind of define for that not sure if I understand correctly: do you mean on platforms like blackberry where it looks like a customized implementation of SelectPopupClient.h is provided? Does this suggest that in that case testing the parent node is not sufficient? I've been trying to find a related define in HTMLSelectElement.* and in the rendering/ directory but no success so far.. >> Source/WebCore/rendering/TextAutosizer.cpp:311 >> +bool TextAutosizer::containerContainsTags(const RenderBlock* container, const Vector<QualifiedName>& tags) > > Name is confusing. so "contains tags" means all tags? or just one of them? renamed to containerContainsOneOfTags(..)

Comment on attachment 179503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179503&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:53 >> + // Returns the tags for the form input elements. > > Btw did you look into content-editable? > > What about input forms like date, email, etc? > Btw did you look into content-editable? I can think of cases where content-editable text would benefit from being autosized. We should probably look into that separately, along with textareas (see below). > What about input forms like date, email, etc? Those all use the <input> tag, so should already be covered? > Source/WebCore/rendering/TextAutosizer.cpp:57 > + formInputTags.append(textareaTag); Please either a) add a test for <textarea> tags, or b) remove them from this list if you didn't mean to disable autosizing of textareas. I'd recommend option b (allow them to be autosized), as large textareas can benefit from autosizing. They suffer from crbug.com/165481, but we can deal with that separately. >> Source/WebCore/rendering/TextAutosizer.cpp:216 >> + return false; > > What abut platforms that doesnt handle select dropdown inline? There is some kind of define for that Even if the select dropdown is handled by the platform, a dropdown button will still be shown, and this shouldn't be autosized (at least until such time as Text Autosizing can smartly resize controls). Perhaps the comment above should say "<select> buttons" instead of "<select> dropdowns"? > LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:27 > + This tests the autosizing of a button input element. This paragraph should be autosized, while It's not clear what "autosized" means (to future people who have to debug this test). Please give something that can be explicitly tested, e.g. set font-size: 16px in your style rules for body, and write "This paragraph should be autosized to 40px computed font-size (16 * 800/320), whereas the buttons below should be rendered at their default font size". Similarly for other tests. In fact it would be useful to have some text next to the buttons to demonstrate that their whole paragraph isn't being autosized. Ditto for your text inputs test (the others already have text, so are fine). > LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:33 > +<form action=""> This <form> tag is unnecessary (ditto in other tests). HTML doesn't require that inputs have a form tag ancestor.

Created attachment 179774 [details] Patch

Comment on attachment 179503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179503&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:57 >> + formInputTags.append(textareaTag); > > Please either a) add a test for <textarea> tags, or b) remove them from this list if you didn't mean to disable autosizing of textareas. > I'd recommend option b (allow them to be autosized), as large textareas can benefit from autosizing. They suffer from crbug.com/165481, but we can deal with that separately. done option b. >>>> Source/WebCore/rendering/TextAutosizer.cpp:216 >>>> + return false; >>> >>> What abut platforms that doesnt handle select dropdown inline? There is some kind of define for that >> >> not sure if I understand correctly: do you mean on platforms like blackberry where it looks like a customized implementation of SelectPopupClient.h is provided? >> Does this suggest that in that case testing the parent node is not sufficient? >> I've been trying to find a related define in HTMLSelectElement.* and in the rendering/ directory but no success so far.. > > Even if the select dropdown is handled by the platform, a dropdown button will still be shown, and this shouldn't be autosized (at least until such time as Text Autosizing can smartly resize controls). Perhaps the comment above should say "<select> buttons" instead of "<select> dropdowns"? done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:27 >> + This tests the autosizing of a button input element. This paragraph should be autosized, while > > It's not clear what "autosized" means (to future people who have to debug this test). Please give something that can be explicitly tested, e.g. set font-size: 16px in your style rules for body, and write "This paragraph should be autosized to 40px computed font-size (16 * 800/320), whereas the buttons below should be rendered at their default font size". Similarly for other tests. > > In fact it would be useful to have some text next to the buttons to demonstrate that their whole paragraph isn't being autosized. Ditto for your text inputs test (the others already have text, so are fine). done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:33 >> +<form action=""> > > This <form> tag is unnecessary (ditto in other tests). HTML doesn't require that inputs have a form tag ancestor. done

Created attachment 179806 [details] Patch

Comment on attachment 179806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179806&action=review Looks good with minor nits. > Source/WebCore/rendering/TextAutosizer.h:76 > + static bool contentHeightIsConstrained(const RenderBlock* container); Nit: the method order here doesn't match that in the .cpp file. It would also be nice if containerContainsOneOfTags and contentHeightIsConstrained were next to containerShouldBeAutosized, and measureDescendantTextWidth remained next to clusterShouldBeAutosized. So perhaps the following order would be good?: 1. containerShouldbeAutosized 2. containerContainsOneOfTags 3. contentHeightIsConstrained 4. clusterShouldBeAutosized 5. measureDescendantTextWidth > LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:29 > + whereas the buttons and the corresponding text below should be rendered at Nit: actually, we've set font-size:16px on the body (or html - see below), so it would probably be more accurate to say "whereas the buttons below should be rendered at their default font size, and the accompanying text below should remain 16px". Similarly for the text input and <select> tests; and similarly for the checkbox/radiobutton tests, except that s/default font size/default size/ for those as the controls don't display any text. > LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:8 > + font-size: 16px; Please set this font-size on the html element instead of the body element, so that when the -expected.html file uses the rem unit it's a multiple of the specified font size (ditto in all tests and test expectations). > LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:35 > +<div> Nit: It doesn't really matter, but this div is technically unnecessary (since all its children are themselves divs, and it just lays them out vertically, which is the same as what the body element would have done if this div didn't exist). Ditto in form-controls-autosizing-button-input-elements. > LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:38 > + <input type="text"> I realise that all of these text input fields are blank. It would be nice to have at least one that contains some text, so you can easily see the field's text size. Perhaps add a case with: <input type="text" value="Default value">

Comment on attachment 179806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179806&action=review >> Source/WebCore/rendering/TextAutosizer.h:76 >> + static bool contentHeightIsConstrained(const RenderBlock* container); > > Nit: the method order here doesn't match that in the .cpp file. It would also be nice if containerContainsOneOfTags and contentHeightIsConstrained were next to containerShouldBeAutosized, and measureDescendantTextWidth remained next to clusterShouldBeAutosized. So perhaps the following order would be good?: > > 1. containerShouldbeAutosized > 2. containerContainsOneOfTags > 3. contentHeightIsConstrained > 4. clusterShouldBeAutosized > 5. measureDescendantTextWidth done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-button-input-elements.html:29 >> + whereas the buttons and the corresponding text below should be rendered at > > Nit: actually, we've set font-size:16px on the body (or html - see below), so it would probably be more accurate to say "whereas the buttons below should be rendered at their default font size, and the accompanying text below should remain 16px". Similarly for the text input and <select> tests; and similarly for the checkbox/radiobutton tests, except that s/default font size/default size/ for those as the controls don't display any text. done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:8 >> + font-size: 16px; > > Please set this font-size on the html element instead of the body element, so that when the -expected.html file uses the rem unit it's a multiple of the specified font size (ditto in all tests and test expectations). done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:35 >> +<div> > > Nit: It doesn't really matter, but this div is technically unnecessary (since all its children are themselves divs, and it just lays them out vertically, which is the same as what the body element would have done if this div didn't exist). Ditto in form-controls-autosizing-button-input-elements. done >> LayoutTests/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html:38 >> + <input type="text"> > > I realise that all of these text input fields are blank. It would be nice to have at least one that contains some text, so you can easily see the field's text size. Perhaps add a case with: <input type="text" value="Default value"> done

Created attachment 180137 [details] Patch

Created attachment 180139 [details] Patch

last patch contains the rebased changes wrt to the patches landed by Anton

Kenneth could you please have another glance at the latest patch. I've reworked it using the comments from John and merged it with upstream.

Comment on attachment 180139 [details] Patch Kenneth thanks for review, can we land this patch? a few mac tests are failing, but looks like to be unrelated to this patch.

Comment on attachment 180139 [details] Patch Clearing flags on attachment: 180139 Committed r138162: <http://trac.webkit.org/changeset/138162>

All reviewed patches have been landed. Closing bug.

I filed bug 106797 to follow-up on the (long term, lower priority) idea of scaling up form fields.