Text Autosizing - elements much narrower than its parent autosizing clusters should be autosized separately.
Created attachment 179760 [details] Patch
Based on patch for bug 103627.
Created attachment 179765 [details] Patch
Comment on attachment 179760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179760&action=review Looks good. Some comments on the tests below. Also you need to add a ChangeLog for this patch. > Source/WebCore/rendering/TextAutosizer.cpp:217 > + // Additionally, any containers that are wider or at least 200px shhorter than s/shhorter/narrower/ > Source/WebCore/rendering/TextAutosizer.cpp:228 > + float parentBlockTextWidth = parentBlockContainingAllText->contentLogicalWidth(); "parentBlockTextWidth" sounds like "the width of the text of the parent block", when really it is "the width of the blockContainingAllText of the enclosing cluster" which is quite different. How about calling this "clusterTextWidth"? > LayoutTests/fast/text-autosizing/cluster-narrow-in-wide-expected.html:15 > This text should be autosized to just 20px computed font size (16 * 400/320), since the float:left causes this to be a new cluster, and it is only 400px wide. This div is supposed to be testing that float:left makes this a cluster. But really it'd happen anyway due to the width now. See suggestion below. > LayoutTests/fast/text-autosizing/cluster-narrow-in-wide-expected.html:-18 > -<div style="width: 320px; font-size: 2.5rem"> It would be nice to have two versions of this paragraph - one with width:600px which exhibits the old behavior. I checked, and you should be able to fit the following 4 divs onscreen at once: <div style="width: 600px; float: left; font-size: 1.875rem"> ... </div> <div style="width: 400px; font-size: 1.25rem"> ... </div> <div style="width: 600px; font-size: 2.5rem"> ... </div> <div style="font-size: 2.5rem"> ... </div> > LayoutTests/fast/text-autosizing/narrow-child.html:25 > <div style="width: 320px"> Might be easiest to make this width:600px, since this was initially supposed to be a simple cluster-less test, and cluster-narrow-in-wide already covers the case where we expect the narrow child to make a new cluster.
Created attachment 179788 [details] Patch
Comment on attachment 179760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179760&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:217 >> + // Additionally, any containers that are wider or at least 200px shhorter than > > s/shhorter/narrower/ Done >> Source/WebCore/rendering/TextAutosizer.cpp:228 >> + float parentBlockTextWidth = parentBlockContainingAllText->contentLogicalWidth(); > > "parentBlockTextWidth" sounds like "the width of the text of the parent block", when really it is "the width of the blockContainingAllText of the enclosing cluster" which is quite different. How about calling this "clusterTextWidth"? Done. >> LayoutTests/fast/text-autosizing/cluster-narrow-in-wide-expected.html:15 >> This text should be autosized to just 20px computed font size (16 * 400/320), since the float:left causes this to be a new cluster, and it is only 400px wide. > > This div is supposed to be testing that float:left makes this a cluster. But really it'd happen anyway due to the width now. See suggestion below. Done. >> LayoutTests/fast/text-autosizing/cluster-narrow-in-wide-expected.html:-18 >> -<div style="width: 320px; font-size: 2.5rem"> > > It would be nice to have two versions of this paragraph - one with width:600px which exhibits the old behavior. I checked, and you should be able to fit the following 4 divs onscreen at once: > > <div style="width: 600px; float: left; font-size: 1.875rem"> ... </div> > <div style="width: 400px; font-size: 1.25rem"> ... </div> > <div style="width: 600px; font-size: 2.5rem"> ... </div> > <div style="font-size: 2.5rem"> ... </div> Done. >> LayoutTests/fast/text-autosizing/narrow-child.html:25 >> <div style="width: 320px"> > > Might be easiest to make this width:600px, since this was initially supposed to be a simple cluster-less test, and cluster-narrow-in-wide already covers the case where we expect the narrow child to make a new cluster. Done.
Created attachment 181491 [details] Patch
Comment on attachment 181491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181491&action=review Minor nits with the test. > LayoutTests/fast/text-autosizing/narrow-child-expected.html:16 > + <div style="width: 600px; font-size: 2.5rem"> Nit: you don't need the font-size: 2.5rem, since the font size from the parent element is inherited. > LayoutTests/fast/text-autosizing/narrow-child-expected.html:17 > + This text should be autosized to 40px computed font-size as part of the same cluster as the div above.<br> Please use spaces not tabs to indent. Also s/as part/as it is part/ and s/div above/parent div/ > LayoutTests/fast/text-autosizing/narrow-child-expected.html:20 > + This text is autosized to 40px computed font-size, similarly to the text at the top.<br> Nit: s/is/should be/
Code looks good to me, apart from the very minor nits above. This significantly improves the rendering of a large number of sites. The only downside is that is causes us to regress bug 102409, but we have a plan for fixing that later, and it's not worth blocking all the sites that this does fix. Kenneth/Julien, do you think this is ready?
Comment on attachment 181491 [details] Patch LGTM with the nits fixed
Created attachment 182155 [details] Patch
The nits are fixed.
Hey Kenneth! Could you r+ this, please? Thanks in advance, Anton.
Comment on attachment 182155 [details] Patch Clearing flags on attachment: 182155 Committed r139435: <http://trac.webkit.org/changeset/139435>
All reviewed patches have been landed. Closing bug.