Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix duplicate stacking context creation for anonymous Flows #18902

Closed
wants to merge 1 commit into from
Closed

Fix duplicate stacking context creation for anonymous Flows #18902

wants to merge 1 commit into from

Conversation

@mrobinson
Copy link
Member

mrobinson commented Oct 16, 2017 •
edited

Anonymous nodes were previously creating duplicate stacking contexts,
one for each node in the anonymous node chain. This change eliminates
that for tables.

Additionally the use of stacking context ids based on node addresses is
no longer necessary since stacking contexts no longer control scrolling.
This is the first step in eliminating the dependency between node
addresses and ClipScrollNodes which causes issues like #16425.

  • ./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 they do not change behavior.

This change is Reviewable
@mrobinson
Use incremental stacking context ids
Loading status checks…
47e69c8 
Using stacking context ids based on node addresses is no longer
necessary since stacking contexts no longer control scrolling. This is
the first step in eliminating the dependency between node addresses and
ClipScrollNodes which causes issues like #16425.
@highfive
Copy link

highfive commented Oct 16, 2017

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list_builder.rs
@highfive
Copy link

highfive commented Oct 16, 2017

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@mrobinson
Copy link
Member Author

mrobinson commented Oct 16, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2017

⌛ Trying commit 47e69c8 with merge acb269b...
bors-servo added a commit that referenced this pull request Oct 16, 2017
@bors-servo
Auto merge of #18902 - mrobinson:ids, r=<try>
Loading status checks…
acb269b 
Use incremental stacking context ids

Using stacking context ids based on node addresses is no longer
necessary since stacking contexts no longer control scrolling. This is
the first step in eliminating the dependency between node addresses and
ClipScrollNodes which causes issues like #16425.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they do not change behavior.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@jdm
Copy link
Member

jdm commented Oct 16, 2017

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2017

⌛ Trying commit 47e69c8 with merge b8b7062...
bors-servo added a commit that referenced this pull request Oct 16, 2017
@bors-servo
Auto merge of #18902 - mrobinson:ids, r=<try>
Loading status checks…
b8b7062 
Use incremental stacking context ids

Using stacking context ids based on node addresses is no longer
necessary since stacking contexts no longer control scrolling. This is
the first step in eliminating the dependency between node addresses and
ClipScrollNodes which causes issues like #16425.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they do not change behavior.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18902)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2017

💔 Test failed - mac-rel-css2
@samlh
Copy link
Contributor

samlh commented Oct 17, 2017 

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-006.htm
  └   → /css-transforms-1_dev/html/transform-abspos-006.htm f96baa41b1ec45393a64ee8efd3edc1d33d77626
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm aacecec50bd95990c8b989b8f356d1b80cc0c85f
Testing f96baa41b1ec45393a64ee8efd3edc1d33d77626 == aacecec50bd95990c8b989b8f356d1b80cc0c85f

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-display-002.htm
  └   → /css-transforms-1_dev/html/transform-display-002.htm 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab
/css-transforms-1_dev/html/reference/transform-display-ref.htm 688a5b1914ffb5edd1352cdfeed6775a3a8cc5f0
Testing 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab == 688a5b1914ffb5edd1352cdfeed6775a3a8cc5f0
/css-transforms-1_dev/html/transform-display-002.htm 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab
/css-transforms-1_dev/html/reference/transform-display-notref.htm 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab
Testing 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab != 9c58df42dbfa7109d2b3d51b0aec0c12f11454ab

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform3d-perspective-008.htm
  └   → /css-transforms-1_dev/html/transform3d-perspective-008.htm cbc1defc8b9b931c6690a3aaddd1bb8cc3f6436a
/css-transforms-1_dev/html/reference/transform-lime-square-ref.htm ea0d0b830335b2af66807ea298a69e4a2e874d08
Testing cbc1defc8b9b931c6690a3aaddd1bb8cc3f6436a == ea0d0b830335b2af66807ea298a69e4a2e874d08
@jdm
Copy link
Member

jdm commented Oct 17, 2017

And on Linux, the transform-display-002.htm failure is missing:

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform3d-perspective-008.htm
  └   → /css-transforms-1_dev/html/transform3d-perspective-008.htm cbc1defc8b9b931c6690a3aaddd1bb8cc3f6436a
/css-transforms-1_dev/html/reference/transform-lime-square-ref.htm ea0d0b830335b2af66807ea298a69e4a2e874d08
Testing cbc1defc8b9b931c6690a3aaddd1bb8cc3f6436a == ea0d0b830335b2af66807ea298a69e4a2e874d08

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-006.htm
  └   → /css-transforms-1_dev/html/transform-abspos-006.htm 74fbd8c49c2ad4240dafda36b594c1a8c104b839
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm baaafb7b31f31cd2dacd1d2270ac7083883a4e32
Testing 74fbd8c49c2ad4240dafda36b594c1a8c104b839 == baaafb7b31f31cd2dacd1d2270ac7083883a4e32
@mrobinson
Copy link
Member Author

mrobinson commented Oct 17, 2017

It seems this change has uncovered a lot of places where we were creating conflicting stacking contexts with the same id. I'm cleaning this up now and hope to have a new version today.
@mrobinson mrobinson changed the title Use incremental stacking context ids Fix duplicate stacking context creation for anonymous Flows Oct 17, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Oct 17, 2017 •
edited

This is the updated version of the PR. The changes uncover some failures in our missing display: inline-table support, so those tests are now marked as failing. I'm also seeing a few more passes, which I'm going to verify via bors here.
@mrobinson
Copy link
Member Author

mrobinson commented Oct 17, 2017

Going to close this PR in favor of a new one since a branch rename seems to have confused GitHub.
@mrobinson mrobinson closed this Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@emilio emilio

Labels
S-awaiting-review S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@mrobinson @highfive @bors-servo @jdm @samlh @emilio
You can’t perform that action at this time.