Bug 105188 – Text Autosizing - elements much narrower than its parent autosizing clusters should be autosized separately.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Anton Vayvod]

Text Autosizing - elements much narrower than its parent autosizing clusters should be autosized separately.

[Anton Vayvod]

Created attachment 179760 [details]
Patch

[Anton Vayvod]

Based on patch for bug 103627.

[Anton Vayvod]

Created attachment 179765 [details]
Patch

[John Mellor]

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.

[Anton Vayvod]

Created attachment 179788 [details]
Patch

[Anton Vayvod]

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.

[Anton Vayvod]

Created attachment 181491 [details]
Patch

[John Mellor]

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/

[John Mellor]

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?

[Kenneth Rohde Christiansen]

Comment on attachment 181491 [details]
Patch

LGTM with the nits fixed

[Anton Vayvod]

Created attachment 182155 [details]
Patch

[Anton Vayvod]

The nits are fixed.

[Anton Vayvod]

Hey Kenneth!

Could you r+ this, please?

Thanks in advance,
Anton.

[WebKit Review Bot]

Comment on attachment 182155 [details]
Patch

Clearing flags on attachment: 182155

Committed r139435: <http://trac.webkit.org/changeset/139435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.