script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. #11680

pcwalton · 2016-06-09T01:46:08Z

Closes #11648.

r? @jdm
cc @paulrouget

This change is

highfive · 2016-06-09T01:46:11Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/node.rs, components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-06-09T01:46:12Z

Warning

These commits modify gfx, layout, and script code, but no tests are modified. Please consider adding a test!

highfive · 2016-06-09T01:46:41Z

New code was committed to pull request.

jdm · 2016-06-09T17:28:10Z

This generally looks fine, apart from the suspected breakage of synchronous scrolling APIs. I would also like @glennw to take a look at the gfx_traits changes, since I don't know enough about what's going on there to feel comfortable reviewing it.
-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 10 of 10 files at r1.
Review status: 10 of 11 files reviewed at latest revision, 4 unresolved discussions.

components/gfx/display_list/mod.rs, line 632 [r1] (raw file):

        //
        // We don't perform this adjustment on the root stacking context because the DOM-side code
        // has already translated the point for us (e.g. in Document::handle_mouse_move_event()`)

nit: stray ` here.

components/layout/layout_thread.rs, line 1302 [r1] (raw file):

        }
        let _ = self.script_chan
                    .send(ConstellationControlMsg::SetScrollState(self.id, script_scroll_states));

Since this message is sent to the regular script event queue, I expect that the following code will print false now since we rely on the latest data that's been pushed to the script thread rather than querying the compositor/layout:

var oldScrollTop = element.scrollTop;
element.scrollTop = oldScrollTop + 10;
console.log(oldScrollTop != element.scrollTop)

components/script/dom/window.rs, line 269 [r1] (raw file):

    /// A list of scroll offsets for each scrollable element.
    #[ignore_heap_size_of = "TODO(#6909) need to measure HashMap"]

HeapSize supports HashMap, so this message is incorrect. What's the actual type that's missing an implementation?

components/script/dom/window.rs, line 373 [r1] (raw file):

    ///
    /// This is only called when WebRender is in use.
    pub fn add_scroll_offset(&self,

Let's combine these two methods into a single set_scroll_offsets(&self, offsets: HashMap<UntrustedNodeAddress, Point2D<f32>>) instead.

Comments from Reviewable

pcwalton · 2016-06-10T01:02:31Z

Review status: 10 of 11 files reviewed at latest revision, 4 unresolved discussions.

components/layout/layout_thread.rs, line 1302 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

Since this message is sent to the regular script event queue, I expect that the following code will print false now since we rely on the latest data that's been pushed to the script thread rather than querying the compositor/layout:
var oldScrollTop = element.scrollTop;
element.scrollTop = oldScrollTop + 10;
console.log(oldScrollTop != element.scrollTop)

This is basically a symptom of the fact that script-initiated scrolling doesn't work at all in WebRender yet. All of our DOM-initiated scrolls are based on layers, which are not a concept that exists in the WebRender world.

How would you like me to fix this? Do you want me to implement script-initiated scrolling in WebRender before landing this patch? (It'll probably be a large change, larger than this patch itself.)

Comments from Reviewable

highfive · 2016-06-10T01:10:13Z

New code was committed to pull request.

highfive · 2016-06-10T01:16:39Z

New code was committed to pull request.

pcwalton · 2016-06-10T01:16:45Z

r? @glennw for the gfx_traits bits

glennw · 2016-06-10T01:34:21Z

Review status: 6 of 12 files reviewed at latest revision, 7 unresolved discussions.

components/gfx_traits/lib.rs, line 182 [r2] (raw file):

    /// Returns a new stacking context ID for a special stacking context.
    fn special_id() -> usize {

Perhaps call it next_special_id() ?

components/gfx_traits/lib.rs, line 183 [r2] (raw file):

    /// Returns a new stacking context ID for a special stacking context.
    fn special_id() -> usize {
        ((NEXT_SPECIAL_STACKING_CONTEXT_ID.fetch_add(1, Ordering::SeqCst) + 1) << 2) &

Perhaps add a comment here explaining the left shift. Is it likely in the future that the fragment type enum might be expanded? Perhaps reference this code in a comment from the fragment type enum?

ports/glutin/window.rs, line 84 [r2] (raw file):

    }
    builder.with_app_name(String::from("Servo"))
           .with_transparent_corner_radius(8)

This seems unrelated to this PR?

Comments from Reviewable

jdm · 2016-06-10T08:39:07Z

Review status: 6 of 12 files reviewed at latest revision, 7 unresolved discussions.

components/layout/layout_thread.rs, line 1302 [r1] (raw file):

Previously, pcwalton (Patrick Walton) wrote…

This is basically a symptom of the fact that script-initiated scrolling doesn't work at all in WebRender yet. All of our DOM-initiated scrolls are based on layers, which are not a concept that exists in the WebRender world.

How would you like me to fix this? Do you want me to implement script-initiated scrolling in WebRender before landing this patch? (It'll probably be a large change, larger than this patch itself.)

I didn't realize that webrender _completely_ broke script-initiated scrolling; that's fun! I propose we add a `script_can_initiate_scroll` function to `script/lib.rs` that checks if webrender is in use, then add `[Func="::script_can_initiate_scroll"]` annotations to the scrolling methods in Window.webidl and Element.webidl. This will help make it clear whether the methods are usable or not given the configuration that's being used for Servo.

Comments from Reviewable

jdm · 2016-06-10T08:40:45Z

-S-awaiting-review +S-needs-code-changes

Previously, pcwalton (Patrick Walton) wrote…

r? @glennw for the gfx_traits bits

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

highfive · 2016-06-10T22:38:23Z

New code was committed to pull request.

pcwalton · 2016-06-10T22:40:50Z

Review status: 6 of 14 files reviewed at latest revision, 4 unresolved discussions.

components/gfx_traits/lib.rs, line 182 [r2] (raw file):

Previously, glennw (Glenn Watson) wrote…

Perhaps call it next_special_id() ?

Done.

components/gfx_traits/lib.rs, line 183 [r2] (raw file):

Previously, glennw (Glenn Watson) wrote…

Perhaps add a comment here explaining the left shift. Is it likely in the future that the fragment type enum might be expanded? Perhaps reference this code in a comment from the fragment type enum?

Done.

components/layout/layout_thread.rs, line 1302 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

I didn't realize that webrender completely broke script-initiated scrolling; that's fun! I propose we add a script_can_initiate_scroll function to script/lib.rs that checks if webrender is in use, then add [Func="::script_can_initiate_scroll"] annotations to the scrolling methods in Window.webidl and Element.webidl. This will help make it clear whether the methods are usable or not given the configuration that's being used for Servo.

Done.

Comments from Reviewable

pcwalton · 2016-06-10T22:41:52Z

Review status: 6 of 14 files reviewed at latest revision, 4 unresolved discussions.

ports/glutin/window.rs, line 84 [r2] (raw file):

Previously, glennw (Glenn Watson) wrote…

This seems unrelated to this PR?

Removed.

Comments from Reviewable

jdm · 2016-06-10T22:48:24Z

r=me with the remaining attributes annotated.
-S-awaiting-review +S-needs-code-changes

Previously, pcwalton (Patrick Walton) wrote…

r? @jdm @glennw

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/webidls/Element.webidl, line 100 [r3] (raw file):

  [Func="::script_can_initiate_scroll"]
  void scrollBy(unrestricted double x, unrestricted double y);
  attribute unrestricted double scrollTop;

scrollTop and scrollLeft need it as well.

Comments from Reviewable

glennw · 2016-06-10T22:51:09Z

@pcwalton The gfx traits related changes look good.

highfive · 2016-06-11T01:40:19Z

New code was committed to pull request.

pcwalton · 2016-06-11T01:40:26Z

@bors-servo: r=jdm

bors-servo · 2016-06-11T01:40:27Z

📌 Commit 041cfe6 has been approved by jdm


        script: When using WebRender, keep the DOM-side scroll positions for

elements with `overflow: scroll` up to date, and take them into account when doing hit testing. Closes #11648.

bors-servo · 2016-06-11T02:32:10Z

⌛ Testing commit 041cfe6 with merge ffcf72d...


        Auto merge of #11680 - pcwalton:wr-overflow-scroll-hit-testing, r=jdm

@jdm

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. Closes #11648. r? @jdm cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11680)

bors-servo · 2016-06-11T03:19:27Z

💔 Test failed - linux-rel

highfive · 2016-06-11T03:19:29Z

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/change_parentage.html
  │ 
  │ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.
  │ ERROR:constellation::constellation: Panic: called `Option::unwrap()` on a `None` value
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00007faaed53c73d - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00007faaed53c6c5 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00007faaec58d709 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h89adae1a802be550
  │ frame #3  - 0x00007faaed52df78 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │ frame #4  - 0x00007faaed7f889c - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00007faaed812ca1 - std::panicking::begin_panic::he426e15a3766089a
  │ frame #6  - 0x00007faaed7fa10a - std::panicking::begin_panic_fmt::hdddb415186c241e7
  │ frame #7  - 0x00007faaed812c3e - rust_begin_unwind
  │ frame #8  - 0x00007faaed84923f - core::panicking::panic_fmt::hf4e16cb7f0d41a25
  │ frame #9  - 0x00007faaed849518 - core::panicking::panic::h907815f47e914305
  │ frame #10 - 0x00007faaec3ae5d1 - script::dom::event::Event::new_uninitialized::h05ae188a870e92b9
  │ frame #11 - 0x00007faaec3b648b - script::dom::event::Event::new::h7a12c706eaf845bc
  │ frame #12 - 0x00007faaec3fd31c - script::dom::eventtarget::EventTarget::fire_event::h0d5b30039f5820d2
  │ frame #13 - 0x00007faaec6020fa - script::task_source::dom_manipulation::DOMManipulationTask::handle_task::h0ea31f97e475cb7a
  │ frame #14 - 0x00007faaec597c05 - script::script_thread::ScriptThread::handle_msg_from_script::h47e979ae1e7fb676
  │ frame #15 - 0x00007faaec5e654f - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::hb50cf7fd97943b65
  │ frame #16 - 0x00007faaec5cdaf7 - script::script_thread::ScriptThread::handle_msgs::h1e1abab71191c950
  │ frame #17 - 0x00007faaec58bd87 - std::panicking::try::call::h78dcd5319adf08fa
  │ frame #18 - 0x00007faaed81ce9b - __rust_try
  │ frame #19 - 0x00007faaed81ce3e - __rust_maybe_catch_panic
  │ frame #20 - 0x00007faaec58d06d - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hce043844655ea6a9
  │ frame #21 - 0x00007faaed810e64 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #22 - 0x00007faae9675183 - start_thread
  │ frame #23 - 0x00007faae918c37c - clone
  │ frame #24 - 0x0000000000000000 - &lt;unknown&gt;
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

cbrewster · 2016-06-11T03:33:31Z

@bors-servo retry

#11693

bors-servo · 2016-06-11T16:01:37Z

⌛ Testing commit 041cfe6 with merge 2d93380...


        Auto merge of #11680 - pcwalton:wr-overflow-scroll-hit-testing, r=jdm

@jdm

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. Closes #11648. r? @jdm cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11680)

metajack · 2016-06-11T16:11:05Z

@bors-servo p=2

will help in london

bors-servo · 2016-06-11T20:06:14Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned jdm Jun 9, 2016

highfive added the S-awaiting-review label Jun 9, 2016

pcwalton force-pushed the pcwalton:wr-overflow-scroll-hit-testing branch from 90932e0 to 6010cb3 Jun 9, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 9, 2016

pcwalton force-pushed the pcwalton:wr-overflow-scroll-hit-testing branch from 6010cb3 to 22ff188 Jun 10, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 10, 2016

pcwalton force-pushed the pcwalton:wr-overflow-scroll-hit-testing branch from 22ff188 to 5afc4db Jun 10, 2016

highfive assigned glennw and unassigned jdm Jun 10, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 10, 2016

pcwalton force-pushed the pcwalton:wr-overflow-scroll-hit-testing branch from 5afc4db to 78a11e4 Jun 10, 2016

highfive removed the S-needs-code-changes label Jun 10, 2016

highfive added the S-awaiting-review label Jun 10, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 10, 2016

pcwalton force-pushed the pcwalton:wr-overflow-scroll-hit-testing branch from 78a11e4 to 041cfe6 Jun 11, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 11, 2016

script: When using WebRender, keep the DOM-side scroll positions for

Loading status checks…

041cfe6

elements with `overflow: scroll` up to date, and take them into account when doing hit testing. Closes #11648.

highfive removed the S-awaiting-merge label Jun 11, 2016

highfive added the S-tests-failed label Jun 11, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 11, 2016

bors-servo merged commit 041cfe6 into servo:master Jun 11, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 11, 2016

This was referenced Jun 11, 2016

Removed some sources of panic from script thread. #11695

Merged

Implement DOMMatrix (issue #8509) #11515

Closed

servo / servo

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. #11680

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. #11680

pcwalton commented Jun 9, 2016 •

edited by larsbergstrom

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

pcwalton commented Jun 10, 2016

highfive commented Jun 10, 2016

highfive commented Jun 10, 2016

pcwalton commented Jun 10, 2016

glennw commented Jun 10, 2016

jdm commented Jun 10, 2016

jdm commented Jun 10, 2016

highfive commented Jun 10, 2016

pcwalton commented Jun 10, 2016

pcwalton commented Jun 10, 2016

jdm commented Jun 10, 2016

glennw commented Jun 10, 2016

highfive commented Jun 11, 2016

pcwalton commented Jun 11, 2016

bors-servo commented Jun 11, 2016

bors-servo commented Jun 11, 2016

bors-servo commented Jun 11, 2016

highfive commented Jun 11, 2016

cbrewster commented Jun 11, 2016

bors-servo commented Jun 11, 2016

metajack commented Jun 11, 2016

bors-servo commented Jun 11, 2016

servo / servo

Join GitHub today

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. #11680

script: When using WebRender, keep the DOM-side scroll positions for elements with `overflow: scroll` up to date, and take them into account when doing hit testing. #11680

Conversation

pcwalton commented Jun 9, 2016 • edited by larsbergstrom

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

pcwalton commented Jun 10, 2016

highfive commented Jun 10, 2016

highfive commented Jun 10, 2016

pcwalton commented Jun 10, 2016

glennw commented Jun 10, 2016

jdm commented Jun 10, 2016

jdm commented Jun 10, 2016

highfive commented Jun 10, 2016

pcwalton commented Jun 10, 2016

pcwalton commented Jun 10, 2016

jdm commented Jun 10, 2016

glennw commented Jun 10, 2016

highfive commented Jun 11, 2016

pcwalton commented Jun 11, 2016

bors-servo commented Jun 11, 2016

bors-servo commented Jun 11, 2016

bors-servo commented Jun 11, 2016

highfive commented Jun 11, 2016

cbrewster commented Jun 11, 2016

bors-servo commented Jun 11, 2016

metajack commented Jun 11, 2016

bors-servo commented Jun 11, 2016

pcwalton commented Jun 9, 2016 •

edited by larsbergstrom