Refactor scrolling on the window object #29680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @atbrakhi |
mrobinson
changed the title
window object from script
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Apr 28, 2023
Fix scrolls run on the `window` object from script The calculation of the viewport scrolling area was incorrect, because it was based on the body element instead of the viewport. This change makes the argument to the layout query optional and when it is not specified, the return value reflects the viewport's scrolling area. In addition, scrolling the window object early on in the page load could sometimes lead to a situation where WebRender would scroll, but not request a frame with the scrolled content. Always request a frame when updating scroll positions from script. This patch was written in collaboration with Rakhi Sharma <atbrakhi@igalia.com>. <!-- 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 - [x] These changes fix #21321. - [x] There are tests for these changes <!-- 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. -->
|
Test results for linux-wpt-layout-2013 from try job (#4830008121): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (17)
Stable unexpected results (22)
|
|
💔 Test failed - checks-github |
mrobinson
changed the title
window object from scriptwindow object from script
|
This needs a bit more work. In particular, we need to properly handle overflow: scroll on the |
|
☔ The latest upstream changes (presumably #29699) made this pull request unmergeable. Please resolve the merge conflicts. |
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
2 times, most recently
from
f72285c
to
07b09ff
Compare
|
@bors-servo try |
|
🔨 Triggering try run (#5787185708) with platform=all and layout=all |
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
07b09ff
to
7659356
Compare
|
@bors-servo try |
|
🔨 Triggering try run (#5787302817) with platform=all and layout=all |
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
7659356
to
fa379c7
Compare
|
@bors-servo try |
|
🔨 Triggering try run (#5787603936) with platform=all and layout=all |
|
|
|
🔨 Triggering try run (#5787603936) with platform=all and layout=all |
|
Test results for linux-wpt-layout-2020 from try job (#5787603936): Flaky unexpected result (1)
Stable unexpected results that are known to be intermittent (2)Stable unexpected results (52)
|
|
Test results for linux-wpt-layout-2013 from try job (#5787603936): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (20)
|
|
|
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
fa379c7
to
ff05543
Compare
|
@bors-servo try |
|
🔨 Triggering try run (#5806668478) with platform=all and layout=all |
|
Test results for linux-wpt-layout-2020 from try job (#5806668478): Flaky unexpected result (2)
Stable unexpected results (1)
|
|
Test results for linux-wpt-layout-2013 from try job (#5806668478): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (12)
|
|
|
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
ff05543
to
822a618
Compare
mrobinson
changed the title
window object from script
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
822a618
to
470f5de
Compare
|
@bors-servo try |
|
🔨 Triggering try run (#5824060244) with platform=all and layout=all |
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
470f5de
to
ef0d95d
Compare
|
Test results for linux-wpt-layout-2020 from try job (#5824060244): Flaky unexpected result (1)
Stable unexpected results that are known to be intermittent (1)
Stable unexpected results (3)
|
|
Test results for linux-wpt-layout-2013 from try job (#5824060244): Flaky unexpected result (15)
Stable unexpected results that are known to be intermittent (15)
|
|
|
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
ef0d95d
to
fc1988f
Compare
|
@bors-servo try=wpt-2020 |
|
🔨 Triggering try run (#5830627435) with platform=linux and layout=2020 |
|
Test results for linux-wpt-layout-2020 from try job (#5830627435): |
|
✨ Try run (#5830627435) succeeded. |
mrobinson
changed the title
mukilan
reviewed
Sep 15, 2023
| pub fn scrolling_area(&self, containing_block: &PhysicalRect<Length>) -> PhysicalRect<Length> { | ||
| match self { | ||
| Fragment::Box(fragment) | Fragment::Float(fragment) => fragment | ||
| .scrollable_overflow(&containing_block) |
There was a problem hiding this comment.
Do we need to re-borrow containing_block here? fn scrollable_overflow takes a &PhysicalRect, so I think we should be able to pass containing_block directly?
There was a problem hiding this comment.
Nope. You're right. I've removed this.
Refactor the scrolling and scrollable area calculation on the window object, to make it better match the specification. This has some mild changes to behavior, but in general things work the same as they did before. This is mainly preparation for properly handling viewport propagation of the `overflow` property but seems to fix a few issues as well. There is one new failure in Layout 2020 regarding `position: sticky`, but this isn't a big deal because there is no support for `position: sticky` in Layout 2020 yet. Co-authored-by: Rakhi Sharma <atbrakhi@igalia.com>
mrobinson
force-pushed
the
fix-root-scrolling-area
branch
from
fc1988f
to
1192895
Compare
|
Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the scrolling and scrollable area calculation on the window object, to make it better match the specification. This has some mild changes to behavior, but in general things work the same as they did before. This is mainly preparation for properly handling viewport propagation of the
overflowproperty.There is one new pass in Layout 2020 regarding
position: sticky, but this isn't a big deal because there is no support forposition: stickyin Layout 2020 yet. In addition,tests/wpt/mozilla/tests/mozilla/scrollTo.htmlis updated to fix a flakiness timing issue. It tries to scroll the page before the entire page is loaded.Co-authored-by: Rakhi Sharma atbrakhi@igalia.com
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors