Fix several bugs related to scrolling #17385

pyfisch · 2017-06-17T13:48:52Z

scrollLeft/scrollTop returned values of parent or even document root
Only the scroll of the node itself is returned. Otherwise 0.0.
Scrolling via script had set viewport.
This resulted in other nodes appearing scrolled.
Now scroll_offsets are updated with correct node id.

These bugs caused other odd behavior like both body and
document.documentElement being scrolled or the view for scrolled
elements jumping.

Also try scrolling this example page in servo with and without this change.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Scroll offset change for both document.body and document.documentElement #17342 (github issue number if applicable).

There are tests for these changes (partially)

This change is

highfive · 2017-06-17T13:48:56Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/element.rs, components/script/dom/window.rs
@KiChjang: components/script/dom/element.rs, components/script/dom/window.rs

pyfisch · 2017-06-17T13:56:22Z

@bors-servo: try

bors-servo · 2017-06-17T13:56:26Z

⌛ Trying commit 6ab0a71 with merge f4876f0...

Fix several bugs related to scrolling * scrollLeft/scrollTop returned values of parent or even document root Only the scroll of the node itself is returned. Otherwise 0.0. * Scrolling via script had set viewport. This resulted in other nodes appearing scrolled. Now scroll_offsets are updated with correct node id. These bugs caused other odd behavior like both body and document.documentElement being scrolled or the view for scrolled elements jumping. Also try scrolling this [example page](https://pyfisch.org/stuff/scrolltest.html) in servo with and without this change.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17342 (github issue number if applicable).  - [x] There are tests for these changes (partially)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17385)

bors-servo · 2017-06-17T14:21:14Z

💔 Test failed - android

pyfisch · 2017-06-17T14:36:51Z

The tests fail because I modified one web-platform-test to include more checks. Should web platform tests not be changed or do I need to update the manifest?

stshine · 2017-06-17T14:51:39Z

r? @mrobinson

jdm · 2017-06-17T20:50:50Z

./mach update-manifest

pyfisch · 2017-06-17T21:18:23Z

Thanks. Ran ./mach update-manifest.

pyfisch · 2017-06-17T21:52:16Z

tests/wpt/metadata/MANIFEST.json

@@ -169897,6 +169897,66 @@
     {}
    ]
   ],
+   "_certs/064162.pem": [


Should these lines really be here? To me they look more like some left-over temporary files.

mrobinson · 2017-06-19T13:48:20Z

components/script/dom/window.rs

                        ReflowReason::Query) {
            return;
        }
+
+        self.scroll_offsets.borrow_mut().insert(node.to_untrusted_node_address(),


Currently these scroll offsets come from WebRender. What is the issue here with waiting until WebRender updates the scroll offset map?

Webrender does not update the scroll offset map if the change is caused by a Msg::UpdateScrollStateFromScript (per documentation). So the offset map needs to updated in script. The reason I think is that if a script does elem.scrollTop = 42 the next line may be console.log(elem.scrollTop). If there is some message-passing involved the log statement will print wrong information. (Another case where this is important is when a website sets scrollTop and scrollLeft at the same time as done in the tests).

Hrm. The next time that WebRender sends new scroll offsets, the offsets really should reflect this change (otherwise that is a bug, I think). You are right though, that if we query the offsets immediately they won't reflect the new status. That's probably worth writing in a comment there.

I don't think it is a bug since it is described. If you scroll with mouse or keyboard the scroll state is updated by webrender. It also takes the changes from the script into account that happened before.

(Setting the offsets from webrender again in this case could be harmful. It may happen that webrender overwrites newer values.)

Changes in scroll position need to be forwarded to WebRender to proper render the scrolled content. WebRender should then return the new offsets, but they won't be available immediately. I'm not sure the documentation touches on this.

Yes this is how it is done for normal scrolling with mouse and keyboard. I think the note on the enum member is sufficient but maybe you want to document it other places too or give more details.

mrobinson · 2017-06-19T14:43:39Z

tests/wpt/metadata/MANIFEST.json

@@ -397231,6 +397291,54 @@
   "b960bef807da94c0146ed2f537eaa1e05ec9a0ab",
   "testharness"
  ],
