Bug 104702 – Consider inline-block and inline-table elements to be autosizing clusters.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anton Vayvod]

Consider inline-block and inline-table elements to be autosizing clusters.

[Anton Vayvod]

Created attachment 178849 [details]
Patch

Consider inline-block and inline-table elements to be autosizing clusters.

[Anton Vayvod]

Created attachment 178850 [details]
Patch

[John Mellor]

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).

[Anton Vayvod]

Created attachment 179278 [details]
Patch

[John Mellor]

Comment on attachment 179278 [details]
Patch

Looks good to me. Kenneth/Julien, what do you think?

[John Mellor]

(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.

[Julien Chaffraix]

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.

[Anton Vayvod]

Created attachment 179478 [details]
Patch

[Anton Vayvod]

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.

[John Mellor]

Comment on attachment 179478 [details]
Patch

Looks good. What do you think Julien?

[Julien Chaffraix]

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.

[John Mellor]

(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...

[Julien Chaffraix]

Comment on attachment 179478 [details]
Patch

OK! I don't need to see the updated change.

[Anton Vayvod]

Created attachment 179512 [details]
Patch

[Anton Vayvod]

Created attachment 179513 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179513 [details]
Patch

Clearing flags on attachment: 179513

Committed r137764: <http://trac.webkit.org/changeset/137764>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.