Bug 96848

Summary: Text Autosizing: Ignore constrained heights in certain circumstances.
Product: WebKit Reporter: John Mellor <johnme>
Component: Layout and RenderingAssignee: John Mellor <johnme>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, jchaffraix, kenneth, peter, satish, simon.fraser, twoimo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96143    
Bug Blocks: 84186    
Attachments:
Description Flags
Patch
none
Patch none

Description John Mellor 2012-09-14 18:39:32 PDT
Text Autosizing: Ignore constrained heights in certain circumstances.
Comment 1 John Mellor 2012-09-14 19:15:56 PDT
Created attachment 164262 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-09-15 08:19:19 PDT
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.
Comment 3 Adam Barth 2012-09-15 23:03:46 PDT
> 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.
Comment 4 John Mellor 2012-09-15 23:19:45 PDT
> 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)
Comment 5 Julien Chaffraix 2012-09-17 18:32:25 PDT
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.
Comment 6 John Mellor 2012-09-18 11:30:07 PDT
Created attachment 164590 [details]
Patch

Addressed jchaffraix's review nits.
Comment 7 WebKit Review Bot 2012-09-18 12:04:22 PDT
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
Comment 8 Julien Chaffraix 2012-09-18 12:09:23 PDT
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.
Comment 9 WebKit Review Bot 2012-09-18 13:40:33 PDT
Comment on attachment 164590 [details]
Patch

Clearing flags on attachment: 164590

Committed r128927: <http://trac.webkit.org/changeset/128927>
Comment 10 WebKit Review Bot 2012-09-18 13:40:37 PDT
All reviewed patches have been landed.  Closing bug.