Bug 60689 – REGRESSION (r81992): layout triggered by position update fails to apply when transform is updated at same time

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shane Stephens]

See http://code.google.com/p/chromium/issues/detail?id=82225 for original report and reduced test case.

Regression seems to have been introduced in http://trac.webkit.org/changeset/81992. A synopsis of what seems to be happening is that the presence of an updated transform causes RenderObject::adjustStyleDifference to change the diff hint from StyleDifferenceLayoutPositionedMovementOnly to StyleDifferenceSimplifiedLayout (this functionality was introduced in 81992), which in turn prevents RenderBlock::simplifiedLayout from calling tryLayoutDoingPositionedMovementOnly(). It may be that the fix is as simple as updating the guard to calling tryLayoutDoingPositionedMovementOnly such that it is also called when the diff hint is StyleDifferenceSimplifiedLayout (and indeed this does fix the problem), however I don't know what other effects this might have.

<rdar://problem/9425874>

[Eric Seidel (no email)]

Thank you mitz.

[Shane Stephens]

Created attachment 93256 [details]
Patch

[Shane Stephens]

This patch fixes the issue and doesn't seem to introduce any new regressions.  I don't know whether it's the right approach though.

Also included in the patch is a new LayoutTest that exposes the issue.

[Julie Parent]

Comment on attachment 93256 [details]
Patch

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

> LayoutTests/transforms/2d/set-transform-and-top.html:23
> +        resultString += "PASS - Element " + id1 + " moved to y=300px";

This statement isn't necessarily true.  I agree that to make the test not flaky you shouldn't test for it to be exactly at 300, but when you print out this statement it sounds like you have verified that it is at 300, and you have just verified that it is > 250.  Also, providing the detail of "element a" probably isn't necessary, you can just say "element" or "box" or whatever.

> LayoutTests/transforms/2d/set-transform-and-top.html:39
> +<!-- You should see green box only. If you see red, the test has failed -->

Copy-paste left behind?

> LayoutTests/transforms/2d/set-transform-and-top.html:44
> +    if (window.layoutTestController) {

No need for brackets for single line if.  You can move the dumpAsText call here though too.

> LayoutTests/transforms/2d/set-transform-and-top.html:47
> +    setTimeout(move, 10);

Why is the setTimeout needed?

[Shane Stephens]

Created attachment 93421 [details]
Patch

[Shane Stephens]

Comment on attachment 93256 [details]
Patch

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

>> LayoutTests/transforms/2d/set-transform-and-top.html:23
>> +        resultString += "PASS - Element " + id1 + " moved to y=300px";
> 
> This statement isn't necessarily true.  I agree that to make the test not flaky you shouldn't test for it to be exactly at 300, but when you print out this statement it sounds like you have verified that it is at 300, and you have just verified that it is > 250.  Also, providing the detail of "element a" probably isn't necessary, you can just say "element" or "box" or whatever.

Fixed

>> LayoutTests/transforms/2d/set-transform-and-top.html:39
>> +<!-- You should see green box only. If you see red, the test has failed -->
> 
> Copy-paste left behind?

Yep.  Removed.

>> LayoutTests/transforms/2d/set-transform-and-top.html:44
>> +    if (window.layoutTestController) {
> 
> No need for brackets for single line if.  You can move the dumpAsText call here though too.

Removed braces.

>> LayoutTests/transforms/2d/set-transform-and-top.html:47
>> +    setTimeout(move, 10);
> 
> Why is the setTimeout needed?

The bug doesn't trigger on a full repaint, only on a particular optimisation pathway.  Without a setTimeout here the script runs as part of the document load and before the first redraw, so the bug doesn't trigger.

[Hajime Morrita]

The patch is small and doing just a conservative change.
Any layout expert will be able to give a glace easily.

[Hajime Morrita]

(In reply to comment #8)
> The patch is small and doing just a conservative change.
> Any layout expert will be able to give a glace easily.
                                                               ^^^^^ meant to say glance

[James Robinson]

Comment on attachment 93421 [details]
Patch

Not quite - theres code on RenderBlock.cpp:2133 to try to do simplifiedNormalFlowLayout().  should the check be needsPositionedMovementLayout() && !needsSimplifiedNormalFlowLayout()? maybe needsPositionedMovementLayout() should be false when needsSimplifiedNormalFlowLayout() is true?

[Shane Stephens]

Created attachment 94887 [details]
Patch

[James Robinson]

Comment on attachment 94887 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 94887 [details]
Patch

Clearing flags on attachment: 94887

Committed r87368: <http://trac.webkit.org/changeset/87368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87368 cherry-picked into qtwebkit-2.2 with commit 7d0d05a <http://gitorious.org/webkit/qtwebkit/commit/7d0d05a>

[James Robinson]

Sorry Shane, I mislead you!  Hyatt explained that the two bits are actually independent and it's wrong to call tryLayout...() on an object that needs simplified normal flow layout but is not positioned.  Instead, we gotta figure out why this object does not have the needs positioned movement layout bit set.

Will back this patch out while we think 'bout it.

[Dave Hyatt]

Reopening.

[Dave Hyatt]

Need a new style difference so that you can set both of these bits if needed.

[Shane Stephens]

Created attachment 95067 [details]
Patch

[Shane Stephens]

Created attachment 95075 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95075 [details]
Patch

Clearing flags on attachment: 95075

Committed r87475: <http://trac.webkit.org/changeset/87475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87475 cherry-picked into qtwebkit-2.2 with commit f201b09 <http://gitorious.org/webkit/qtwebkit/commit/f201b09>