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 #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...


        Auto merge of #17385 - pyfisch:better-scroll, r=<try>

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


        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.

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...


        Auto merge of #17385 - pyfisch:better-scroll, r=mrobinson

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 label Jun 17, 2017

highfive added the S-tests-failed label Jun 17, 2017

highfive assigned mrobinson and unassigned nox Jun 17, 2017

pyfisch force-pushed the pyfisch:better-scroll branch from 6ab0a71 to 1a70a02 Jun 17, 2017

highfive removed the S-tests-failed label Jun 17, 2017

pyfisch reviewed Jun 17, 2017

View changes

mrobinson reviewed Jun 19, 2017

View changes

pyfisch force-pushed the pyfisch:better-scroll branch from 1a70a02 to 5a87833 Jun 19, 2017

mrobinson approved these changes Jun 19, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 19, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 19, 2017

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

highfive added S-awaiting-review and removed S-tests-failed 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 and removed S-awaiting-review labels Jun 23, 2017

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 23, 2017

servo / servo

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

servo / servo

Join GitHub today

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

pyfisch commented Jun 17, 2017 •

edited by larsbergstrom