Bug 96143 - Text Autosizing: Don't autosize text in constrained height elements.
Summary: Text Autosizing: Don't autosize text in constrained height elements.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Mellor
URL:
Keywords:
Depends on:
Blocks: FontBoosting 96557 96848
  Show dependency treegraph
 
Reported: 2012-09-07 13:50 PDT by John Mellor
Modified: 2012-09-14 19:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (16.93 KB, patch)
2012-09-07 14:03 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (16.52 KB, patch)
2012-09-10 11:26 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (31.64 KB, patch)
2012-09-10 16:58 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (31.57 KB, patch)
2012-09-10 17:06 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (31.64 KB, patch)
2012-09-10 17:08 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (42.29 KB, patch)
2012-09-11 21:38 PDT, John Mellor
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Mellor 2012-09-07 13:50:15 PDT
Text Autosizing: Don't autosize text in constrained height elements.
Comment 1 John Mellor 2012-09-07 14:03:51 PDT
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.
Comment 2 Julien Chaffraix 2012-09-10 10:53:11 PDT
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?
Comment 3 John Mellor 2012-09-10 11:26:01 PDT
Created attachment 163170 [details]
Patch

Addressed jchaffraix's review comments.
Comment 4 John Mellor 2012-09-10 11:26:54 PDT
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...
Comment 5 Julien Chaffraix 2012-09-10 13:45:14 PDT
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.
Comment 6 John Mellor 2012-09-10 16:54:47 PDT
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).
Comment 7 John Mellor 2012-09-10 16:58:42 PDT
Created attachment 163248 [details]
Patch

Addressed jchaffraix's 2nd round review comments.
Comment 8 John Mellor 2012-09-10 17:06:22 PDT
Created attachment 163252 [details]
Patch

Minor tweak to a test.
Comment 9 John Mellor 2012-09-10 17:08:36 PDT
Created attachment 163253 [details]
Patch

Undo minor tweak to a test :)
Comment 10 Julien Chaffraix 2012-09-11 11:06:12 PDT
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.
Comment 11 John Mellor 2012-09-11 21:36:33 PDT
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).
Comment 12 John Mellor 2012-09-11 21:38:10 PDT
Created attachment 163515 [details]
Patch

Addressed jchaffraix's 3rd round review comments.
Comment 13 WebKit Review Bot 2012-09-12 08:47:16 PDT
Comment on attachment 163515 [details]
Patch

Clearing flags on attachment: 163515

Committed r128317: <http://trac.webkit.org/changeset/128317>
Comment 14 WebKit Review Bot 2012-09-12 08:47:21 PDT
All reviewed patches have been landed.  Closing bug.