+  "_certs/064162.pem": [


The update-manifest script isn't smart enough to ignore files that it shouldn't include, so you'll need to remove all of these extraneous lines manually. Sorry, I find this behavior really annoying too!

Thanks for the explanation. I will do so.

pyfisch · 2017-06-19T14:56:10Z

Added a comment and removed lines from manifest.

@mrobinson can you please r+?

@bors-servo try

bors-servo · 2017-06-19T14:56:14Z

⌛ Trying commit 5a87833 with merge dd1230f33443231864af970e48a7c6bf787d3c6e...

mrobinson

Thanks! This looks good.

mrobinson · 2017-06-19T15:19:16Z

@bors-servo r+

bors-servo · 2017-06-19T15:19:17Z

📌 Commit 5a87833 has been approved by mrobinson

bors-servo · 2017-06-19T15:19:21Z

⌛ Testing commit 5a87833 with merge 2ff7ce783a9c144b80be492f3d0ad5468fc26edc...

bors-servo · 2017-06-19T16:00:34Z

💔 Test failed - mac-rel-wpt2

jdm · 2017-06-19T16:47:56Z

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/003.html:
  │ FAIL [expected PASS] Fragment Navigation: Updating scroll position
  │   → assert_true: expected true got false
  │ 
  │ @http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/003.html:17:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/003.html:12:1

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:
  │ FAIL [expected PASS] Fragment Navigation: fragment id should be percent-decoded
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:17:9
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:40:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:
  │ FAIL [expected PASS] Fragment Navigation: scroll to anchor name is lower priority than equal id
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:16:9
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:34:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:
  │ FAIL [expected PASS] Fragment Navigation: TOP is a valid element id, which overrides navigating to top of the document
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:14:7
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:32:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:
  │ FAIL [expected PASS] Fragment Navigation: When fragid is TOP scroll to the top of the document
  │   → assert_not_equals: got disallowed value 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:14:7
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:41:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

* scrollLeft/scrollTop returned values of parent or even document root Only the scroll of the node itself is returned. Otherwise 0.0. * Scrolling via script had set viewport. This resulted in other nodes appearing scrolled. Now scroll_offsets are updated with correct node id. These bugs caused other odd behavior like both body and document.documentElement being scrolled or the view for scrolled elements jumping.

pyfisch · 2017-06-19T19:44:48Z

▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/003.html:
  │ FAIL [expected PASS] Fragment Navigation: Updating scroll position
  │   → assert_true: expected true got false
  │ 
  │ @http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/003.html:17:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/003.html:12:1

This is a standards mode document (<!doctype html>). It is scrolled to some fragments. Then document.body.scrollTop is requested. It should be nonzero if the document is scrolled. The spec states that this is only true in quirks mode, in standards mode document.documentElement is scrolled. (The test passes if one replaces document.body with document.documentElement). Test is wrong. (This passed before because servo scrolled both body and root element.)

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:
  │ FAIL [expected PASS] Fragment Navigation: fragment id should be percent-decoded
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:17:9
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html:40:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:
  │ FAIL [expected PASS] Fragment Navigation: scroll to anchor name is lower priority than equal id
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:16:9
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html:34:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:
  │ FAIL [expected PASS] Fragment Navigation: TOP is a valid element id, which overrides navigating to top of the document
  │   → assert_equals: expected 200 but got 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:14:7
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html:32:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:
  │ FAIL [expected PASS] Fragment Navigation: When fragid is TOP scroll to the top of the document
  │   → assert_not_equals: got disallowed value 0
  │ 
  │ steps<.handler@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:14:7
  │ runNextStep/listener<@http://web-platform.test:8000/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html:41:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

These failures are real bugs caused by me removing the line self.update_viewport_for_scroll(x, y);. I am still not sure how perform_a_scroll works on elements, maybe someone can explain it? But it is necessary to do the scrolls.

I have updated the pull request to disable the first test and added update_viewport_for_scroll at two places.

jdm · 2017-06-19T20:04:57Z

Let's file an issue upstream about 003.html in that case.

pyfisch · 2017-06-20T12:28:18Z

Filed an issue at web-platform-tests/wpt#6289.

pyfisch · 2017-06-22T15:15:34Z

Is there anything still blocking this PR? The broken test is broken in Firefox too so it no servo bug. In Chrome it will work because of a known and soon fixed incompatibility.

mrobinson · 2017-06-23T08:01:36Z

@bors-servo r+

bors-servo · 2017-06-23T08:01:37Z

📌 Commit 284cb8a has been approved by mrobinson

bors-servo · 2017-06-23T08:01:37Z

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

bors-servo · 2017-06-23T08:50:42Z

⌛ Testing commit 284cb8a with merge 626c029...

Fix several bugs related to scrolling * scrollLeft/scrollTop returned values of parent or even document root Only the scroll of the node itself is returned. Otherwise 0.0. * Scrolling via script had set viewport. This resulted in other nodes appearing scrolled. Now scroll_offsets are updated with correct node id. These bugs caused other odd behavior like both body and document.documentElement being scrolled or the view for scrolled elements jumping. Also try scrolling this [example page](https://pyfisch.org/stuff/scrolltest.html) in servo with and without this change.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17342 (github issue number if applicable).  - [x] There are tests for these changes (partially)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17385)

bors-servo · 2017-06-23T09:45:14Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mrobinson
Pushing 626c029 to master...

highfive assigned nox Jun 17, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 17, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 17, 2017

highfive assigned mrobinson and unassigned nox Jun 17, 2017

pyfisch force-pushed the better-scroll branch from 6ab0a71 to 1a70a02 Compare June 17, 2017 21:18

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 17, 2017

pyfisch commented Jun 17, 2017

View reviewed changes

mrobinson reviewed Jun 19, 2017

View reviewed changes

pyfisch force-pushed the better-scroll branch from 1a70a02 to 5a87833 Compare June 19, 2017 14:55

mrobinson approved these changes Jun 19, 2017

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 19, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 19, 2017

pyfisch force-pushed the better-scroll branch from 5a87833 to 284cb8a Compare June 19, 2017 19:43

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 19, 2017

pyfisch mentioned this pull request Jun 20, 2017

/browsing-the-web/scroll-to-fragid/003.html test wrong web-platform-tests/wpt#6289

Closed

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 23, 2017

bors-servo merged commit 284cb8a into servo:master Jun 23, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several bugs related to scrolling #17385

Fix several bugs related to scrolling #17385

pyfisch commented Jun 17, 2017 •

edited by larsbergstrom

highfive commented Jun 17, 2017

pyfisch commented Jun 17, 2017

bors-servo commented Jun 17, 2017

bors-servo commented Jun 17, 2017

pyfisch commented Jun 17, 2017

stshine commented Jun 17, 2017

jdm commented Jun 17, 2017

pyfisch commented Jun 17, 2017

pyfisch Jun 17, 2017

mrobinson Jun 19, 2017

pyfisch Jun 19, 2017

mrobinson Jun 19, 2017

pyfisch Jun 19, 2017

mrobinson Jun 19, 2017

pyfisch Jun 19, 2017

mrobinson Jun 19, 2017

pyfisch Jun 19, 2017

pyfisch commented Jun 19, 2017

bors-servo commented Jun 19, 2017

mrobinson left a comment

mrobinson commented Jun 19, 2017

bors-servo commented Jun 19, 2017

bors-servo commented Jun 19, 2017

bors-servo commented Jun 19, 2017

jdm commented Jun 19, 2017

pyfisch commented Jun 19, 2017

jdm commented Jun 19, 2017

pyfisch commented Jun 20, 2017

pyfisch commented Jun 22, 2017

mrobinson commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

Fix several bugs related to scrolling #17385

Fix several bugs related to scrolling #17385

Conversation

pyfisch commented Jun 17, 2017 • edited by larsbergstrom

highfive commented Jun 17, 2017

pyfisch commented Jun 17, 2017

bors-servo commented Jun 17, 2017

bors-servo commented Jun 17, 2017

pyfisch commented Jun 17, 2017

stshine commented Jun 17, 2017

jdm commented Jun 17, 2017

pyfisch commented Jun 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyfisch commented Jun 19, 2017

bors-servo commented Jun 19, 2017

mrobinson left a comment

Choose a reason for hiding this comment

mrobinson commented Jun 19, 2017

bors-servo commented Jun 19, 2017

bors-servo commented Jun 19, 2017

bors-servo commented Jun 19, 2017

jdm commented Jun 19, 2017

pyfisch commented Jun 19, 2017

jdm commented Jun 19, 2017

pyfisch commented Jun 20, 2017

pyfisch commented Jun 22, 2017

mrobinson commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

pyfisch commented Jun 17, 2017 •

edited by larsbergstrom