Auto merge of #14367 - mrobinson:scroll-fragment-point, r=pcwalton

Reimplement scrolling to fragments

<!-- 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 #13736, #10753 (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. -->

This reimplemntation of the feature uses ScrollRootIds to scroll
particular scrollable areas of the page.

Fixes #13736.
Fixes #10753.

<!-- 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/14367)
<!-- Reviewable:end -->

components/compositing/compositor.rs

@@ -39,7 +39,7 @@ use util::geometry::ScreenPx;
 use util::opts;
 use util::prefs::PREFS;
 use webrender;
-use webrender_traits::{self, ScrollEventPhase};
+use webrender_traits::{self, ScrollEventPhase, ServoScrollRootId};
 use windowing::{self, MouseWindowEvent, WindowEvent, WindowMethods, WindowNavigateMsg};
 
 #[derive(Debug, PartialEq)]
@@ -493,9 +493,9 @@ impl<Window: WindowMethods> IOCompositor<Window> {
                 self.title_for_main_frame();
             }
 
-            (Msg::ScrollFragmentPoint(pipeline_id, point, _),
+            (Msg::ScrollFragmentPoint(pipeline_id, scroll_root_id, point, _),
              ShutdownState::NotShuttingDown) => {
-                self.scroll_fragment_to_point(pipeline_id, point);
+                self.scroll_fragment_to_point(pipeline_id, scroll_root_id, point);
             }
 
             (Msg::MoveTo(point),
@@ -761,9 +761,13 @@ impl<Window: WindowMethods> IOCompositor<Window> {
     }
 
     fn scroll_fragment_to_point(&mut self,
-                                _pipeline_id: PipelineId,
+                                pipeline_id: PipelineId,
-                                _point: Point2D<f32>) {
+                                scroll_root_id: ScrollRootId,
-        println!("TODO: Support scroll_fragment_to_point again");
+                                point: Point2D<f32>) {
+        self.webrender_api.scroll_layers_with_scroll_root_id(
+            point,
+            pipeline_id.to_webrender(),
+            ServoScrollRootId(scroll_root_id.0));
     }
 
     fn handle_window_message(&mut self, event: WindowEvent) {

components/compositing/compositor_thread.rs

@@ -8,6 +8,7 @@ use SendableFrameTree;
 use compositor::CompositingReason;
 use euclid::point::Point2D;
 use euclid::size::Size2D;
+use gfx_traits::ScrollRootId;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::{Key, KeyModifiers, KeyState, PipelineId};
 use net_traits::image::base::Image;
@@ -72,7 +73,7 @@ pub enum Msg {
     ShutdownComplete,
 
     /// Scroll a page in a window
-    ScrollFragmentPoint(PipelineId, Point2D<f32>, bool),
+    ScrollFragmentPoint(PipelineId, ScrollRootId, Point2D<f32>, bool),
     /// Alerts the compositor that the current page has changed its title.
     ChangePageTitle(PipelineId, Option<String>),
     /// Alerts the compositor that the current page has changed its URL.

components/constellation/constellation.rs

@@ -1038,8 +1038,9 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
                 self.handle_alert(pipeline_id, message, sender);
             }
 
-            FromScriptMsg::ScrollFragmentPoint(pipeline_id, point, smooth) => {
+            FromScriptMsg::ScrollFragmentPoint(pipeline_id, scroll_root_id, point, smooth) => {
                 self.compositor_proxy.send(ToCompositorMsg::ScrollFragmentPoint(pipeline_id,
+                                                                                scroll_root_id,
                                                                                 point,
                                                                                 smooth));
             }

components/layout/flow.rs

@@ -406,7 +406,10 @@ pub trait Flow: fmt::Debug + Sync + Send + 'static {
     /// Print any extra children (such as fragments) contained in this Flow
     /// for debugging purposes. Any items inserted into the tree will become
     /// children of this flow.
-    fn print_extra_flow_children(&self, _: &mut PrintTree) {
+    fn print_extra_flow_children(&self, _: &mut PrintTree) { }
+
+    fn scroll_root_id(&self) -> ScrollRootId {
+        base(self).scroll_root_id
     }
 }
 

components/layout/query.rs

@@ -12,13 +12,14 @@ use euclid::size::Size2D;
 use flow::{self, Flow};
 use fragment::{Fragment, FragmentBorderBoxIterator, SpecificFragmentInfo};
 use gfx::display_list::{DisplayItemMetadata, DisplayList, OpaqueNode, ScrollOffsetMap};
+use gfx_traits::ScrollRootId;
 use ipc_channel::ipc::IpcSender;
 use opaque_node::OpaqueNodeMethods;
 use script_layout_interface::rpc::{ContentBoxResponse, ContentBoxesResponse};
 use script_layout_interface::rpc::{HitTestResponse, LayoutRPC};
 use script_layout_interface::rpc::{MarginStyleResponse, NodeGeometryResponse};
 use script_layout_interface::rpc::{NodeOverflowResponse, OffsetParentResponse};
-use script_layout_interface::rpc::ResolvedStyleResponse;
+use script_layout_interface::rpc::{NodeScrollRootIdResponse, ResolvedStyleResponse};
 use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use script_traits::LayoutMsg as ConstellationMsg;
 use script_traits::UntrustedNodeAddress;
@@ -64,6 +65,9 @@ pub struct LayoutThreadData {
     /// A queued response for the node at a given point
     pub hit_test_response: (Option<DisplayItemMetadata>, bool),
 
+    /// A queued response for the scroll root id for a given node.
+    pub scroll_root_id_response: Option<ScrollRootId>,
+
     /// A pair of overflow property in x and y
     pub overflow_response: NodeOverflowResponse,
 
@@ -178,6 +182,12 @@ impl LayoutRPC for LayoutRPCImpl {
         }
     }
 
+    fn node_scroll_root_id(&self) -> NodeScrollRootIdResponse {
+        NodeScrollRootIdResponse(self.0.lock()
+                                        .unwrap().scroll_root_id_response
+                                        .expect("scroll_root_id is not correctly fetched"))
+    }
+
     /// Retrieves the resolved value for a CSS style property.
     fn resolved_style(&self) -> ResolvedStyleResponse {
         let &LayoutRPCImpl(ref rw_data) = self;
@@ -578,6 +588,11 @@ pub fn process_node_geometry_request<N: LayoutNode>(requested_node: N, layout_ro
     iterator.client_rect
 }
 
+pub fn process_node_scroll_root_id_request<N: LayoutNode>(requested_node: N) -> ScrollRootId {
+    let layout_node = requested_node.to_threadsafe();
+    layout_node.scroll_root_id()
+}
+
 pub fn process_node_scroll_area_request< N: LayoutNode>(requested_node: N, layout_root: &mut Flow)
         -> Rect<i32> {
     let mut iterator = UnioningFragmentScrollAreaIterator::new(requested_node.opaque());

components/layout_thread/lib.rs

@@ -72,7 +72,7 @@ use layout::parallel;
 use layout::query::{LayoutRPCImpl, LayoutThreadData, process_content_box_request, process_content_boxes_request};
 use layout::query::{process_margin_style_query, process_node_overflow_request, process_resolved_style_request};
 use layout::query::{process_node_geometry_request, process_node_scroll_area_request};
-use layout::query::process_offset_parent_query;
+use layout::query::{process_node_scroll_root_id_request, process_offset_parent_query};
 use layout::sequential;
 use layout::traversal::{ComputeAbsolutePositions, RecalcStyleAndConstructFlows};
 use layout::webrender_helpers::WebRenderDisplayListConverter;
@@ -460,6 +460,7 @@ impl LayoutThread {
                     content_boxes_response: Vec::new(),
                     client_rect_response: Rect::zero(),
                     hit_test_response: (None, false),
+                    scroll_root_id_response: None,
                     scroll_area_response: Rect::zero(),
                     overflow_response: NodeOverflowResponse(None),
                     resolved_style_response: None,
@@ -1003,6 +1004,9 @@ impl LayoutThread {
                     ReflowQueryType::NodeOverflowQuery(_) => {
                         rw_data.overflow_response = NodeOverflowResponse(None);
                     },
+                    ReflowQueryType::NodeScrollRootIdQuery(_) => {
+                        rw_data.scroll_root_id_response = None;
+                    },
                     ReflowQueryType::ResolvedStyleQuery(_, _, _) => {
                         rw_data.resolved_style_response = None;
                     },
@@ -1232,6 +1236,10 @@ impl LayoutThread {
                 let node = unsafe { ServoLayoutNode::new(&node) };
                 rw_data.overflow_response = process_node_overflow_request(node);
             },
+            ReflowQueryType::NodeScrollRootIdQuery(node) => {
+                let node = unsafe { ServoLayoutNode::new(&node) };
+                rw_data.scroll_root_id_response = Some(process_node_scroll_root_id_request(node));
+            },
             ReflowQueryType::ResolvedStyleQuery(node, ref pseudo, ref property) => {
                 let node = unsafe { ServoLayoutNode::new(&node) };
                 let layout_context = LayoutContext::new(&shared_layout_context);
@@ -1556,9 +1564,9 @@ fn reflow_query_type_needs_display_list(query_type: &ReflowQueryType) -> bool {
         ReflowQueryType::HitTestQuery(..) => true,
         ReflowQueryType::ContentBoxQuery(_) | ReflowQueryType::ContentBoxesQuery(_) |
         ReflowQueryType::NodeGeometryQuery(_) | ReflowQueryType::NodeScrollGeometryQuery(_) |
-        ReflowQueryType::NodeOverflowQuery(_) | ReflowQueryType::ResolvedStyleQuery(..) |
+        ReflowQueryType::NodeOverflowQuery(_) | ReflowQueryType::NodeScrollRootIdQuery(_) |
-        ReflowQueryType::OffsetParentQuery(_) | ReflowQueryType::MarginStyleQuery(_) |
+        ReflowQueryType::ResolvedStyleQuery(..) | ReflowQueryType::OffsetParentQuery(_) |
-        ReflowQueryType::NoQuery => false,
+        ReflowQueryType::MarginStyleQuery(_) | ReflowQueryType::NoQuery => false,
     }
 }
 

components/script/dom/document.rs

@@ -87,6 +87,7 @@ use dom::window::{ReflowReason, Window};
 use encoding::EncodingRef;
 use encoding::all::UTF_8;
 use euclid::point::Point2D;
+use gfx_traits::ScrollRootId;
 use html5ever::tree_builder::{LimitedQuirks, NoQuirks, Quirks, QuirksMode};
 use html5ever_atoms::{LocalName, QualName};
 use ipc_channel::ipc::{self, IpcSender};
@@ -619,7 +620,10 @@ impl Document {
 
         if let Some((x, y)) = point {
             // Step 3
-            self.window.perform_a_scroll(x, y, ScrollBehavior::Instant,
+            self.window.perform_a_scroll(x,
+                                         y,
+                                         ScrollRootId::root(),
+                                         ScrollBehavior::Instant,
                                          target.r());
         }
     }

components/script/dom/window.rs

@@ -48,6 +48,7 @@ use dom::storage::Storage;
 use dom::testrunner::TestRunner;
 use euclid::{Point2D, Rect, Size2D};
 use fetch;
+use gfx_traits::ScrollRootId;
 use ipc_channel::ipc::{self, IpcSender};
 use js::jsapi::{HandleObject, HandleValue, JSAutoCompartment, JSContext};
 use js::jsapi::{JS_GC, JS_GetRuntime, SetWindowProxy};
@@ -67,7 +68,8 @@ use script_layout_interface::TrustedNodeAddress;
 use script_layout_interface::message::{Msg, Reflow, ReflowQueryType, ScriptReflow};
 use script_layout_interface::reporter::CSSErrorReporter;
 use script_layout_interface::rpc::{ContentBoxResponse, ContentBoxesResponse, LayoutRPC};
-use script_layout_interface::rpc::{MarginStyleResponse, ResolvedStyleResponse};
+use script_layout_interface::rpc::{MarginStyleResponse, NodeScrollRootIdResponse};
+use script_layout_interface::rpc::ResolvedStyleResponse;
 use script_runtime::{CommonScriptMsg, ScriptChan, ScriptPort, ScriptThreadEventCategory};
 use script_thread::{MainThreadScriptChan, MainThreadScriptMsg, Runnable, RunnableWrapper};
 use script_thread::SendableMainThreadScriptChan;
@@ -967,13 +969,20 @@ impl Window {
         //TODO Step 11
         //let document = self.Document();
         // Step 12
-        self.perform_a_scroll(x.to_f32().unwrap_or(0.0f32), y.to_f32().unwrap_or(0.0f32),
+        self.perform_a_scroll(x.to_f32().unwrap_or(0.0f32),
-                              behavior, None);
+                              y.to_f32().unwrap_or(0.0f32),
+                              ScrollRootId::root(),
+                              behavior,
+                              None);
     }
 
     /// https://drafts.csswg.org/cssom-view/#perform-a-scroll
-    pub fn perform_a_scroll(&self, x: f32, y: f32,
+    pub fn perform_a_scroll(&self,
-                            behavior: ScrollBehavior, element: Option<&Element>) {
+                            x: f32,
+                            y: f32,
+                            scroll_root_id: ScrollRootId,
+                            behavior: ScrollBehavior,
+                            element: Option<&Element>) {
         //TODO Step 1
         let point = Point2D::new(x, y);
         let smooth = match behavior {
@@ -992,7 +1001,7 @@ impl Window {
 
         let global_scope = self.upcast::<GlobalScope>();
         let message = ConstellationMsg::ScrollFragmentPoint(
-            global_scope.pipeline_id(), point, smooth);
+            global_scope.pipeline_id(), scroll_root_id, point, smooth);
         global_scope.constellation_chan().send(message).unwrap();
     }
 
@@ -1272,11 +1281,24 @@ impl Window {
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scroll
-    pub fn scroll_node(&self, _node: TrustedNodeAddress,
+    pub fn scroll_node(&self,
-                       x_: f64, y_: f64, behavior: ScrollBehavior) {
+                       node: TrustedNodeAddress,
+                       x_: f64,
+                       y_: f64,
+                       behavior: ScrollBehavior) {
+        if !self.reflow(ReflowGoal::ForScriptQuery,
+                        ReflowQueryType::NodeScrollRootIdQuery(node),
+                        ReflowReason::Query) {
+            return;
+        }
+        let NodeScrollRootIdResponse(scroll_root_id) = self.layout_rpc.node_scroll_root_id();
+
         // Step 12
-        self.perform_a_scroll(x_.to_f32().unwrap_or(0.0f32), y_.to_f32().unwrap_or(0.0f32),
+        self.perform_a_scroll(x_.to_f32().unwrap_or(0.0f32),
-                              behavior, None);
+                              y_.to_f32().unwrap_or(0.0f32),
+                              scroll_root_id,
+                              behavior,
+                              None);
     }
 
     pub fn resolved_style_query(&self,
@@ -1648,6 +1670,7 @@ fn debug_reflow_events(id: PipelineId, goal: &ReflowGoal, query_type: &ReflowQue
         ReflowQueryType::NodeGeometryQuery(_n) => "\tNodeGeometryQuery",
         ReflowQueryType::NodeOverflowQuery(_n) => "\tNodeOverFlowQuery",
         ReflowQueryType::NodeScrollGeometryQuery(_n) => "\tNodeScrollGeometryQuery",
+        ReflowQueryType::NodeScrollRootIdQuery(_n) => "\tNodeScrollRootIdQuery",
         ReflowQueryType::ResolvedStyleQuery(_, _, _) => "\tResolvedStyleQuery",
         ReflowQueryType::OffsetParentQuery(_n) => "\tOffsetParentQuery",
         ReflowQueryType::MarginStyleQuery(_n) => "\tMarginStyleQuery",

components/script_layout_interface/message.rs

@@ -94,6 +94,7 @@ pub enum ReflowQueryType {
     ContentBoxesQuery(TrustedNodeAddress),
     NodeOverflowQuery(TrustedNodeAddress),
     HitTestQuery(Point2D<f32>, Point2D<f32>, bool),
+    NodeScrollRootIdQuery(TrustedNodeAddress),
     NodeGeometryQuery(TrustedNodeAddress),
     NodeScrollGeometryQuery(TrustedNodeAddress),
     ResolvedStyleQuery(TrustedNodeAddress, Option<PseudoElement>, Atom),

components/script_layout_interface/rpc.rs

@@ -5,6 +5,7 @@
 use app_units::Au;
 use euclid::point::Point2D;
 use euclid::rect::Rect;
+use gfx_traits::ScrollRootId;
 use script_traits::UntrustedNodeAddress;
 use style::properties::longhands::{margin_top, margin_right, margin_bottom, margin_left, overflow_x};
 
@@ -27,6 +28,8 @@ pub trait LayoutRPC {
     fn node_overflow(&self) -> NodeOverflowResponse;
     /// Requests the scroll geometry of this node. Used by APIs such as `scrollTop`.
     fn node_scroll_area(&self) -> NodeGeometryResponse;
+    /// Requests the scroll root id of this node. Used by APIs such as `scrollTop`
+    fn node_scroll_root_id(&self) -> NodeScrollRootIdResponse;
     /// Requests the node containing the point of interest
     fn hit_test(&self) -> HitTestResponse;
     /// Query layout for the resolved value of a given CSS property
@@ -48,6 +51,8 @@ pub struct NodeGeometryResponse {
 
 pub struct NodeOverflowResponse(pub Option<Point2D<overflow_x::computed_value::T>>);
 
+pub struct NodeScrollRootIdResponse(pub ScrollRootId);
+
 pub struct HitTestResponse {
     pub node_address: Option<UntrustedNodeAddress>,
 }

components/script_layout_interface/wrapper_traits.rs

@@ -8,7 +8,7 @@ use HTMLCanvasData;
 use LayoutNodeType;
 use OpaqueStyleAndLayoutData;
 use SVGSVGData;
-use gfx_traits::ByteIndex;
+use gfx_traits::{ByteIndex, FragmentType, ScrollRootId};
 use html5ever_atoms::{Namespace, LocalName};
 use msg::constellation_msg::PipelineId;
 use range::Range;
@@ -264,6 +264,20 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + Debug + GetLayoutData + NodeInfo
     fn iframe_pipeline_id(&self) -> PipelineId;
 
     fn get_colspan(&self) -> u32;
+
+    fn fragment_type(&self) -> FragmentType {
+        match self.get_pseudo_element_type() {
+            PseudoElementType::Normal => FragmentType::FragmentBody,
+            PseudoElementType::Before(_) => FragmentType::BeforePseudoContent,
+            PseudoElementType::After(_) => FragmentType::AfterPseudoContent,
+            PseudoElementType::DetailsSummary(_) => FragmentType::FragmentBody,
+            PseudoElementType::DetailsContent(_) => FragmentType::FragmentBody,
+        }
+    }
+
+    fn scroll_root_id(&self) -> ScrollRootId {
+        ScrollRootId::new_of_type(self.opaque().id() as usize, self.fragment_type())
+    }
 }
 
 // This trait is only public so that it can be implemented by the gecko wrapper.

components/script_traits/script_msg.rs

@@ -16,6 +16,7 @@ use canvas_traits::CanvasMsg;
 use devtools_traits::{ScriptToDevtoolsControlMsg, WorkerId};
 use euclid::point::Point2D;
 use euclid::size::Size2D;
+use gfx_traits::ScrollRootId;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::{FrameId, PipelineId, TraversalDirection};
 use msg::constellation_msg::{Key, KeyModifiers, KeyState};
@@ -121,7 +122,7 @@ pub enum ScriptMsg {
     /// Check if an alert dialog box should be presented
     Alert(PipelineId, String, IpcSender<bool>),
     /// Scroll a page in a window
-    ScrollFragmentPoint(PipelineId, Point2D<f32>, bool),
+    ScrollFragmentPoint(PipelineId, ScrollRootId, Point2D<f32>, bool),
     /// Set title of current page
     /// https://html.spec.whatwg.org/multipage/#document.title
     SetTitle(PipelineId, Option<String>),

tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html.ini

@@ -1,6 +0,0 @@
-[scroll-frag-percent-encoded.html]
-  type: testharness
-  disabled: https://github.com/servo/servo/issues/10753
-  [Fragment Navigation: fragment id should be percent-decoded]
-    expected: FAIL
-

tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-id-top.html.ini

@@ -1,6 +0,0 @@
-[scroll-to-id-top.html]
-  type: testharness
-  disabled: https://github.com/servo/servo/issues/10753
-  [Fragment Navigation: TOP is a valid element id, which overrides navigating to top of the document]
-    expected: FAIL
-

tests/wpt/metadata/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html.ini

@@ -1,6 +0,0 @@
-[scroll-to-top.html]
-  type: testharness
-  disabled: https://github.com/servo/servo/issues/10753
-  [Fragment Navigation: When fragid is TOP scroll to the top of the document]
-    expected: FAIL
-

tests/wpt/mozilla/meta/MANIFEST.json

@@ -6414,6 +6414,18 @@
             "url": "/_mozilla/mozilla/iframe/resize_after_load.html"
           }
         ],
+        "mozilla/simple_scroll_to_fragment.html": [
+          {
+            "path": "mozilla/simple_scroll_to_fragment.html",
+            "references": [
+              [
+                "/_mozilla/mozilla/simple_scroll_to_fragment_ref.html",
+                "=="
+              ]
+            ],
+            "url": "/_mozilla/mozilla/simple_scroll_to_fragment.html"
+          }
+        ],
         "mozilla/sslfail.html": [
           {
             "path": "mozilla/sslfail.html",
@@ -21432,6 +21444,18 @@
           "url": "/_mozilla/mozilla/iframe/resize_after_load.html"
         }
       ],
+      "mozilla/simple_scroll_to_fragment.html": [
+        {
+          "path": "mozilla/simple_scroll_to_fragment.html",
+          "references": [
+            [
+              "/_mozilla/mozilla/simple_scroll_to_fragment_ref.html",
+              "=="
+            ]
+          ],
+          "url": "/_mozilla/mozilla/simple_scroll_to_fragment.html"
+        }
+      ],
       "mozilla/sslfail.html": [
         {
           "path": "mozilla/sslfail.html",

tests/wpt/mozilla/tests/mozilla/simple_scroll_to_fragment.html

@@ -0,0 +1,14 @@
+<!doctype html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <link rel="match" href="simple_scroll_to_fragment_ref.html">
+    </head>
+    <body>
+        <div id="scroller" style="height: 100px; width: 100px; overflow: scroll; background: red;">
+            <div style="background: green; margin-top: 100px; width: 100px; height: 100px;"></div>
+        </div>
+        <script>
+            document.getElementById("scroller").scrollTop = 100;
+        </script>
+</html>

tests/wpt/mozilla/tests/mozilla/simple_scroll_to_fragment_ref.html

@@ -0,0 +1,9 @@
+<!doctype html>
+<html>
+    <head>
+        <meta charset="utf-8">
+    </head>
+    <body>
+        <div style="height: 100px; width: 100px; background: green;">
+        </div>
+</html>