Bug 43022 – 100% height elements to not respond to vertical browser rescaling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Gregg Tavares]

View the attached file in any Webkit based browser.

Vertically resize the browser to be **LARGER**.

Notice the image does not resize.

**NOTE**: You MUST RESIZE ONLY VERTICALLY!!!  Any resizing in the horizontal dimension does not show the bug.

In Windows this is easy to reproduce because you can scale a window by dragging the bottom of the window. In OSX it is much harder to reproduce by resizing the window.

To reliably reproduce on OSX 

1) view the page
2) choose Develop->Start Debugging JavaScript
3) if the debugging tools are not docked to the main window then dock them (icon in bottom left)
4) Drag the separator between the webpage and the debugging tools vertically up and release
5) Drag the separator between the webpage and the debugging tools vertically down and release.

Notice the image does not resize.

This bug effect all webpages that resize with 100% images, canvas, possibly other tags

[Gregg Tavares]

Created attachment 62651 [details]
small HTML file to demonstrate the bug.

small HTML file to demonstrate the bug.

[Denise Cheng]

Created attachment 66914 [details]
Patch

[Denise Cheng]

Created attachment 66918 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 66918 [details]
Patch

The test should not contain any red; red indicates failure. The best kind of testcase is one where a green box overlaps a red box on success (and some red is left showing on failure).

[Dave Hyatt]

Comment on attachment 66918 [details]
Patch

No, this is horrible performance-wise.  You don't want to do this when no kids depend on the height.

[Pierre-Antoine LaFayette]

Adding comments from IRC chat with dhyatt: 

[14:40] <dhyatt> Percentage intrinsic heights are evaluated with respect to the containing block's height, if that height is specified explicitly, or if the replaced element is absolutely positioned.

[14:40] <dhyatt> If neither of these conditions is met, then percentage values on such replaced elements cannot be resolved and such elements are assumed to have no intrinsic height.

[14:41] <plafayette> what happens if they have no intrinsic height?

[14:42] <dhyatt> "Specifies a percentage height. The percentage is calculated with respect to the height of the generated box's http://www.w3.org/TR/CSS21/visuren.html#containing-block. If the height of the containing block is not specified explicitly (i.e., it depends on content height), and this element is not absolutely positioned, the value computes to 'auto'"

[14:42] <dhyatt> so the image's height for sure should go to auto 

...

[14:46] <plafayette> dhyatt: what do you think the proper solution is?
 
[14:46] <dhyatt> well it must be some bug with the percentheightdescendant stuff 

...

[14:53] <dhyatt> plafayette: anyway just a guess but i would imagine that something is going wrong with the propagation of percent height descendant tracking because of that <div> with auto height

[14:54] <dhyatt> it may be related to the fact that our strict mode rendering is wrong 

[14:57] <dhyatt> looks like replaced elements not adding themselves to the percentheightdescendant thing

[14:57] <dhyatt> maybe mitz would know why

[14:57] <dhyatt> they're also not respecting quirks vs. strict stuff

[14:57] <dhyatt> like calcPercentageHeight is

[14:57] <dhyatt> so the bugs all appear to be in RenderBox::calcReplacedHeightUsing (the percentage case)

[14:57] <dhyatt> i think it needs to match calcPercentageHEight a bit more closely

[14:59] <dhyatt> firefox gets this all right in quirks and strict mode

[14:59] <dhyatt> so good to compare against

[Terry Anderson]

The following comments are based on the attached HTML file.  In RenderBlock::layoutBlock, relayoutChildren is set to true when the browser is vertically resized to be be smaller:

if (oldWidth != logicalWidth() || oldColumnWidth != desiredColumnWidth())
        relayoutChildren = true;

This is because oldWidth is 15 pixels larger than logicalWidth().  Is this difference in widths the result of some computation involving a scrollbar (which does not actually appear)?  

It seems this is why the bug can only be reproduced when the browser is resized vertically to be larger.

