Summary: | REGRESSION (r81992): layout triggered by position update fails to apply when transform is updated at same time | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shane Stephens <shanestephens> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Shane Stephens <shanestephens> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | adele, ademar, cmarrin, commit-queue, eric, hyatt, jamesr, mikelawther, mitz, morrita | ||||||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 61564 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Shane Stephens
2011-05-11 22:21:04 PDT
Thank you mitz. Created attachment 93256 [details]
Patch
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 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? Created attachment 93421 [details]
Patch
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. The patch is small and doing just a conservative change. Any layout expert will be able to give a glace easily. (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 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?
Created attachment 94887 [details]
Patch
Comment on attachment 94887 [details]
Patch
R=me
Comment on attachment 94887 [details] Patch Clearing flags on attachment: 94887 Committed r87368: <http://trac.webkit.org/changeset/87368> All reviewed patches have been landed. Closing bug. Revision r87368 cherry-picked into qtwebkit-2.2 with commit 7d0d05a <http://gitorious.org/webkit/qtwebkit/commit/7d0d05a> 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. Reopening. Need a new style difference so that you can set both of these bits if needed. Created attachment 95067 [details]
Patch
Created attachment 95075 [details]
Patch
Comment on attachment 95075 [details] Patch Clearing flags on attachment: 95075 Committed r87475: <http://trac.webkit.org/changeset/87475> All reviewed patches have been landed. Closing bug. Revision r87475 cherry-picked into qtwebkit-2.2 with commit f201b09 <http://gitorious.org/webkit/qtwebkit/commit/f201b09> |