Bug 104702

Created attachment 178849 [details] Patch Consider inline-block and inline-table elements to be autosizing clusters.

Created attachment 178850 [details] Patch

Comment on attachment 178850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178850&action=review Looks great, only minor nits. > Source/WebCore/ChangeLog:8 > + The elements with the display property set to either inline-block or inline-table often behave "behave differently" is a little vague :) How about: "A common pattern for creating adjacent columns in CSS is to set each of the columns to display:inline-block or display:inline-table. Whenever columns are used, Text Autosizing needs to assign each of the columns to different clusters (since the user can zoom in such that a column fills the width of the screen, so text within a column should have a smaller multiplier than it's wider enclosing cluster would have needed). This patch causes display:inline-block and display:inline-table to trigger new clusters." > Source/WebCore/rendering/TextAutosizer.cpp:207 > + // Exceptions are inline-block and inline-table elements. Please expand on this, e.g.: s/elements/elements, as they often contain entire multi-line columns of text/. > Source/WebCore/rendering/TextAutosizer.cpp:241 > + // Since isInlineBlockOrInlineTable() is overridden as a private method of the RenderBlock class Kenneth/Julien, what do you think about this? It's been a private override since Darin Adler's patch for bug 23522. Some of the things made hidden in that patch make sense (for example, you shouldn't be allowed to call isRenderBlock on a variable of type RenderBlock*, since you already know that it's a RenderBlock). But it doesn't seem to make sense for RenderBlock to hide isInlineBlockOrInlineTable, since it's a method that is frequently useful to call on RenderBlocks (indeed, RenderBlock itself uses the method extensively). > Source/WebCore/rendering/TextAutosizer.cpp:243 > + const RenderObject* rendererAsObject = renderer; While you're at it, could you rename |renderer| to |container| and |rendererAsObject| to |containerAsObject| ? "renderer" alone usually means a generic RenderObject, so this would be slightly clearer. > Source/WebCore/rendering/TextAutosizer.cpp:244 > + if (rendererAsObject->isInlineBlockOrInlineTable()) Nit: you could probably incorporate this into the return statement below (perhaps after isOutOfFlowPositioned)? Do whatever seems cleanest... > LayoutTests/ChangeLog:8 > + The elements with the display property set to either inline-block or inline-table often behave You don't need to repeat the explanation here - the description in LayoutTests/ChangeLog is only meant to talk about the changes to the tests. For example "Added a test to check that display:inline-block and display:inline-table trigger new clusters." > LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:31 > + </div></div> Please add "<!-- can't leave whitespace here, since these are inline -->" between </div> and <div> (ditto below, and in the -expected.html).

Created attachment 179278 [details] Patch

Comment on attachment 179278 [details] Patch Looks good to me. Kenneth/Julien, what do you think?