[Terry Anderson]

Created attachment 130828 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130828 [details]
Patch

Attachment 130828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11861361

New failing tests:
tables/mozilla_expected_failures/bugs/bug85016.html

[Julien Chaffraix]

Comment on attachment 130828 [details]
Patch

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

r- because of the tests should really be revised.

> Source/WebCore/rendering/RenderBox.cpp:2345
> +                    toRenderBlock(cb)->addPercentHeightDescendant(const_cast<RenderBox*>(this));

I think this should be moved after the cb = cb->containingBlock() to match the other invocations of addPercentHeightDescendant. Also because you may miss the |cb| when you break out of the |while|. Btw this should be tested if your 2 tests don't cover it.

> LayoutTests/ChangeLog:12
> +        * fast/replaced/vertical-resize-100percent-contents.html: Added.
> +        * fast/replaced/vertical-resize-100percent-image.html: Added.

I really don't understand why we need them to be pixel tests here. It really looks like a getComputedStyle() test would be enough. If not, a ref test would be better.

> LayoutTests/fast/replaced/vertical-resize-100percent-image.html:16
> +setTimeout(run, 1000);

Please don't check-in a test that takes 1 second to run! We have 25,000+ tests so imagine if all took 1 second to run!

You could wait for the load event either on the main frame or on a subframe and resize your frame and that would work AFAICT.

[Terry Anderson]

Created attachment 131086 [details]
Patch

[Terry Anderson]

