Bug 125756 – [CSSRegions] Incorrect repaint of fixed element with transformed parent

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

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.

[Mihnea Ovidenie]

Created attachment 219272 [details]
Patch

[Mihai Maerean]

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 ...".

[Mihnea Ovidenie]

(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).

[Mihnea Ovidenie]

Created attachment 219425 [details]
Patch 2

[Darin Adler]

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.

[Mihnea Ovidenie]

Created attachment 219429 [details]
Patch for landing

[Mihnea Ovidenie]

(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.

[WebKit Commit Bot]

Comment on attachment 219429 [details]
Patch for landing

Clearing flags on attachment: 219429

Committed r160717: <http://trac.webkit.org/changeset/160717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This seems to have broken 3 tests:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160717%20(1758)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 125863

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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)

[Mihnea Ovidenie]

(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.

[Mihnea Ovidenie]

Created attachment 220740 [details]
Patch 3

[Mihnea Ovidenie]

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

[Mihnea Ovidenie]

Created attachment 221165 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 221165 [details]
Patch

Clearing flags on attachment: 221165

Committed r162050: <http://trac.webkit.org/changeset/162050>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.