Skip to content
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

Allow overflow:scroll without a stacking context #18188

Merged
merged 1 commit into from Aug 24, 2017

Conversation

@mrobinson
Copy link
Member

mrobinson commented Aug 22, 2017

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.


  • ./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 _____

This change is Reviewable

@highfive
Copy link

highfive commented Aug 22, 2017

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list_builder.rs, components/layout/fragment.rs
@jdm
Copy link
Member

jdm commented Aug 22, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

Trying commit 5610564 with merge 72a8825...

bors-servo added a commit that referenced this pull request Aug 22, 2017
[DO NOT MERGE] Allow overflow:scroll without a stacking context

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.

<!-- 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: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/18188)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

💔 Test failed - android

@mrobinson mrobinson force-pushed the mrobinson:overflow-stacking-context branch from 5610564 to b3d03e0 Aug 22, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Aug 22, 2017

Oof. Forgot to commit my manifest changes and made a bors-servo try/test thinko. Well, hopefully this time goes better.

@mrobinson
Copy link
Member Author

mrobinson commented Aug 22, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

Trying commit b3d03e0 with merge 9f27d06...

bors-servo added a commit that referenced this pull request Aug 22, 2017
[DO NOT MERGE] Allow overflow:scroll without a stacking context

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.

<!-- 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: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/18188)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

💔 Test failed - mac-rel-wpt3

@jdm
Copy link
Member

jdm commented Aug 22, 2017

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

Trying commit b3d03e0 with merge c9dbb0d...

bors-servo added a commit that referenced this pull request Aug 22, 2017
[DO NOT MERGE] Allow overflow:scroll without a stacking context

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.

<!-- 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: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/18188)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2017

@mrobinson mrobinson changed the title [DO NOT MERGE] Allow overflow:scroll without a stacking context Allow overflow:scroll without a stacking context Aug 22, 2017
@mrobinson mrobinson requested review from mbrubeck, pcwalton and glennw Aug 22, 2017
@glennw
glennw approved these changes Aug 22, 2017
Copy link
Member

glennw left a comment

Looks good - it seems surprising there's nothing in CSS/WPT reftests that test this though?

(position::T::fixed, _, _, _) |
(position::T::relative, _, _, _) => true,
(position::T::static_, _, _, _) => false
// Statically positioned fragments don't establish stacking contexts if the previous

This comment has been minimized.

@glennw

glennw Aug 22, 2017

Member

Could we add a spec link / reference here?

This comment has been minimized.

@mrobinson

mrobinson Aug 23, 2017

Author Member

Sure. I've updated the branch.

Copy link
Contributor

mbrubeck left a comment

Look good to me.

@mrobinson mrobinson force-pushed the mrobinson:overflow-stacking-context branch from b3d03e0 to 6ea46e4 Aug 23, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Aug 23, 2017

@glennw Yeah. I was also surprised that this didn't affect the CSS or WPT tests. I guess they are focused more on positioning than stacking order. This is a bit of an edge case as well.

@mrobinson
Copy link
Member Author

mrobinson commented Aug 23, 2017

@bors-servo r=mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

💔 Test failed - linux-rel-css

@mrobinson
Copy link
Member Author

mrobinson commented Aug 23, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

Testing commit 6ea46e4 with merge d552336...

bors-servo added a commit that referenced this pull request Aug 23, 2017
Allow overflow:scroll without a stacking context

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.

<!-- 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: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/18188)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

💔 Test failed - linux-rel-wpt

@mrobinson
Copy link
Member Author

mrobinson commented Aug 23, 2017

The latest failure seems to be a subtest failure in /_mozilla/css/animations/mixed-units.html. I cannot find a intermittent bug for this, but this doesn't seem likely to be related to my patch.

@mrobinson
Copy link
Member Author

mrobinson commented Aug 23, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

The latest upstream changes (presumably #18179) made this pull request unmergeable. Please resolve the merge conflicts.

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.
@mrobinson mrobinson force-pushed the mrobinson:overflow-stacking-context branch from 6ea46e4 to f1b9839 Aug 24, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Aug 24, 2017

@bors-servo r=mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2017

📌 Commit f1b9839 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2017

Testing commit f1b9839 with merge 4725a05...

bors-servo added a commit that referenced this pull request Aug 24, 2017
Allow overflow:scroll without a stacking context

Fix the long-standing bug where items that are positioned and have
overflow:scroll or overflow:auto automatically create stacking
contexts. In order to do this we need to fix another bug where display
list sorting can put a Clip or ScrollFrame definition after the first
time it is used in a display list.

<!-- 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: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/18188)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2017

@bors-servo bors-servo merged commit f1b9839 into servo:master Aug 24, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:overflow-stacking-context branch Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.