(In reply to comment #11)
> Created an attachment (id=131086) [details]
> Patch

jchaffraix: Thank you for your previous feedback. Please let me know if my revised layout test is more appropriate.

[WebKit Review Bot]

Comment on attachment 131086 [details]
Patch

Attachment 131086 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894815

New failing tests:
tables/mozilla_expected_failures/bugs/bug85016.html

[Julien Chaffraix]

Comment on attachment 131086 [details]
Patch

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

The change and test looks very good. However tables/mozilla_expected_failures/bugs/bug85016.html has a different output. This test uses percent height too so it's likely not a coincidence. If this is a progression, I would need to see a rebaseline on at least one platform and a proper disabling on all platforms. If it's a regression, well, it should be solved.

> LayoutTests/fast/replaced/vertical-resize-100percent-element.html:48
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

No need for the waitUntilDone() / notifyDone(). By default DRT dumps after the load event and you add those only if you need to continue after.

[Terry Anderson]

Created attachment 131347 [details]
Actual results for tables/mozilla_expected_failures/bugs/bug85016.html

[Terry Anderson]

Created attachment 131349 [details]
Pretty diff for tables/mozilla_expected_failures/bugs/bug85016.html

[Julien Chaffraix]

The diff doesn't shock me. FF seems to have smaller tables and your patch makes us shrink them so it seems to go towards FF. I would compare the new output with other browsers and see if anything stands out.

Finding out where this difference comes from would be neat though (I would suspect the special case code for table-cell in RenderBox::computeReplacedLogicalHeightUsing).

[Terry Anderson]

Created attachment 131390 [details]
Patch

[Julien Chaffraix]

Comment on attachment 131390 [details]
Patch

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

tables/mozilla_expected_failures/bugs/bug85016.html should be Skipped on all platforms that don't have a updated result or you will turn the bots to red. The rest is fine.

> LayoutTests/ChangeLog:15
> +        * platform/chromium-linux/tables/mozilla_expected_failures/bugs/bug85016-expected.txt: Added.
> +              Rebaseline.

I bet that this baseline is shared with chromium-win and lives under Layout/platform/chromium-win/. I would rather see the other one rebaselined as it would show the diff. Also it would avoid temporarily skip the test on Chromium Windows.

[Terry Anderson]

Created attachment 131459 [details]
Patch

[Julien Chaffraix]

Comment on attachment 131459 [details]
Patch

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

> LayoutTests/ChangeLog:14
> +        * platform/chromium-linux/tables/mozilla_expected_failures/bugs/bug85016-expected.png: Removed.
> +        * platform/chromium-win/tables/mozilla_expected_failures/bugs/bug85016-expected.png:

This rebaseline is wrong. Look at the scrollbars!

I should have been clearer: we may share the text baseline but may not share the image baselines due to platform difference in painting native controls. In this case, the text baseline is the same, the image needs to regenerated on Chromium Win.

> LayoutTests/ChangeLog:16
> +        * platform/chromium-win/tables/mozilla_expected_failures/bugs/bug85016-expected.txt:
> +              Rebaseline for chromium.

Have you investigated why this test has a difference? I don't see any explanation as to why we are fine with updating the baselines and I would like one.

> LayoutTests/ChangeLog:22
> +        * platform/efl/Skipped:
> +        * platform/gtk/Skipped:
> +        * platform/mac/Skipped:
> +        * platform/qt/Skipped:
> +        * platform/win/Skipped:
> +              Skip the rebaselined test on all platforms except for chromium. 

You should skip it on Chromium too because it will fail:
* IMAGE+TEXT on Mac
* IMAGE on Windows

[Terry Anderson]

Created attachment 132138 [details]
Patch

[Julien Chaffraix]

Comment on attachment 132138 [details]
Patch

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

Did you mean to ask for commit-queue too?

> LayoutTests/platform/efl/Skipped:2436
>  tables/mozilla_expected_failures/
> +tables/mozilla_expected_failures/bugs/bug85016.html

This entry is unneeded as it is covered by the one just above (I guess that's why you put it just below). The tools are happy so am I.

> LayoutTests/platform/qt/Skipped:2071
> +tables/mozilla_expected_failures/bugs/bug85016.html

It's always nice to put a bug number so that port maintainers know what caused this change in baseline.

[Fady Samuel]

(In reply to comment #23)
> (From update of attachment 132138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132138&action=review
> 
> Did you mean to ask for commit-queue too?
> 
> > LayoutTests/platform/efl/Skipped:2436
> >  tables/mozilla_expected_failures/
> > +tables/mozilla_expected_failures/bugs/bug85016.html
> 
> This entry is unneeded as it is covered by the one just above (I guess that's why you put it just below). The tools are happy so am I.
> 
> > LayoutTests/platform/qt/Skipped:2071
> > +tables/mozilla_expected_failures/bugs/bug85016.html
> 
> It's always nice to put a bug number so that port maintainers know what caused this change in baseline.

Awesome! Terry, please address these comments, in a new patch, and I will cq+ it for you. Thanks!

[Terry Anderson]

Created attachment 132298 [details]
Patch

[Julien Chaffraix]

Comment on attachment 132298 [details]
Patch

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

> LayoutTests/fast/replaced/resources/vertical-resize-100percent-contents.html:29
> +    Layout test for <a href="https://bugs.webkit.org/show_bug.cgi?id=43022">https://bugs.webkit.org/show_bug.cgi?id=43022</a>.  Checks to see if an image having a percentage height is resized when its containing iframe is vertically resized.  This test only works in DumpRenderTree since it involves accessing the internal elements of an iframe.

Sorry I missed that previously: normally we have the bug information visible in the expected result (this is some important information). Care to move it out of the iframe into your main page?

[Terry Anderson]

Created attachment 132329 [details]
Patch

[Terry Anderson]

Created attachment 132337 [details]
Patch

[Julien Chaffraix]

Comment on attachment 132337 [details]
Patch

Thanks, Terry. I guess you are using webkit-patch to upload your patches, you can use the following option to just ask for commit-queue instead of having to manually change the flags:

webkit-patch upload -m "Patch for landing" --no-review --request-commit

[WebKit Review Bot]

Comment on attachment 132337 [details]
Patch

Clearing flags on attachment: 132337

Committed r111064: <http://trac.webkit.org/changeset/111064>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.