Bug 60689 - REGRESSION (r81992): layout triggered by position update fails to apply when transform is updated at same time
Summary: REGRESSION (r81992): layout triggered by position update fails to apply when ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Shane Stephens
URL:
Keywords: InRadar, Regression
Depends on: 61564
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-11 22:21 PDT by Shane Stephens
Modified: 2011-05-27 11:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2011-05-11 23:28 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2011-05-13 02:56 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2011-05-25 17:34 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2011-05-26 16:17 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2011-05-26 16:45 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shane Stephens 2011-05-11 22:21:04 PDT
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.
Comment 1 mitz@webkit.org 2011-05-11 22:25:40 PDT
<rdar://problem/9425874>
Comment 2 Eric Seidel 2011-05-11 22:29:44 PDT
Thank you mitz.
Comment 3 Shane Stephens 2011-05-11 23:28:20 PDT
Created attachment 93256 [details]
Patch
Comment 4 Shane Stephens 2011-05-11 23:38:18 PDT
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.
Comment 5 Julie Parent 2011-05-12 10:05:23 PDT
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?
Comment 6 Shane Stephens 2011-05-13 02:56:03 PDT
Created attachment 93421 [details]
Patch
Comment 7 Shane Stephens 2011-05-13 02:58:29 PDT
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.
Comment 8 Hajime Morrita 2011-05-24 21:32:23 PDT
The patch is small and doing just a conservative change.
Any layout expert will be able to give a glace easily.
Comment 9 Hajime Morrita 2011-05-24 21:52:30 PDT
(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
Comment 10 James Robinson 2011-05-25 16:41:49 PDT
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?
Comment 11 Shane Stephens 2011-05-25 17:34:30 PDT
Created attachment 94887 [details]
Patch
Comment 12 James Robinson 2011-05-25 17:38:22 PDT
Comment on attachment 94887 [details]
Patch

R=me
Comment 13 WebKit Commit Bot 2011-05-26 00:59:58 PDT
Comment on attachment 94887 [details]
Patch

Clearing flags on attachment: 94887

Committed r87368: <http://trac.webkit.org/changeset/87368>
Comment 14 WebKit Commit Bot 2011-05-26 01:00:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Ademar Reis 2011-05-26 08:58:30 PDT
Revision r87368 cherry-picked into qtwebkit-2.2 with commit 7d0d05a <http://gitorious.org/webkit/qtwebkit/commit/7d0d05a>
Comment 16 James Robinson 2011-05-26 14:32:35 PDT
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.
Comment 17 Dave Hyatt 2011-05-26 15:02:22 PDT
Reopening.
Comment 18 Dave Hyatt 2011-05-26 15:03:02 PDT
Need a new style difference so that you can set both of these bits if needed.
Comment 19 Shane Stephens 2011-05-26 16:17:34 PDT
Created attachment 95067 [details]
Patch
Comment 20 Shane Stephens 2011-05-26 16:45:33 PDT
Created attachment 95075 [details]
Patch
Comment 21 WebKit Commit Bot 2011-05-26 23:28:53 PDT
Comment on attachment 95075 [details]
Patch

Clearing flags on attachment: 95075

Committed r87475: <http://trac.webkit.org/changeset/87475>
Comment 22 WebKit Commit Bot 2011-05-26 23:29:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Ademar Reis 2011-05-27 11:15:50 PDT
Revision r87475 cherry-picked into qtwebkit-2.2 with commit f201b09 <http://gitorious.org/webkit/qtwebkit/commit/f201b09>