Mix stacking contexts into the positioned content list #8266

mrobinson · 2015-10-30T15:19:25Z

Sometimes positioned content needs to be layered on top of stacking
contexts. The layer synthesis code can do this, but the current design
prevents it because stacking contexts are stored in a separate struct
member. In order to preserve tree order, mix stacking contexts into the
positioned content list, by adding a new StackingContextClass
DisplayItem. Such items do not have a base DisplayItem.

In some ways this simplifies the code, because we no longer have to
have a separate code path in the StackingContextLayerCreator.

mrobinson · 2015-10-30T15:20:56Z

@pcwalton r?

This is the main prerequisite for eliminating LAYERS_NEEDED_FOR_DESCENDANTS.

pcwalton · 2015-11-02T20:56:13Z

components/gfx/display_list/mod.rs

@@ -206,23 +204,31 @@ impl DisplayList {
    /// inefficient and should only be used for debugging.
    pub fn all_display_items(&self) -> Vec<DisplayItem> {
        let mut result = Vec::new();
+        fn add_all_display_items_for_item(result: &mut Vec<DisplayItem>, item: &DisplayItem) {


Maybe just call this flatten? That's the terminology WR uses for a similar operation.

pcwalton · 2015-11-02T21:02:25Z

components/gfx/display_list/mod.rs

+
+    fn has_negative_z_index(&self) -> bool {
+        if let &DisplayItem::StackingContextClass(ref stacking_context) = self {
+            return stacking_context.z_index < 0


nit: No need for return

pcwalton · 2015-11-02T21:03:40Z

components/gfx/display_list/mod.rs

-        match paint_context.transient_clip {
-            Some(ref transient_clip) if transient_clip == this_clip => {}
-            Some(_) | None => paint_context.push_transient_clip((*this_clip).clone()),
+    fn draw_into_context(&self, transform: &Matrix4, paint_context: &mut PaintContext) {


Can you explain why transform is needed here now?

Sure. transform is necessary to properly draw StackingContexts, because it is used to calculate a new transform based on the StackingContexts bounds (display items are positioned relative to stacking contexts). Before StackingContexts were drawn outside of DisplayItem::draw_into_context (in DisplayList::draw_into_context). Now that StackingContexts can be DisplayItems it is moved here and the transform must be included in the case that this item is a StackingContext item.

mrobinson · 2015-11-02T22:09:57Z

@pcwalton Thanks, as always, for the review comments. I think I've made all the changes you suggested in the latest version of the branch. Do you see any other issues here?


        Adds wpt css test for stacking order bug

Adding overflow:hidden to an absolute positioned div will change it's stacking order. referencing servo#8122

paulrouget · 2015-11-03T10:30:38Z

Not sure if you want to address this issue in this PR or not, but for the record: #8310

mrobinson · 2015-11-03T18:58:00Z

@paulrouget Thanks for finding that issue! I've fixed it in the latest version. I had omitted some code that was removed.

pcwalton · 2015-11-03T21:37:01Z

@bors-servo: r+

bors-servo · 2015-11-03T21:37:01Z

📌 Commit 5b3e953 has been approved by pcwalton

pcwalton · 2015-11-03T21:38:55Z

@bors-servo: r-

@mrobinson wants to add a test

pcwalton · 2015-11-03T21:40:22Z

@bors-servo: r+

bors-servo · 2015-11-03T21:40:23Z

📌 Commit 5b3e953 has been approved by pcwalton

bors-servo · 2015-11-03T23:28:27Z

⌛ Testing commit 5b3e953 with merge 61ef78e...


        Auto merge of #8266 - mrobinson:stacking-context-mix, r=pcwalton

Mix stacking contexts into the positioned content list Sometimes positioned content needs to be layered on top of stacking contexts. The layer synthesis code can do this, but the current design prevents it because stacking contexts are stored in a separate struct member. In order to preserve tree order, mix stacking contexts into the positioned content list, by adding a new StackingContextClass DisplayItem. Such items do not have a base DisplayItem. In some ways this simplifies the code, because we no longer have to have a separate code path in the StackingContextLayerCreator.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8266)

bors-servo · 2015-11-03T23:59:58Z

💔 Test failed - linux-rel

mrobinson · 2015-11-04T00:36:59Z

Looks like some tests are now passing with this change. I'll update the branch and reupload.


        Mix stacking contexts into the positioned content list

Sometimes positioned content needs to be layered on top of stacking contexts. The layer synthesis code can do this, but the current design prevents it because stacking contexts are stored in a separate struct member. In order to preserve tree order, mix stacking contexts into the positioned content list, by adding a new StackingContextClass DisplayItem. Such items do not have a base DisplayItem. In some ways this simplifies the code, because we no longer have to have a separate code path in the StackingContextLayerCreator. Fixes #7779. Fixes #7983. Fixes #8122. Fixes #8310.

mrobinson · 2015-11-04T01:50:58Z

@bors-servo: r=pcwalton

I confirmed with @pcwalton via IRC that he was okay with trying to land this again.

bors-servo · 2015-11-04T01:51:01Z

📌 Commit c1a38e2 has been approved by pcwalton

bors-servo · 2015-11-04T01:52:44Z

⌛ Testing commit c1a38e2 with merge 86e3add...


        Auto merge of #8266 - mrobinson:stacking-context-mix, r=pcwalton

Mix stacking contexts into the positioned content list Sometimes positioned content needs to be layered on top of stacking contexts. The layer synthesis code can do this, but the current design prevents it because stacking contexts are stored in a separate struct member. In order to preserve tree order, mix stacking contexts into the positioned content list, by adding a new StackingContextClass DisplayItem. Such items do not have a base DisplayItem. In some ways this simplifies the code, because we no longer have to have a separate code path in the StackingContextLayerCreator.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8266)

bors-servo · 2015-11-04T02:45:57Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Oct 30, 2015

mrobinson mentioned this pull request Oct 30, 2015

Mix stacking contexts into the positioned content list #8174

Closed

mrobinson force-pushed the mrobinson:stacking-context-mix branch 2 times, most recently from 88b6ca2 to 5c63287 Nov 2, 2015

This was referenced Nov 2, 2015

Stacking context issue with CSS transforms #7779

Closed

overflow:hidden changes the stacking order #8122

Closed

mrobinson force-pushed the mrobinson:stacking-context-mix branch from 5c63287 to 90a16a4 Nov 2, 2015

eefriedman assigned pcwalton Nov 2, 2015

pcwalton reviewed Nov 2, 2015
View changes

mrobinson force-pushed the mrobinson:stacking-context-mix branch from 90a16a4 to a2acb2a Nov 2, 2015

gilles-leblanc referenced this pull request in gilles-leblanc/servo Nov 3, 2015

Adds wpt css test for stacking order bug

b2ba72a

Adding overflow:hidden to an absolute positioned div will change it's stacking order. referencing servo#8122

This was referenced Nov 3, 2015

Stacking issue with iframe + overflow:hidden #8310

Closed

Servo + Browser.html: webplatform bugs browserhtml/browserhtml#562

Closed

mrobinson force-pushed the mrobinson:stacking-context-mix branch from a2acb2a to 5b3e953 Nov 3, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 3, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 4, 2015

mrobinson force-pushed the mrobinson:stacking-context-mix branch from 5b3e953 to c1a38e2 Nov 4, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Nov 4, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 4, 2015

bors-servo merged commit c1a38e2 into servo:master Nov 4, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

mrobinson deleted the mrobinson:stacking-context-mix branch Nov 5, 2015

paulrouget added the A-browserhtml/P3 label Jan 26, 2016

servo / servo

Mix stacking contexts into the positioned content list #8266

Mix stacking contexts into the positioned content list #8266

mrobinson commented Oct 30, 2015

mrobinson commented Oct 30, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mrobinson commented Nov 2, 2015

paulrouget commented Nov 3, 2015

mrobinson commented Nov 3, 2015

pcwalton commented Nov 3, 2015

bors-servo commented Nov 3, 2015

pcwalton commented Nov 3, 2015

pcwalton commented Nov 3, 2015

bors-servo commented Nov 3, 2015

bors-servo commented Nov 3, 2015

bors-servo commented Nov 3, 2015

mrobinson commented Nov 4, 2015

mrobinson commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

servo / servo

Join GitHub today

Mix stacking contexts into the positioned content list #8266

Mix stacking contexts into the positioned content list #8266

Conversation

mrobinson commented Oct 30, 2015

mrobinson commented Oct 30, 2015

This comment has been minimized.

pcwalton Nov 2, 2015

This comment has been minimized.

pcwalton Nov 2, 2015

This comment has been minimized.

pcwalton Nov 2, 2015

This comment has been minimized.

mrobinson Nov 2, 2015

mrobinson commented Nov 2, 2015

paulrouget commented Nov 3, 2015

mrobinson commented Nov 3, 2015

pcwalton commented Nov 3, 2015

bors-servo commented Nov 3, 2015

pcwalton commented Nov 3, 2015

pcwalton commented Nov 3, 2015

bors-servo commented Nov 3, 2015

bors-servo commented Nov 3, 2015

bors-servo commented Nov 3, 2015

mrobinson commented Nov 4, 2015

mrobinson commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015

bors-servo commented Nov 4, 2015