Auto merge of #9715 - ecoal95:mousemove, r=mbrubeck

script: Fix MouseOver handling

Now we only query for the topmost node, and apply the hover state to all
of the parent elements.

This fixes things like #9705, where the hover state was applied only to
the children.

This also makes us more conformant with other browsers in the case of
taking in account margins and paddings.

For example, prior to this PR, when your mouse was over the inner
element, in the bottom part, `hover` styles didn't apply to the parent.

```html
<style>
div {
  padding: 10px;
  margin: 10px;
  height: 15px;
  background: blue;
}

div:hover {
  background: red;
}
</style>

<div>
  <div></div>
</div>
```

Fixes #9705

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9715)
<!-- Reviewable:end -->

components/layout/query.rs

@@ -16,7 +16,7 @@ use layout_thread::LayoutThreadData;
 use msg::constellation_msg::ConstellationChan;
 use opaque_node::OpaqueNodeMethods;
 use script::layout_interface::{ContentBoxResponse, ContentBoxesResponse, NodeGeometryResponse};
-use script::layout_interface::{HitTestResponse, LayoutRPC, MouseOverResponse, OffsetParentResponse};
+use script::layout_interface::{HitTestResponse, LayoutRPC, OffsetParentResponse};
 use script::layout_interface::{ResolvedStyleResponse, ScriptLayoutChan, MarginStyleResponse};
 use script_traits::LayoutMsg as ConstellationMsg;
 use sequential;
@@ -67,51 +67,29 @@ impl LayoutRPC for LayoutRPCImpl {
     }
 
     /// Requests the node containing the point of interest.
