Add initial support for stacking contexts to layout_2020 #25763

mrobinson · 2020-02-14T08:34:18Z

These changes add initial support for stacking contexts, enabling some parts of z-index to work properly.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-02-14T08:34:23Z

Heads up! This PR modifies the following files:

@jgraham: tests/wpt/include-layout-2020.ini
@emilio: components/style/properties/longhands/position.mako.rs


        Add support for a stacking context pass to layout_2020

This adds an intermediary data structure that allows the display list builder to move through the fragment tree in stacking context painting order. Spatial nodes are built during this phase and all display list items are added to the end of the display list.


        Unskip z-order WPT tests on layout_2020

SimonSapin · 2020-02-14T17:28:26Z

components/layout_2020/display_list/stacking_context.rs

+pub(crate) enum StackingContextType {
+    Real,
+    PseudoPositioned,
+    PseudoFloat,


The spec uses the phrase “treat the element as if it created a new stacking context” a third time: for “inline-block and inline-table elements”. I think this should generalize to non-replaced atomic inline-level elements/boxes.

Nice catch! I plan to add support for this kind of pseudo stacking context in a followup change, if that's okay with you. Adding it allows more tests to pass once support is added for sorting the fragments inside a stacking context.

SimonSapin · 2020-02-14T17:28:26Z

components/layout_2020/display_list/stacking_context.rs

+                return a.z_index.cmp(&b.z_index);
+            }
+
+            match (a.context_type, b.context_type) {


Instead of this enum being part of the sort, what do you think of having separate Vecs for each categories? (I’m not saying we should do that, I haven’t thought through the advantages or downsides of each approach.)

So it's essentially a tradeoff between sorting one slightly larger Vec of stacking contexts versus sorting three or four smaller ones. I'm not sure which is better, but I've tried to more-or-less copy the design of the old layout system which went through a process of iteration and optimization.

SimonSapin · 2020-02-14T17:28:26Z

components/layout_2020/display_list/stacking_context.rs

+        if !self.style.get_box().transform.0.is_empty() {
+            return self.style.get_position().z_index.integer_or(0);
+        }


My reading of https://drafts.csswg.org/css-transforms/#transform-rendering is that this should be removed:

For elements whose layout is governed by the CSS box model, any value other than none for the transform property results in the creation of a stacking context. Implementations must paint the layer it creates, within its parent stacking context, at the same stacking order that would be used if it were a positioned element with z-index: 0. If an element with a transform is positioned, the z-index property applies as described in [CSS2], except that auto is treated as 0 since a new stacking context is always created.

This is quite odd, because Gecko has a strange behavior here that Servo has been replicating. Observe how this particular file is rendered in Gecko:

<div style="position: absolute; top: 2px; z-index: 2; background: green; width: 100px; height: 100px;"></div> <div style="background: blue; transform: rotate(45deg); z-index: 3; width: 100px; height: 100px;"></div>

In Gecko, the z-index of the transformed div is taken into account, while in Chromium and WebKit it is not.

This is arguably a Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1256980

Another bit of relevant spec is https://drafts.csswg.org/css2/visuren.html#z-index with “Applies to: positioned elements”

Oh, thanks for finding that bug. I've pushed a new version of the branch which removes this, bringing layout_2020 in line with WebKit and Blink.


        Add initial stacking context paint order support to layout_2020

This adds very rudimentary support for paint order in stacking context. In particular z-index is now handled properly, apart from issues with hoisted fragments.

SimonSapin · 2020-02-17T09:38:06Z

Looks good, thanks!

@bors-servo r+

bors-servo · 2020-02-17T09:38:08Z

📌 Commit 4a2787b has been approved by SimonSapin

bors-servo · 2020-02-17T09:38:14Z

⌛ Testing commit 4a2787b with merge 3198864...


        Auto merge of #25763 - mrobinson:stacking-contexts-v1, r=SimonSapin

Add initial support for stacking contexts to layout_2020 These changes add initial support for stacking contexts, enabling some parts of z-index to work properly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-02-17T10:25:36Z

💔 Test failed - status-taskcluster

mrobinson · 2020-02-17T11:46:26Z

+ ./mach filter-intermittents wpt-jsonsummary.log --log-intermittents intermittents.log --log-filteredsummary filtered-wpt-errorsummary.log --tracker-api default --reporter-api default
1 unexpected results that are NOT known-intermittents:
  ▶ FAIL [expected PASS] /css/CSS2/margin-padding-clear/margin-bottom-042.xht
  │   → /css/CSS2/margin-padding-clear/margin-bottom-042.xht 54a9df64f1476dd12020019d7cf22ac34d727bc0
  │   → /css/CSS2/reference/ref-no-vert-space-between.xht 361532ece0d2a0454900e9de0bb71598ea4a4295
  └   → Screenshot is solid color 0xFFFFFF for /css/CSS2/margin-padding-clear/margin-bottom-042.xht

Not sure what could be causing this failure on the non-Layout 2020 test run.

SimonSapin · 2020-02-17T12:32:38Z

Screenshot is solid color 0xFFFFFF

This is a symptom of #24726

@bors-servo retry

bors-servo · 2020-02-17T12:32:44Z

⌛ Testing commit 4a2787b with merge 13ff7ce...


        Auto merge of #25763 - mrobinson:stacking-contexts-v1, r=SimonSapin

Add initial support for stacking contexts to layout_2020 These changes add initial support for stacking contexts, enabling some parts of z-index to work properly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-02-17T12:51:57Z

☀️ Test successful - status-taskcluster
Approved by: SimonSapin
Pushing 13ff7ce to master...

highfive assigned SimonSapin Feb 14, 2020

highfive added the S-awaiting-review label Feb 14, 2020

mrobinson changed the title ~~Add initial support for stacking contexts~~ Add initial support for stacking contexts to layout_2020 Feb 14, 2020

mrobinson requested a review from SimonSapin Feb 14, 2020

mrobinson added the A-layout/2020 label Feb 14, 2020

mrobinson added 2 commits Feb 13, 2020

Unskip z-order WPT tests on layout_2020

318cc16

mrobinson force-pushed the mrobinson:stacking-contexts-v1 branch 2 times, most recently from 4906a75 to a1f3cea Feb 14, 2020

SimonSapin reviewed Feb 14, 2020

View changes

SimonSapin added this to In progress in Layout 2020 via automation Feb 14, 2020

Add initial stacking context paint order support to layout_2020

Loading status checks…

4a2787b

This adds very rudimentary support for paint order in stacking context. In particular z-index is now handled properly, apart from issues with hoisted fragments.

mrobinson force-pushed the mrobinson:stacking-contexts-v1 branch from a1f3cea to 4a2787b Feb 17, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 17, 2020

bors-servo merged commit 4a2787b into servo:master Feb 17, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

Layout 2020 automation moved this from In progress to Merged / resolved Feb 17, 2020

highfive removed the S-awaiting-merge label Feb 17, 2020

mrobinson deleted the mrobinson:stacking-contexts-v1 branch Feb 17, 2020

servo / servo

Add initial support for stacking contexts to layout_2020 #25763

Add initial support for stacking contexts to layout_2020 #25763

mrobinson commented Feb 14, 2020

highfive commented Feb 14, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020

mrobinson commented Feb 17, 2020

SimonSapin commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020

servo / servo

Join GitHub today

Add initial support for stacking contexts to layout_2020 #25763

Add initial support for stacking contexts to layout_2020 #25763

Conversation

mrobinson commented Feb 14, 2020

highfive commented Feb 14, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020

mrobinson commented Feb 17, 2020

SimonSapin commented Feb 17, 2020

bors-servo commented Feb 17, 2020

bors-servo commented Feb 17, 2020