Bug 125756 - [CSSRegions] Incorrect repaint of fixed element with transformed parent
Summary: [CSSRegions] Incorrect repaint of fixed element with transformed parent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords:
Depends on: 125863
Blocks: 57312
  Show dependency treegraph
 
Reported: 2013-12-14 22:45 PST by Mihnea Ovidenie
Modified: 2014-01-14 22:39 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.08 KB, patch)
2013-12-14 22:55 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (11.30 KB, patch)
2013-12-17 09:14 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (11.70 KB, patch)
2013-12-17 10:08 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 3 (19.65 KB, patch)
2014-01-09 08:15 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch (13.76 KB, patch)
2014-01-14 08:26 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2013-12-14 22:45:45 PST
When a fixed positioned element in a named flow has an ancestor that dynamically receives/loses a transform, the fixed positioned element is incorrectly repainted wrt its modified containing block.
Comment 1 Mihnea Ovidenie 2013-12-14 22:55:11 PST
Created attachment 219272 [details]
Patch
Comment 2 Mihai Maerean 2013-12-16 07:02:51 PST
Comment on attachment 219272 [details]
Patch

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

> LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:6
> +                window.internals.settings.setAcceleratedCompositingForFixedPositionEnabled(false);

What is the purpose of this line?
You could add the reason as a comment in the code.

> LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:17
> +        <p>That that a fixed positioned element whose parent gets a 3d transform dynamically is correctly re-positioned relative to the parent.</p>

You probably meant to say that "<p>Tests that a ...".

> LayoutTests/fast/regions/fixed-in-named-flow-cb-changed2-expected.html:14
> +        <p>That that a fixed positioned element whose parent dynamically looses its transformation is correctly re-positioned relative to the view.</p>

