Bug 96143 – Text Autosizing: Don't autosize text in constrained height elements.

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

[John Mellor]

Text Autosizing: Don't autosize text in constrained height elements.

[John Mellor]

Created attachment 162862 [details]
Patch

Autosizing text within constained height elements usually goes badly, as the autosized text will wrap onto more lines, hence it will become taller and often overflow its container, breaking the page's layout.

This first patch is going to change slightly, as I plan to address the vertical writing mode FIXME before requesting commit, but uploading it now in case people want to take an initial look.

[Julien Chaffraix]

Comment on attachment 162862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162862&action=review

> Source/WebCore/rendering/TextAutosizer.cpp:141
> +bool TextAutosizer::contentHeightIsConstrained(const RenderObject* renderer)

It's better to keep the precision here: you are passed a RenderBox so you should accept a RenderBox.

> Source/WebCore/rendering/TextAutosizer.cpp:148
> +        if (renderer->isBlockFlow()) {

Why do you ignore non-block flow? (It's probably related to tables but it's unclear why you wouldn't consider table with fixed height)

> Source/WebCore/rendering/TextAutosizer.cpp:150
> +            if (style->overflowY() != OVISIBLE && style->overflowY() != OHIDDEN)

style()->overflowY() >= OSCROLL

(note that we usually handle overflowX but it's strictly equivalent to handling overflowY as you wrote it)

> Source/WebCore/rendering/TextAutosizer.cpp:158
> +        renderer = renderer->parent();
> +    } while (renderer);

It would be more readable as a |for| loop:

for (const RenderBox* currentObject = renderer; currentObject; currentObject = currentObject->containingBlock())

Please note that you shouldn't walk to your parent() but to your containingBlock() here as your parent may not be the one constraining the current box (for example for positioned objects). Using containingBlock, you ensures that everything is at least a RenderBox (per comment above).

> Source/WebCore/rendering/TextAutosizer.h:62
> +    static bool contentHeightIsConstrained(const RenderObject*);

Do we really need to expose this function to the rest of WebCore?

[John Mellor]

Created attachment 163170 [details]
Patch

Addressed jchaffraix's review comments.

[John Mellor]

Comment on attachment 162862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162862&action=review

>> Source/WebCore/rendering/TextAutosizer.cpp:141
>> +bool TextAutosizer::contentHeightIsConstrained(const RenderObject* renderer)
> 
> It's better to keep the precision here: you are passed a RenderBox so you should accept a RenderBox.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:148
>> +        if (renderer->isBlockFlow()) {
> 
> Why do you ignore non-block flow? (It's probably related to tables but it's unclear why you wouldn't consider table with fixed height)

Fixed (I needed it because I was erroneously using ->parent() instead of ->containingBlock(); though I think anything containingBlock() returns would actually be block flow anyway?)

>> Source/WebCore/rendering/TextAutosizer.cpp:150
>> +            if (style->overflowY() != OVISIBLE && style->overflowY() != OHIDDEN)
> 
> style()->overflowY() >= OSCROLL
> 
> (note that we usually handle overflowX but it's strictly equivalent to handling overflowY as you wrote it)

Done (I'm always worried that inequalities might break if someone reorders the constants, but I guess that's unlikely).

> Source/WebCore/rendering/TextAutosizer.cpp:152
> +            if (style->height().isFixed() || style->maxHeight().isFixed()

Actually, I should also treat percentage Lengths as fixed, so I've changed this to just use isSpecified (I got confused, because a percentage length font size or line height is a percentage of the font size, hence scales with the font size, whereas a percentage length here means a percentage of the size of its containing block, which is fixed).

>> Source/WebCore/rendering/TextAutosizer.cpp:158
>> +    } while (renderer);
> 
> It would be more readable as a |for| loop:
> 
> for (const RenderBox* currentObject = renderer; currentObject; currentObject = currentObject->containingBlock())
> 
> Please note that you shouldn't walk to your parent() but to your containingBlock() here as your parent may not be the one constraining the current box (for example for positioned objects). Using containingBlock, you ensures that everything is at least a RenderBox (per comment above).

Done (ah, containingBlock is what I was looking for (that's why I had that isBlockFlow check earlier).

>> Source/WebCore/rendering/TextAutosizer.h:62
>> +    static bool contentHeightIsConstrained(const RenderObject*);
> 
> Do we really need to expose this function to the rest of WebCore?

I'm not sure what you mean? It's private...

[Julien Chaffraix]

Comment on attachment 163170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163170&action=review

> Source/WebCore/rendering/TextAutosizer.h:62
> +    static bool contentHeightIsConstrained(const RenderBox*);

>> Do we really need to expose this function to the rest of WebCore?
> I'm not sure what you mean? It's private...

Simple, this function could have static linkage in TextAutosizer.cpp and not be exposed in the header. I would prefer if it was the case as there is no point in exposing it (even private) to the rest of WebCore.

> LayoutTests/ChangeLog:19
> +        * fast/text-autosizing/overflow-then-fixed-ancestor.html: Added.

All the tests should include a description of what is tested and what the expected outcome is.

> LayoutTests/fast/text-autosizing/fixed-and-overflow-ancestor-expected.html:15
> +        <div style="font-size: 2.5rem">

Why do you expect a font increase here? (due to max-height, I would have expected no boosting would happen)

> LayoutTests/fast/text-autosizing/overflow-then-fixed-ancestor-expected.html:13
> +<div style="overflow-y: auto">

You only test overflow: auto, it would have been better to test other values to catch regressions.

[John Mellor]

Comment on attachment 163170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163170&action=review

>> Source/WebCore/rendering/TextAutosizer.h:62
>> +    static bool contentHeightIsConstrained(const RenderBox*);
> 
> 

> Simple, this function could have static linkage in TextAutosizer.cpp and not be exposed in the header.

Done.

>> LayoutTests/ChangeLog:19
>> +        * fast/text-autosizing/overflow-then-fixed-ancestor.html: Added.
> 
> All the tests should include a description of what is tested and what the expected outcome is.

Done.

>> LayoutTests/fast/text-autosizing/fixed-and-overflow-ancestor-expected.html:15
>> +        <div style="font-size: 2.5rem">
> 
> Why do you expect a font increase here? (due to max-height, I would have expected no boosting would happen)

The overflow-y:auto means there is infinite available vertical height for the content, even though there is a max-height. Documented this in the test.

>> LayoutTests/fast/text-autosizing/overflow-then-fixed-ancestor-expected.html:13
>> +<div style="overflow-y: auto">
> 
> You only test overflow: auto, it would have been better to test other values to catch regressions.

Added tests of overflow:scroll, overflow:hidden and overflow:-webkit-paged-x, and some other tests (e.g. a percentage, a viewport-percentage height, and a fixed height applied directly to the block rather than to an ancestor).

[John Mellor]

Created attachment 163248 [details]
Patch

Addressed jchaffraix's 2nd round review comments.

[John Mellor]

Created attachment 163252 [details]
Patch

Minor tweak to a test.

[John Mellor]

Created attachment 163253 [details]
Patch

Undo minor tweak to a test :)

[Julien Chaffraix]

Comment on attachment 163253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163253&action=review

I would include a test for a constrained positioned element (ideally one for fixed and one for absolute). I don't need to see the follow-up patch.

> LayoutTests/ChangeLog:32
> +        * fast/text-autosizing/fixed-within-overflow-ancestor.html: Added.

As a whole, fixed should be removed from those test cases' names as it has a meaning of his own per our discussion 'constrained' is a better alternative.

> LayoutTests/fast/text-autosizing/fixed-and-overflow-auto-ancestor-expected.html:16
> +        <div style="font-size: 2.5rem">

Nit: we could put 40px here instead of using relative font size and removing the html selector. The upside is that you don't rely on the relative font size calculation (which may be something we would like to keep). Not repeated for the other tests.

> LayoutTests/fast/text-autosizing/fixed-within-overflow-ancestor.html:25
> +            This text should be not autosized, as autosizing usually causes text to wrap onto more lines, which might cause it to overflow the parent's max-height (the grandparent's overflow-y:auto doesn't prevent this).

It would be nice to specify and explicitly mention the expected font-size in this case too.

[John Mellor]

Comment on attachment 163253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163253&action=review

> I would include a test for a constrained positioned element (ideally one for fixed and one for absolute).

Done (added 3 tests).

>> LayoutTests/ChangeLog:32
>> +        * fast/text-autosizing/fixed-within-overflow-ancestor.html: Added.
> 
> As a whole, fixed should be removed from those test cases' names as it has a meaning of his own per our discussion 'constrained' is a better alternative.

Done.

>> LayoutTests/fast/text-autosizing/fixed-and-overflow-auto-ancestor-expected.html:16
>> +        <div style="font-size: 2.5rem">
> 
> Nit: we could put 40px here instead of using relative font size and removing the html selector. The upside is that you don't rely on the relative font size calculation (which may be something we would like to keep). Not repeated for the other tests.

Not sure what you mean here. I put 2.5rem instead of 40px in order to explicitly call out the fact that this is being multiplied by 2.5x from its specified size; it seems more opaque to just put a size here...

>> LayoutTests/fast/text-autosizing/fixed-within-overflow-ancestor.html:25
>> +            This text should be not autosized, as autosizing usually causes text to wrap onto more lines, which might cause it to overflow the parent's max-height (the grandparent's overflow-y:auto doesn't prevent this).
> 
> It would be nice to specify and explicitly mention the expected font-size in this case too.

Done (added "its computed font-size should remain 16px" to all negative tests).

[John Mellor]

Created attachment 163515 [details]
Patch

Addressed jchaffraix's 3rd round review comments.

[WebKit Review Bot]

Comment on attachment 163515 [details]
Patch

Clearing flags on attachment: 163515

Committed r128317: <http://trac.webkit.org/changeset/128317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.