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

Don't promote all scrollable regions to stacking contexts #14084

Merged
merged 2 commits into from Nov 8, 2016
Merged

Don't promote all scrollable regions to stacking contexts #14084

merged 2 commits into from Nov 8, 2016

Conversation

@mrobinson
Copy link
Member

mrobinson commented Nov 5, 2016 •
edited by larsbergstrom

Don't promote all scrollable regions to stacking contexts

Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated. When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13529 and #13298. (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@mrobinson
Don't promote all scrollable regions to stacking contexts
ef82d77 
Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated.  When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

Fixes #13529.
Fixed #13298.
@highfive
Copy link

highfive commented Nov 5, 2016

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list_builder.rs, components/layout/fragment.rs, components/layout/traversal.rs, components/layout/block.rs, components/layout/webrender_helpers.rs
@mrobinson
Copy link
Member Author

mrobinson commented Nov 5, 2016

@pcwalton or @glennw r?
@glennw
Copy link
Member

glennw commented Nov 5, 2016

@mrobinson Did any shaders need to be copied across with the WR update?
@wafflespeanut wafflespeanut assigned pcwalton and glennw and unassigned wafflespeanut Nov 6, 2016
@mrobinson
Copy link
Member Author

mrobinson commented Nov 7, 2016

@glennw Actually, yes! Some shaders were updated. I was copying them into the wrong checkout of Servo. I'll update the branch.
@mrobinson
Update WebRender to get necessary changes
Loading status checks…
dea0c7c
@mrobinson mrobinson force-pushed the mrobinson:scroll_root_2 branch from 1bb487e to dea0c7c Nov 7, 2016
@glennw
Copy link
Member

glennw commented Nov 7, 2016

@mrobinson OK, once the shaders are copied across let's issue a try run and I'll review in the morning. Thanks!
@glennw
Copy link
Member

glennw commented Nov 7, 2016

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

bors-servo commented Nov 7, 2016

⌛ Trying commit dea0c7c with merge 67ec102...
bors-servo added a commit that referenced this pull request Nov 7, 2016
@bors-servo
Auto merge of #14084 - mrobinson:scroll_root_2, r=<try>
Loading status checks…
67ec102 
Don't promote all scrollable regions to stacking contexts

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

Don't promote all scrollable regions to stacking contexts

Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated.  When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

---
<!-- 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
- [x] These changes fix #13529 and #13298. (github issue number if applicable).

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

<!-- 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/14084)
<!-- Reviewable:end -->
@mrobinson
Copy link
Member Author

mrobinson commented Nov 7, 2016

@glennw Thanks!
@bors-servo bors-servo mentioned this pull request Nov 7, 2016
Closed
@emilio
Copy link
Member

emilio commented Nov 7, 2016 •
edited

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

bors-servo commented Nov 7, 2016

⌛ Trying commit dea0c7c with merge 5145a40...
bors-servo added a commit that referenced this pull request Nov 7, 2016
@bors-servo
Auto merge of #14084 - mrobinson:scroll_root_2, r=<try>
Loading status checks…
5145a40 
Don't promote all scrollable regions to stacking contexts

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

Don't promote all scrollable regions to stacking contexts

Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated.  When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

---
<!-- 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
- [x] These changes fix #13529 and #13298. (github issue number if applicable).

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

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

bors-servo commented Nov 7, 2016

💔 Test failed - mac-rel-wpt2
@jdm
Copy link
Member

jdm commented Nov 7, 2016

Filed #14118 for the previous failure.
@glennw
Copy link
Member

glennw commented Nov 7, 2016

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

bors-servo commented Nov 7, 2016

⌛ Trying commit dea0c7c with merge e18a64e...
bors-servo added a commit that referenced this pull request Nov 7, 2016
@bors-servo
Auto merge of #14084 - mrobinson:scroll_root_2, r=<try>
Loading status checks…
e18a64e 
Don't promote all scrollable regions to stacking contexts

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

Don't promote all scrollable regions to stacking contexts

Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated.  When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

---
<!-- 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
- [x] These changes fix #13529 and #13298. (github issue number if applicable).

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

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

bors-servo commented Nov 7, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@glennw
Copy link
Member

glennw commented Nov 7, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2016

📌 Commit dea0c7c has been approved by glennw
@glennw
Copy link
Member

glennw commented Nov 7, 2016

@mrobinson Awesome! The code looks good. We don't have great test coverage for scroll issues, but I tested manually on a number of sites, including browser.html and all seemed good from what I could see.
@glennw
Copy link
Member

glennw commented Nov 7, 2016

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

bors-servo commented Nov 7, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #12862
@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2016

📌 Commit dea0c7c has been approved by glennw
@glennw
Copy link
Member

glennw commented Nov 7, 2016

@bors-servo retry
bors-servo added a commit that referenced this pull request Nov 8, 2016
@bors-servo
Auto merge of #14084 - mrobinson:scroll_root_2, r=glennw
Loading status checks…
ef5ca14 
Don't promote all scrollable regions to stacking contexts

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

Don't promote all scrollable regions to stacking contexts

Instead annotate all flows with their owning ScrollRoots. When
processing the display list items into a flattened display list, we add
PushScrollRoot and PopScrollRoot to signal when scrolling regions start
and end. It is possible for content from different scrolling regions to
intersect and when they do, the stack of scrolling regions is
duplicated.  When these duplicated scrolling regions stacks reach
WebRender, it will scroll them in tandem.

The PushScrollRoot and PopScrollRoot items are currently represented as
StackingContexts in WebRender, but eventually these will be replaced
with special WebRender display items.

---
<!-- 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
- [x] These changes fix #13529 and #13298. (github issue number if applicable).

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

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

bors-servo commented Nov 8, 2016

⌛ Testing commit dea0c7c with merge ef5ca14...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 8, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit dea0c7c into servo:master Nov 8, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:scroll_root_2 branch Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@glennw glennw

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Should be able to interleave scroll frame contents with unscrolled elements from outside the scroll frame
8 participants
@mrobinson @highfive @glennw @bors-servo @emilio @jdm @pcwalton @wafflespeanut
You can’t perform that action at this time.