Bug 96848 – Text Autosizing: Ignore constrained heights in certain circumstances.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[John Mellor]

Text Autosizing: Ignore constrained heights in certain circumstances.

[John Mellor]

Created attachment 164262 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164262 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:93
> +        if (style->height().isSpecified() || style->maxHeight().isSpecified()) {
> +            // Some sites (wikipedia) accidentally set their html and/or body
> +            // elements to height:100%, without intending to constrain the
> +            // height of the content within them.
> +            return !container->isRoot() && !container->isBody();

Wouldn't it be better to contact wikipedia? I know some people there.

[Adam Barth]

> Wouldn't it be better to contact wikipedia? I know some people there.

If the issue is in mediawiki, that code tends to be copied to lots of web sites and is difficult to update.

[John Mellor]

> Wouldn't it be better to contact wikipedia? I know some people there.

The thing is wikipedia haven't done anything "wrong". Until they try and set a border or background* on the body element, there is no difference to having the body be only as tall as the viewport and its children overflow from it. It just happens to breaks Text Autosizing's constrained height heuristic.

A more general solution might be to try and detect cases where content is already overflowing from its containing block before Text Autosizing is applied, and jchaffraix and I explored this a little, however it ends up being nontrivial to implement (you can't just reuse layoutOverflowRect), so I'll file a follow-up bug about looking into that but this seems to be the more practical short term solution.

*: (by a "background on the body" I mean a background on both the html element and body element, such that the background on the body actually applies to the body element rather than being propagated to the root element; wikipedia do in fact have a background on the body, but they don't have one on the html so the background on the body doesn't actually apply to the body)

[Julien Chaffraix]

Comment on attachment 164262 [details]
Patch

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

r=me, please leave some time for Kenneth to object if he prefers to go with evangelism.

> so I'll file a follow-up bug about looking into that but this seems to be the more practical short term solution.

Please do, detecting the general case where we overflow will remove the need for this. As it will involve deeper changes (the currently tracked overflows are not sufficient for our needs), it's better to defer that until we can make a better assessment.

> Source/WebCore/ChangeLog:8
> +        Ignores constrained heights on html and body elements, as some sites

typo: Ignore?

> Source/WebCore/rendering/TextAutosizer.cpp:90
> +            // Some sites (wikipedia) accidentally set their html and/or body

Some sites (for example wikipedia) ...

>> Source/WebCore/rendering/TextAutosizer.cpp:93
>> +            return !container->isRoot() && !container->isBody();
> 
> Wouldn't it be better to contact wikipedia? I know some people there.

Evangelism would be really nice but reading Adam's comment, it really looks like we will need this change anyway.

> LayoutTests/fast/text-autosizing/constrained-then-float-ancestors.html:11
> +<script type="text/javascript">

No 'type' attribute needed. This applies to the other tests.

[John Mellor]

Created attachment 164590 [details]
Patch

Addressed jchaffraix's review nits.

[WebKit Review Bot]

Comment on attachment 164590 [details]
Patch

Rejecting attachment 164590 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13904247

[Julien Chaffraix]

Comment on attachment 164590 [details]
Patch

To ask only for cq?, you need to let webkit-patch fill the reviewer information in your ChangeLog from bugzilla.

[WebKit Review Bot]

Comment on attachment 164590 [details]
Patch

Clearing flags on attachment: 164590

Committed r128927: <http://trac.webkit.org/changeset/128927>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.