(In reply to comment #5) > Looks good to me. Kenneth/Julien, what do you think? By the way, we tested this on 1000 popular homepages and the effects were consistently net positive.

Comment on attachment 179278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179278&action=review > Source/WebCore/ChangeLog:8 > + A common pattern for creating adjacent columns in CSS is to set each of the columns to display:inline-block or display:inline-table. Whenever columns are used, Text Autosizing needs to assign each of the columns to different clusters (since the user can zoom in such that a column fills the width of the screen, so text within a column should have a smaller multiplier than it's wider enclosing cluster would have needed). You should wrap this sentence. Even if WebKit doesn't have a line limit, it still makes it more readable to break your text at some point. > Source/WebCore/ChangeLog:16 > + (WebCore::TextAutosizer::isAutosizingContainer): Consider display:inline-block or inline-table elements to be autosizing containers. > + (WebCore::TextAutosizer::isAutosizingCluster): Consider display:inline-block or inline-table elements to be autosizing clusters. You can factor this comment by putting it below the 2 lines. See other ChangeLog entries. > Source/WebCore/rendering/TextAutosizer.cpp:212 > + if (!renderer->isRenderBlock() || (renderer->isInline() && !renderer->isInlineBlockOrInlineTable())) That will probably need to account for CSS 3 displays (-webkit-inline-grid and -webkit-inline-flex) along with old flexbox (display: -webkit-inline-box). I would advise you to just use renderer->style()->isDisplayReplacedType() and extend your test. > Source/WebCore/rendering/TextAutosizer.cpp:219 > -bool TextAutosizer::isAutosizingCluster(const RenderBlock* renderer) > +bool TextAutosizer::isAutosizingCluster(const RenderBlock* container) I don't think it makes the code more readable to do this rename. > Source/WebCore/rendering/TextAutosizer.cpp:246 > + // Since isInlineBlockOrInlineTable() is overridden as a private method of the RenderBlock class > + // it can't be called via a pointer to a RenderBlock. > + const RenderObject* containerAsObject = container; > + if (containerAsObject->isInlineBlockOrInlineTable()) > + return true; This looks like you could make the function public or just replace this with the content of the function. Also see above for a probably better fix. > LayoutTests/fast/text-autosizing/cluster-inline-block-or-table-expected.html:14 > + <div> > + <div style="font-size: 2.5rem"> There are tabs in this file which will not pass the commit-hook. We prefer the indentation of the file to match regular WebKit style. > LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:22 > + <div> Nit: Add the bug number and title. > LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:31 > + </div><!-- can't leave whitespace here, since these are inline --></div> Do you really need this comment? It seems useless to me. > LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:36 > + </div><!-- can't leave whitespace here, since these are inline --></div> Ditto.

Created attachment 179478 [details] Patch

Comment on attachment 179278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179278&action=review >> Source/WebCore/ChangeLog:8 >> + A common pattern for creating adjacent columns in CSS is to set each of the columns to display:inline-block or display:inline-table. Whenever columns are used, Text Autosizing needs to assign each of the columns to different clusters (since the user can zoom in such that a column fills the width of the screen, so text within a column should have a smaller multiplier than it's wider enclosing cluster would have needed). > > You should wrap this sentence. Even if WebKit doesn't have a line limit, it still makes it more readable to break your text at some point. Done. >> Source/WebCore/ChangeLog:16 >> + (WebCore::TextAutosizer::isAutosizingCluster): Consider display:inline-block or inline-table elements to be autosizing clusters. > > You can factor this comment by putting it below the 2 lines. See other ChangeLog entries. Done. >> Source/WebCore/rendering/TextAutosizer.cpp:212 >> + if (!renderer->isRenderBlock() || (renderer->isInline() && !renderer->isInlineBlockOrInlineTable())) > > That will probably need to account for CSS 3 displays (-webkit-inline-grid and -webkit-inline-flex) along with old flexbox (display: -webkit-inline-box). I would advise you to just use renderer->style()->isDisplayReplacedType() and extend your test. Thanks for the suggestion! Done. >> Source/WebCore/rendering/TextAutosizer.cpp:219 >> +bool TextAutosizer::isAutosizingCluster(const RenderBlock* container) > > I don't think it makes the code more readable to do this rename. Reverted. >> Source/WebCore/rendering/TextAutosizer.cpp:246 >> + return true; > > This looks like you could make the function public or just replace this with the content of the function. Also see above for a probably better fix. Chose the better fix. >> LayoutTests/fast/text-autosizing/cluster-inline-block-or-table-expected.html:14 >> + <div style="font-size: 2.5rem"> > > There are tabs in this file which will not pass the commit-hook. We prefer the indentation of the file to match regular WebKit style. Done. >> LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:22 >> + <div> > > Nit: Add the bug number and title. Added a title with a link to the bug. Seems like what some other tests do. >> LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:31 >> + </div><!-- can't leave whitespace here, since these are inline --></div> > > Do you really need this comment? It seems useless to me. Removed. >> LayoutTests/fast/text-autosizing/cluster-inline-block-or-table.html:36 >> + </div><!-- can't leave whitespace here, since these are inline --></div> > > Ditto. Done.

Comment on attachment 179478 [details] Patch Looks good. What do you think Julien?

Comment on attachment 179478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179478&action=review The change is great. I was about to r+ but I think we can do a better job at testing. > LayoutTests/ChangeLog:13 > + * fast/text-autosizing/cluster-inline-block-or-table-expected.html: Added. > + * fast/text-autosizing/cluster-inline-block-or-table.html: Added. > + * fast/text-autosizing/cluster-inline-grid-flex-box-expected.html: Added. > + * fast/text-autosizing/cluster-inline-grid-flex-box.html: Added. All your test files contain tabs which are not allowed in WebKit and they will get rejected by the svn commit hook. > LayoutTests/fast/text-autosizing/cluster-inline-grid-flex-box-expected.html:15 > + <div style="display: -webkit-inline-grid; width: 600px; font-size: 1.875rem"> I really wonder if we need reftests here. You just need to check the font size of these elements which can be done with a variant of check-layout.js that checks getComputedStyle(element).fontSize Btw, using rem makes the test *less* readable that using absolute length, especially since you mention what you expect below.

(In reply to comment #11) > I really wonder if we need reftests here. You just need to check the font size of these elements which can be done with a variant of check-layout.js that checks getComputedStyle(element).fontSize While they are slightly overkill in some cases, using reftests for Text Autosizing has caught a variety of subtle issues that we'd have missed, like divs having too much margin, or line-height not matching font-size correctly, so they do have advantages... > Btw, using rem makes the test *less* readable that using absolute length, especially since you mention what you expect below. I'm not convinced about rems either - while it makes sense for the value to match the text, it's useful for the textAutosizingMultiplier to be exposed somewhere. i.e. it's ultimately more interesting that we expect the text to have been multiplied by 2x than that we expect it to be 32px because it happened to start off at 16px...

Comment on attachment 179478 [details] Patch OK! I don't need to see the updated change.

Created attachment 179512 [details] Patch

Created attachment 179513 [details] Patch

Comment on attachment 179513 [details] Patch Clearing flags on attachment: 179513 Committed r137764: <http://trac.webkit.org/changeset/137764>

All reviewed patches have been landed. Closing bug.