You probably meant to say that "<p>Tests that a ...".
Comment 3 Mihnea Ovidenie 2013-12-16 10:32:14 PST
(In reply to comment #2)
> (From update of attachment 219272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219272&action=review
> 
> > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:6
> > +                window.internals.settings.setAcceleratedCompositingForFixedPositionEnabled(false);
> 
> What is the purpose of this line?
> You could add the reason as a comment in the code.

At this moment, accelerated compositing is not yet enabled for fixed positioned elements that are part of the named flow.

> 
> > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:17
> > +        <p>That that a fixed positioned element whose parent gets a 3d transform dynamically is correctly re-positioned relative to the parent.</p>
> 
> You probably meant to say that "<p>Tests that a ...".
> 
> > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed2-expected.html:14
> > +        <p>That that a fixed positioned element whose parent dynamically looses its transformation is correctly re-positioned relative to the view.</p>
> 
> You probably meant to say that "<p>Tests that a ...".

Right, thanks for catching this. I will change when i will land the patch (after review).
Comment 4 Mihnea Ovidenie 2013-12-17 09:14:15 PST
Created attachment 219425 [details]
Patch 2
Comment 5 Darin Adler 2013-12-17 09:23:52 PST
Comment on attachment 219425 [details]
Patch 2

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

> Source/WebCore/rendering/FlowThreadController.cpp:290
> +// The list of layers is returned already sorted by z-Index.

I think the word “already” only makes sense here if you know how the code used to work and so should be removed. The “I” in “z-index” should not be capitalized.

> Source/WebCore/rendering/FlowThreadController.cpp:301
> +        // Iterate over the lists of fixed positioned layers collected at flow thread layer level

I don’t understand how this comment adds to the readability of the code. A for loop already says “iterate”. The if statements already says “fixed position” and the variable name also says “fixed positioned layers”, albeit with “positioned” abbreviated to “pos”. The other variable name says “flow thread layer”. The comment just repeats exactly what the code says. Please omit such comments.

> Source/WebCore/rendering/FlowThreadController.cpp:323
> +    // Sort the fixed layers list

Same thing about this comment. Fine to write this as a placeholder before writing the code, but once you write the code the comment seems superfluous.
Comment 6 Mihnea Ovidenie 2013-12-17 10:08:39 PST
Created attachment 219429 [details]
Patch for landing
Comment 7 Mihnea Ovidenie 2013-12-17 10:10:05 PST
(In reply to comment #5)
> (From update of attachment 219425 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219425&action=review
> 
> > Source/WebCore/rendering/FlowThreadController.cpp:290
> > +// The list of layers is returned already sorted by z-Index.
> 
> I think the word “already” only makes sense here if you know how the code used to work and so should be removed. The “I” in “z-index” should not be capitalized.
> 
> > Source/WebCore/rendering/FlowThreadController.cpp:301
> > +        // Iterate over the lists of fixed positioned layers collected at flow thread layer level
> 
> I don’t understand how this comment adds to the readability of the code. A for loop already says “iterate”. The if statements already says “fixed position” and the variable name also says “fixed positioned layers”, albeit with “positioned” abbreviated to “pos”. The other variable name says “flow thread layer”. The comment just repeats exactly what the code says. Please omit such comments.
> 
> > Source/WebCore/rendering/FlowThreadController.cpp:323
> > +    // Sort the fixed layers list
> 
> Same thing about this comment. Fine to write this as a placeholder before writing the code, but once you write the code the comment seems superfluous.

Thank you for the review. I removed the unnecessary comments and changed the first comment.
Comment 8 WebKit Commit Bot 2013-12-17 10:42:54 PST
Comment on attachment 219429 [details]
Patch for landing

Clearing flags on attachment: 219429

Committed r160717: <http://trac.webkit.org/changeset/160717>
Comment 9 WebKit Commit Bot 2013-12-17 10:42:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2013-12-17 11:11:29 PST
This seems to have broken 3 tests:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160717%20(1758)/results.html
Comment 11 WebKit Commit Bot 2013-12-17 11:45:33 PST
Re-opened since this is blocked by bug 125863
Comment 12 Alexey Proskuryakov 2013-12-17 12:11:43 PST
Rolled out in <http://trac.webkit.org/changeset/160720>.

I think that it's only the two new tests that were broken, fast/frames/sandboxed-iframe-navigation-allowed.html passed on the next run, so it's probably a flake.
Comment 13 Alexey Proskuryakov 2013-12-17 12:21:27 PST
Looks like there were assertion failures too:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160718%20(1142)/results.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010bb7cb2a WTFCrash + 42 (Assertions.cpp:341)
1   com.apple.WebCore             	0x000000010da55f42 WebCore::RenderLayer::negZOrderList() const + 82 (RenderLayer.h:550)
2   com.apple.WebCore             	0x000000010da54c5f WebCore::FlowThreadController::collectFixedPositionedLayers(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>&) const + 223 (FlowThreadController.cpp:302)
3   com.apple.WebCore             	0x000000010e8ac488 WebCore::RenderLayer::paintFixedLayersInNamedFlows(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 136 (RenderLayer.cpp:4048)
4   com.apple.WebCore             	0x000000010e8ab79b WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 2347 (RenderLayer.cpp:4185)
5   com.apple.WebCore             	0x000000010e8aae68 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 264 (RenderLayer.cpp:3896)
6   com.apple.WebCore             	0x000000010e8a9d3c WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 1612 (RenderLayer.cpp:3877)
Comment 14 Mihnea Ovidenie 2013-12-17 22:59:13 PST
(In reply to comment #13)
> Looks like there were assertion failures too:
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160718%20(1142)/results.html
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore          0x000000010bb7cb2a WTFCrash + 42 (Assertions.cpp:341)
> 1   com.apple.WebCore                 0x000000010da55f42 WebCore::RenderLayer::negZOrderList() const + 82 (RenderLayer.h:550)
> 2   com.apple.WebCore                 0x000000010da54c5f WebCore::FlowThreadController::collectFixedPositionedLayers(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>&) const + 223 (FlowThreadController.cpp:302)
> 3   com.apple.WebCore                 0x000000010e8ac488 WebCore::RenderLayer::paintFixedLayersInNamedFlows(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 136 (RenderLayer.cpp:4048)
> 4   com.apple.WebCore                 0x000000010e8ab79b WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 2347 (RenderLayer.cpp:4185)
> 5   com.apple.WebCore                 0x000000010e8aae68 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 264 (RenderLayer.cpp:3896)
> 6   com.apple.WebCore                 0x000000010e8a9d3c WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 1612 (RenderLayer.cpp:3877)

Thanks, investigating. Looks like it hits the assertion only on WebKit1.
Comment 15 Mihnea Ovidenie 2014-01-09 08:15:21 PST
Created attachment 220740 [details]
Patch 3
Comment 16 Mihnea Ovidenie 2014-01-10 11:36:35 PST
Darin, would you like to review this one too please? I managed to fix the assertions in the debug build from the previous fix you reviewed as well as fixing the paths in the tests.

Thank you
Comment 17 Mihnea Ovidenie 2014-01-14 08:26:25 PST
Created attachment 221165 [details]
Patch
Comment 18 WebKit Commit Bot 2014-01-14 22:39:29 PST
Comment on attachment 221165 [details]
Patch

Clearing flags on attachment: 221165

Committed r162050: <http://trac.webkit.org/changeset/162050>
Comment 19 WebKit Commit Bot 2014-01-14 22:39:32 PST
All reviewed patches have been landed.  Closing bug.