-    fn hit_test(&self, point: Point2D<f32>) -> Result<HitTestResponse, ()> {
+    fn hit_test(&self, point: Point2D<f32>, update_cursor: bool) -> Result<HitTestResponse, ()> {
         let point = Point2D::new(Au::from_f32_px(point.x), Au::from_f32_px(point.y));
         let &LayoutRPCImpl(ref rw_data) = self;
         let rw_data = rw_data.lock().unwrap();
-        let result = match rw_data.display_list {
-            None => panic!("Tried to hit test without a DisplayList"),
-            Some(ref display_list) => display_list.hit_test(point),
-        };
+        let display_list = rw_data.display_list.as_ref().expect("Tried to hit test without a DisplayList!");
 
-        if result.is_empty() {
-            return Err(());
-        }
+        let result = display_list.hit_test(point);
 
-        Ok(HitTestResponse(result[0].node.to_untrusted_node_address()))
-    }
-
-    fn mouse_over(&self, point: Point2D<f32>) -> Result<MouseOverResponse, ()> {
-        let point = Point2D::new(Au::from_f32_px(point.x), Au::from_f32_px(point.y));
-        let mouse_over_list = {
-            let &LayoutRPCImpl(ref rw_data) = self;
-            let rw_data = rw_data.lock().unwrap();
-            let result = match rw_data.display_list {
-                None => panic!("Tried to hit test without a DisplayList"),
-                Some(ref display_list) => display_list.hit_test(point),
-            };
-
-            // Compute the new cursor.
+        if update_cursor {
             let cursor = if !result.is_empty() {
                 result[0].pointing.unwrap()
             } else {
                 Cursor::DefaultCursor
             };
+
             let ConstellationChan(ref constellation_chan) = rw_data.constellation_chan;
             constellation_chan.send(ConstellationMsg::SetCursor(cursor)).unwrap();
-            result
         };
 
-        if mouse_over_list.is_empty() {
-            Err(())
+        if !result.is_empty() {
+            Ok(HitTestResponse(result[0].node.to_untrusted_node_address()))
         } else {
-            let response_list =
-                mouse_over_list.iter()
-                               .map(|metadata| metadata.node.to_untrusted_node_address())
-                               .collect();
-            Ok(MouseOverResponse(response_list))
+            Err(())
         }
     }
 

components/script/dom/document.rs

@@ -80,7 +80,7 @@ use html5ever::tree_builder::{LimitedQuirks, NoQuirks, Quirks, QuirksMode};
 use ipc_channel::ipc::{self, IpcSender};
 use js::jsapi::JS_GetRuntime;
 use js::jsapi::{JSContext, JSObject, JSRuntime};
-use layout_interface::{HitTestResponse, MouseOverResponse};
+use layout_interface::HitTestResponse;
 use layout_interface::{LayoutChan, Msg, ReflowQueryType};
 use msg::constellation_msg::{ALT, CONTROL, SHIFT, SUPER};
 use msg::constellation_msg::{ConstellationChan, Key, KeyModifiers, KeyState};
@@ -563,9 +563,9 @@ impl Document {
                 .map(Root::upcast)
     }
 
-    pub fn hit_test(&self, page_point: &Point2D<f32>) -> Option<UntrustedNodeAddress> {
+    pub fn hit_test(&self, page_point: &Point2D<f32>, update_cursor: bool) -> Option<UntrustedNodeAddress> {
         assert!(self.GetDocumentElement().is_some());
-        match self.window.layout().hit_test(*page_point) {
+        match self.window.layout().hit_test(*page_point, update_cursor) {
             Ok(HitTestResponse(node_address)) => Some(node_address),
             Err(()) => {
                 debug!("layout query error");
@@ -574,14 +574,6 @@ impl Document {
         }
     }
 
-    pub fn get_nodes_under_mouse(&self, page_point: &Point2D<f32>) -> Vec<UntrustedNodeAddress> {
-        assert!(self.GetDocumentElement().is_some());
-        match self.window.layout().mouse_over(*page_point) {
-            Ok(MouseOverResponse(node_address)) => node_address,
-            Err(()) => vec![],
-        }
-    }
-
     // https://html.spec.whatwg.org/multipage/#current-document-readiness
     pub fn set_ready_state(&self, state: DocumentReadyState) {
         match state {
@@ -693,7 +685,7 @@ impl Document {
 
         let page_point = Point2D::new(client_point.x + self.window.PageXOffset() as f32,
                                       client_point.y + self.window.PageYOffset() as f32);
-        let node = match self.hit_test(&page_point) {
+        let node = match self.hit_test(&page_point, false) {
             Some(node_address) => {
                 debug!("node address is {:?}", node_address);
                 node::from_untrusted_node_address(js_runtime, node_address)
@@ -806,63 +798,33 @@ impl Document {
     pub fn handle_mouse_move_event(&self,
                                    js_runtime: *mut JSRuntime,
                                    client_point: Option<Point2D<f32>>,
-                                   prev_mouse_over_targets: &mut RootedVec<JS<Element>>) {
-        // Build a list of elements that are currently under the mouse.
-        let mouse_over_addresses = client_point.as_ref().map(|client_point| {
-            let page_point = Point2D::new(client_point.x + self.window.PageXOffset() as f32,
-                                          client_point.y + self.window.PageYOffset() as f32);
-            self.get_nodes_under_mouse(&page_point)
-        }).unwrap_or(vec![]);
-        let mut mouse_over_targets = mouse_over_addresses.iter().map(|node_address| {
-            node::from_untrusted_node_address(js_runtime, *node_address)
-                .inclusive_ancestors()
+                                   prev_mouse_over_target: &MutNullableHeap<JS<Element>>) {
+        let page_point = match client_point {
+            None => {
+                // If there's no point, there's no target under the mouse
+                // FIXME: dispatch mouseout here. We have no point.
+                prev_mouse_over_target.set(None);
+                return;
+            }
+            Some(ref client_point) => {
+                Point2D::new(client_point.x + self.window.PageXOffset() as f32,
+                             client_point.y + self.window.PageYOffset() as f32)
+            }
+        };
+
+        let client_point = client_point.unwrap();
+
+        let maybe_new_target = self.hit_test(&page_point, true).and_then(|address| {
+            let node = node::from_untrusted_node_address(js_runtime, address);
+            node.inclusive_ancestors()
                 .filter_map(Root::downcast::<Element>)
                 .next()
-                .unwrap()
-        }).collect::<RootedVec<JS<Element>>>();
-
-        // Remove hover from any elements in the previous list that are no longer
-        // under the mouse.
-        for target in prev_mouse_over_targets.iter() {
-            if !mouse_over_targets.contains(target) {
-                let target_ref = &**target;
-                if target_ref.get_hover_state() {
-                    target_ref.set_hover_state(false);
-
-                    let target = target_ref.upcast();
-
-                    // FIXME: we should be dispatching this event but we lack an actual
-                    //        point to pass to it.
-                    if let Some(client_point) = client_point {
-                        self.fire_mouse_event(client_point, &target, "mouseout".to_owned());
-                    }
-                }
-            }
-        }
-
-        // Set hover state for any elements in the current mouse over list.
-        // Check if any of them changed state to determine whether to
-        // force a reflow below.
-        for target in mouse_over_targets.r() {
-            if !target.get_hover_state() {
-                target.set_hover_state(true);
-
-                let target = target.upcast();
-
-                if let Some(client_point) = client_point {
-                    self.fire_mouse_event(client_point, target, "mouseover".to_owned());
-                }
-            }
-        }
-
-        // Send mousemove event to topmost target
-        if mouse_over_addresses.len() > 0 {
-            let top_most_node = node::from_untrusted_node_address(js_runtime,
-                                                                  mouse_over_addresses[0]);
-            let client_point = client_point.unwrap(); // Must succeed because hit test succeeded.
+        });
 
+        // Send mousemove event to topmost target, and forward it if it's an iframe
+        if let Some(ref new_target) = maybe_new_target {
             // If the target is an iframe, forward the event to the child document.
-            if let Some(iframe) = top_most_node.downcast::<HTMLIFrameElement>() {
+            if let Some(iframe) = new_target.downcast::<HTMLIFrameElement>() {
                 if let Some(pipeline_id) = iframe.pipeline_id() {
                     let rect = iframe.upcast::<Element>().GetBoundingClientRect();
                     let child_origin = Point2D::new(rect.X() as f32, rect.Y() as f32);
@@ -874,13 +836,59 @@ impl Document {
                 return;
             }
 
-            let target = top_most_node.upcast();
-            self.fire_mouse_event(client_point, target, "mousemove".to_owned());
+            self.fire_mouse_event(client_point, new_target.upcast(), "mousemove".to_owned());
         }
 
-        // Store the current mouse over targets for next frame
-        prev_mouse_over_targets.clear();
-        prev_mouse_over_targets.append(&mut *mouse_over_targets);
+        // Nothing more to do here, mousemove is sent,
+        // and the element under the mouse hasn't changed.
+        if maybe_new_target == prev_mouse_over_target.get() {
+            return;
+        }
+
+        let old_target_is_ancestor_of_new_target = match (prev_mouse_over_target.get(), maybe_new_target.as_ref()) {
+            (Some(old_target), Some(new_target))
+                => old_target.upcast::<Node>().is_ancestor_of(new_target.upcast::<Node>()),
+            _   => false,
+        };
+
+        // Here we know the target has changed, so we must update the state,
+        // dispatch mouseout to the previous one, mouseover to the new one,
+        if let Some(old_target) = prev_mouse_over_target.get() {
+            // If the old target is an ancestor of the new target, this can be skipped
+            // completely, since the node's hover state will be reseted below.
+            if !old_target_is_ancestor_of_new_target {
+                for element in old_target.upcast::<Node>()
+                                         .inclusive_ancestors()
+                                         .filter_map(Root::downcast::<Element>) {
+                    element.set_hover_state(false);
+                }
+            }
+
+            // Remove hover state to old target and its parents
+            self.fire_mouse_event(client_point, old_target.upcast(), "mouseout".to_owned());
+
+            // TODO: Fire mouseleave here only if the old target is
+            // not an ancestor of the new target.
+        }
+
+        if let Some(ref new_target) = maybe_new_target {
+            for element in new_target.upcast::<Node>()
+                                     .inclusive_ancestors()
+                                     .filter_map(Root::downcast::<Element>) {
+                if element.get_hover_state() {
+                    break;
+                }
+
+                element.set_hover_state(true);
+            }
+
+            self.fire_mouse_event(client_point, &new_target.upcast(), "mouseover".to_owned());
+
+            // TODO: Fire mouseenter here.
+        }
+
+        // Store the current mouse over target for next frame.
+        prev_mouse_over_target.set(maybe_new_target.as_ref().map(|target| target.r()));
 
         self.window.reflow(ReflowGoal::ForDisplay,
                            ReflowQueryType::NoQuery,
@@ -900,7 +908,7 @@ impl Document {
             TouchEventType::Cancel => "touchcancel",
         };
 
-        let node = match self.hit_test(&point) {
+        let node = match self.hit_test(&point, false) {
             Some(node_address) => node::from_untrusted_node_address(js_runtime, node_address),
             None => return false,
         };
@@ -2567,7 +2575,7 @@ impl DocumentMethods for Document {
 
         let js_runtime = unsafe { JS_GetRuntime(window.get_cx()) };
 
-        match self.hit_test(point)  {
+        match self.hit_test(point, false)  {
             Some(untrusted_node_address) => {
                 let node = node::from_untrusted_node_address(js_runtime, untrusted_node_address);
                 let parent_node = node.GetParentNode().unwrap();

components/script/dom/node.rs

@@ -518,7 +518,11 @@ impl Node {
     }
 
     pub fn is_inclusive_ancestor_of(&self, parent: &Node) -> bool {
-        self == parent || parent.ancestors().any(|ancestor| ancestor.r() == self)
+        self == parent || self.is_ancestor_of(parent)
+    }
+
+    pub fn is_ancestor_of(&self, parent: &Node) -> bool {
+        parent.ancestors().any(|ancestor| ancestor.r() == self)
     }
 
     pub fn following_siblings(&self) -> NodeSiblingIterator {

components/script/layout_interface.rs

@@ -105,8 +105,7 @@ pub trait LayoutRPC {
     /// Requests the geometry of this node. Used by APIs such as `clientTop`.
     fn node_geometry(&self) -> NodeGeometryResponse;
     /// Requests the node containing the point of interest
-    fn hit_test(&self, point: Point2D<f32>) -> Result<HitTestResponse, ()>;
-    fn mouse_over(&self, point: Point2D<f32>) -> Result<MouseOverResponse, ()>;
+    fn hit_test(&self, point: Point2D<f32>, update_cursor: bool) -> Result<HitTestResponse, ()>;
     /// Query layout for the resolved value of a given CSS property
     fn resolved_style(&self) -> ResolvedStyleResponse;
     fn offset_parent(&self) -> OffsetParentResponse;
@@ -139,7 +138,6 @@ pub struct NodeGeometryResponse {
     pub client_rect: Rect<i32>,
 }
 pub struct HitTestResponse(pub UntrustedNodeAddress);
-pub struct MouseOverResponse(pub Vec<UntrustedNodeAddress>);
 pub struct ResolvedStyleResponse(pub Option<String>);
 
 #[derive(Clone)]

components/script/script_thread.rs

@@ -27,10 +27,10 @@ use dom::bindings::codegen::Bindings::DocumentBinding::{DocumentMethods, Documen
 use dom::bindings::conversions::{FromJSValConvertible, StringificationBehavior};
 use dom::bindings::global::GlobalRef;
 use dom::bindings::inheritance::Castable;
-use dom::bindings::js::{JS, RootCollection, trace_roots};
+use dom::bindings::js::{JS, MutNullableHeap, Root, RootCollection, trace_roots};
 use dom::bindings::js::{RootCollectionPtr, RootedReference};
 use dom::bindings::refcounted::{LiveDOMReferences, Trusted, TrustedReference, trace_refcounted_objects};
-use dom::bindings::trace::{JSTraceable, RootedVec, trace_traceables};
+use dom::bindings::trace::{JSTraceable, trace_traceables};
 use dom::bindings::utils::{DOM_CALLBACKS, WRAP_CALLBACKS};
 use dom::browsingcontext::BrowsingContext;
 use dom::document::{Document, DocumentProgressHandler, DocumentSource, FocusType, IsHTMLDocument};
@@ -91,7 +91,6 @@ use std::cell::{Cell, RefCell};
 use std::collections::HashSet;
 use std::io::{Write, stdout};
 use std::marker::PhantomData;
-use std::mem as std_mem;
 use std::option::Option;
 use std::ptr;
 use std::rc::Rc;
@@ -543,7 +542,8 @@ pub struct ScriptThread {
     /// The JavaScript runtime.
     js_runtime: Rc<Runtime>,
 
-    mouse_over_targets: DOMRefCell<Vec<JS<Element>>>,
+    /// The topmost element over the mouse.
+    topmost_mouse_over_target: MutNullableHeap<JS<Element>>,
 
     /// List of pipelines that have been owned and closed by this script thread.
     closed_pipelines: DOMRefCell<HashSet<PipelineId>>,
@@ -789,7 +789,7 @@ impl ScriptThread {
             devtools_sender: ipc_devtools_sender,
 
             js_runtime: Rc::new(runtime),
-            mouse_over_targets: DOMRefCell::new(vec!()),
+            topmost_mouse_over_target: MutNullableHeap::new(Default::default()),
             closed_pipelines: DOMRefCell::new(HashSet::new()),
 
             scheduler_chan: state.scheduler_chan,
@@ -1979,46 +1979,55 @@ impl ScriptThread {
                 let page = get_page(&self.root_page(), pipeline_id);
                 let document = page.document();
 
-                let mut prev_mouse_over_targets: RootedVec<JS<Element>> = RootedVec::new();
-                for target in &*self.mouse_over_targets.borrow_mut() {
-                    prev_mouse_over_targets.push(target.clone());
+                // Get the previous target temporarily
+                let prev_mouse_over_target = self.topmost_mouse_over_target.get();
+
+                document.handle_mouse_move_event(self.js_runtime.rt(), point,
+                                                 &self.topmost_mouse_over_target);
+
+                // Short-circuit if nothing changed
+                if self.topmost_mouse_over_target.get() == prev_mouse_over_target {
+                    return;
                 }
 
-                // We temporarily steal the list of targets over which the mouse is to pass it to
-                // handle_mouse_move_event() in a safe RootedVec container.
-                let mut mouse_over_targets = RootedVec::new();
-                std_mem::swap(&mut *self.mouse_over_targets.borrow_mut(), &mut *mouse_over_targets);
-                document.handle_mouse_move_event(self.js_runtime.rt(), point, &mut mouse_over_targets);
-
-                // Notify Constellation about anchors that are no longer mouse over targets.
-                for target in &*prev_mouse_over_targets {
-                    if !mouse_over_targets.contains(target) && target.is::<HTMLAnchorElement>() {
-                        let event = ConstellationMsg::NodeStatus(None);
-                        let ConstellationChan(ref chan) = self.constellation_chan;
-                        chan.send(event).unwrap();
-                        break;
-                    }
-                }
+                let mut state_already_changed = false;
 
                 // Notify Constellation about the topmost anchor mouse over target.
-                for target in &*mouse_over_targets {
-                    if target.is::<HTMLAnchorElement>() {
-                        let status = target.get_attribute(&ns!(), &atom!("href"))
+                if let Some(target) = self.topmost_mouse_over_target.get() {
+                    if let Some(anchor) = target.upcast::<Node>()
+                                                .inclusive_ancestors()
+                                                .filter_map(Root::downcast::<HTMLAnchorElement>)
+                                                .next() {
+                        let status = anchor.upcast::<Element>()
+                                           .get_attribute(&ns!(), &atom!("href"))
                                            .and_then(|href| {
                                                let value = href.value();
                                                let url = document.url();
                                                url.join(&value).map(|url| url.serialize()).ok()
                                            });
+
                         let event = ConstellationMsg::NodeStatus(status);
                         let ConstellationChan(ref chan) = self.constellation_chan;
                         chan.send(event).unwrap();
-                        break;
+
+                        state_already_changed = true;
                     }
                 }
 
-                std_mem::swap(&mut *self.mouse_over_targets.borrow_mut(), &mut *mouse_over_targets);
+                // We might have to reset the anchor state
+                if !state_already_changed {
+                    if let Some(target) = prev_mouse_over_target {
+                        if let Some(_) = target.upcast::<Node>()
+                                               .inclusive_ancestors()
+                                               .filter_map(Root::downcast::<HTMLAnchorElement>)
+                                               .next() {
+                            let event = ConstellationMsg::NodeStatus(None);
+                            let ConstellationChan(ref chan) = self.constellation_chan;
+                            chan.send(event).unwrap();
+                        }
+                    }
+                }
             }
-
             TouchEvent(event_type, identifier, point) => {
                 let handled = self.handle_touch_event(pipeline_id, event_type, identifier, point);
                 match event_type {

tests/wpt/metadata/MANIFEST.json

@@ -34513,6 +34513,14 @@
   "local_changes": {
     "deleted": [],
     "items": {
+      "manual": {
+        "uievents/order-of-events/mouse-events/mouseover-out-manual.html": [
+          {
+            "path": "uievents/order-of-events/mouse-events/mouseover-out-manual.html",
+            "url": "/uievents/order-of-events/mouse-events/mouseover-out-manual.html"
+          }
+        ]
+      },
       "testharness": {
         "dom/lists/DOMTokenList-value.html": [
           {

tests/wpt/web-platform-tests/uievents/order-of-events/mouse-events/mouseover-out-manual.html

@@ -0,0 +1,125 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Mouseover/mouseout handling</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+#outer,
+#inner,
+#released {
+  padding: 10px;
+  margin: 10px;
+  height: 15px;
+}
+
+#outer:hover,
+#inner:hover,
+#released:hover {
+  background: red;
+}
+
+#outer {
+  background: blue;
+}
+
+#inner {
+  background: green;
+}
+
+#released {
+  background: yellow;
+}
+</style>
+<p>
+  Steps:
+
+  <ol>
+    <li>Move your mouse over the blue <code>&lt;div&gt;</code> element, later
+      over the green one, later over the yellow one.
+    <li>Move the mouse from the yellow element to the green one, later to the
+      blue one, and later over this paragraph.
+  </ol>
+</p>
+
+
+<div id="outer">
+  <div id="inner"></div>
+</div>
+<div id="released"></div>
+
+<div id="log"></div>
+
+<script>
+var t = async_test("Mouseover/out events");
+var outer = document.getElementById("outer");
+var inner = document.getElementById("inner");
+
+var inner_over = 0;
+var inner_out = 0;
+
+var outer_own_over = 0;
+var outer_own_out = 0;
+var outer_over = 0;
+var outer_out = 0;
+
+
+inner.addEventListener("mouseover", t.step_func(function(e) {
+  assert_true(inner_over == inner_out, "mouseover is received before mouseout");
+
+  switch (inner_over) {
+    case 0:
+      assert_true(outer_own_over == 1, "should have triggered a mouseover in the outer before");
+      break;
+    case 1:
+      assert_true(outer_own_over == 1, "should have not triggered a mouseover in the outer before");
+      break;
+    default:
+      assert_true(false, "should not get more than two mouseovers");
+  }
+
+  inner_over++;
+}), false);
+
+inner.addEventListener("mouseout", t.step_func(function(e) {
+  assert_true(inner_over == inner_out + 1, "mouseout is received after mouseover");
+
+  switch (inner_out) {
+    case 0:
+      assert_true(outer_own_out == 1, "mouseout should have been received in the parent when hovering over this element");
+      break;
+    case 1:
+      break;
+    default:
+      assert_true(false, "should not get more than two mouseouts");
+  }
+
+  inner_out++;
+}), false);
+
+outer.addEventListener("mouseover", t.step_func(function(e) {
+  if (e.target == outer) {
+    assert_true(outer_own_over == outer_own_out, "outer: mouseover is received before mouseout");
+    outer_own_over++;
+  } else {
+    assert_true(outer_over - outer_own_over == inner_over - 1, "mouseover: should only receive this via bubbling");
+  }
+
+  outer_over++;
+}), false);
+
+outer.addEventListener('mouseout', t.step_func(function(e) {
+  if (e.target == outer) {
+    assert_true(outer_own_over == outer_own_out + 1, "outer: mouseout is received after mouseover");
+    if (outer_own_out == 1) {
+      assert_true(inner_out == 2, "inner should be done now");
+      t.done();
+    }
+
+    outer_own_out++;
+  } else {
+    assert_true(outer_out - outer_own_out == inner_out - 1, "mouseout: should only receive this via bubbling");
+  }
+
+  outer_out++;
+}), false);
+</script>