From 5fe54b800334d0963cbf52496e2cbce42d839c46 Mon Sep 17 00:00:00 2001 From: Utsav Oza Date: Sun, 26 Jul 2020 18:36:30 +0530 Subject: [PATCH 1/5] Fix ordering of mousemove event --- components/compositing/compositor.rs | 2 +- components/script/dom/document.rs | 160 +++++++++++++-------------- components/script_traits/lib.rs | 2 +- 3 files changed, 80 insertions(+), 84 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index ba5002a83dd..49b33f1d7de 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -940,7 +940,7 @@ impl IOCompositor { let results = self.hit_test_at_point(cursor); if let Some(item) = results.items.first() { let node_address = Some(UntrustedNodeAddress(item.tag.0 as *const c_void)); - let event = MouseMoveEvent(Some(item.point_in_viewport.to_untyped()), node_address, 0); + let event = MouseMoveEvent(item.point_in_viewport.to_untyped(), node_address, 0); let pipeline_id = PipelineId::from_webrender(item.pipeline); let msg = ConstellationMsg::ForwardEvent(pipeline_id, event); if let Err(e) = self.constellation_chan.send(msg) { diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 8244f4af31f..cb4d9160cac 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1376,21 +1376,11 @@ impl Document { pub fn handle_mouse_move_event( &self, js_runtime: *mut JSRuntime, - client_point: Option>, + client_point: Point2D, prev_mouse_over_target: &MutNullableDom, node_address: Option, pressed_mouse_buttons: u16, ) { - let client_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(client_point) => client_point, - }; - let maybe_new_target = node_address.and_then(|address| { let node = unsafe { node::from_untrusted_node_address(js_runtime, address) }; node.inclusive_ancestors(ShadowIncluding::No) @@ -1398,13 +1388,82 @@ impl Document { .next() }); - // Send mousemove event to topmost target, unless it's an iframe, in which case the - // compositor should have also sent an event to the inner document. let new_target = match maybe_new_target { Some(ref target) => target, None => return, }; + let target_has_changed = prev_mouse_over_target + .get() + .as_ref() + .map_or(true, |old_target| old_target != new_target); + + // 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 target_has_changed { + // Dispatch mouseout to previous target + if let Some(old_target) = prev_mouse_over_target.get() { + let old_target_is_ancestor_of_new_target = prev_mouse_over_target + .get() + .as_ref() + .map_or(false, |old_target| { + old_target + .upcast::() + .is_ancestor_of(new_target.upcast::()) + }); + + // 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::() + .inclusive_ancestors(ShadowIncluding::No) + .filter_map(DomRoot::downcast::) + { + element.set_hover_state(false); + element.set_active_state(false); + } + } + + // Remove hover state to old target and its parents + self.fire_mouse_event( + client_point, + old_target.upcast(), + FireMouseEventType::Out, + pressed_mouse_buttons, + ); + + // TODO: Fire mouseleave here only if the old target is + // not an ancestor of the new target. + } + + // Dispatch mouseover to new target - TODO: Redundant check? + if let Some(new_target) = maybe_new_target.as_ref() { + for element in new_target + .upcast::() + .inclusive_ancestors(ShadowIncluding::No) + .filter_map(DomRoot::downcast::) + { + if element.hover_state() { + break; + } + + element.set_hover_state(true); + } + + self.fire_mouse_event( + client_point, + &new_target.upcast(), + FireMouseEventType::Over, + pressed_mouse_buttons, + ); + + // TODO: Fire mouseenter here. + } + } + + // Send mousemove event to topmost target, unless it's an iframe, in which case the + // compositor should have also sent an event to the inner document. self.fire_mouse_event( client_point, new_target.upcast(), @@ -1412,76 +1471,13 @@ impl Document { pressed_mouse_buttons, ); - // 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; + if target_has_changed { + // Store the current mouse over target for next frame. + prev_mouse_over_target.set(maybe_new_target.as_deref()); + + self.window + .reflow(ReflowGoal::Full, ReflowReason::MouseEvent); } - - 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::() - .is_ancestor_of(new_target.upcast::()), - _ => 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::() - .inclusive_ancestors(ShadowIncluding::No) - .filter_map(DomRoot::downcast::) - { - element.set_hover_state(false); - element.set_active_state(false); - } - } - - // Remove hover state to old target and its parents - self.fire_mouse_event( - client_point, - old_target.upcast(), - FireMouseEventType::Out, - pressed_mouse_buttons, - ); - - // 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::() - .inclusive_ancestors(ShadowIncluding::No) - .filter_map(DomRoot::downcast::) - { - if element.hover_state() { - break; - } - - element.set_hover_state(true); - } - - self.fire_mouse_event( - client_point, - &new_target.upcast(), - FireMouseEventType::Over, - pressed_mouse_buttons, - ); - - // TODO: Fire mouseenter here. - } - - // Store the current mouse over target for next frame. - prev_mouse_over_target.set(maybe_new_target.as_deref()); - - self.window - .reflow(ReflowGoal::Full, ReflowReason::MouseEvent); } #[allow(unsafe_code)] diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 13dc9c32fa3..1087e37a9fc 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -552,7 +552,7 @@ pub enum CompositorEvent { ), /// The mouse was moved over a point (or was moved out of the recognizable region). MouseMoveEvent( - Option>, + Point2D, Option, // Bitmask of MouseButton values representing the currently pressed buttons u16, From c235e1ee4337c97ea451d9bc93c6937851431073 Mon Sep 17 00:00:00 2001 From: Utsav Oza Date: Mon, 27 Jul 2020 19:39:29 +0530 Subject: [PATCH 2/5] Fire mouseenter and mouseleave events --- components/script/dom/document.rs | 87 ++++++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index cb4d9160cac..3decbc64b18 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -189,10 +189,13 @@ pub enum TouchEventResult { Forwarded, } +#[derive(Clone, Copy, PartialEq)] pub enum FireMouseEventType { Move, Over, Out, + Enter, + Leave, } impl FireMouseEventType { @@ -201,6 +204,8 @@ impl FireMouseEventType { &FireMouseEventType::Move => "mousemove", &FireMouseEventType::Over => "mouseover", &FireMouseEventType::Out => "mouseout", + &FireMouseEventType::Enter => "mouseenter", + &FireMouseEventType::Leave => "mouseleave", } } } @@ -1343,6 +1348,8 @@ impl Document { client_point: Point2D, target: &EventTarget, event_name: FireMouseEventType, + can_bubble: EventBubbles, + cancelable: EventCancelable, pressed_mouse_buttons: u16, ) { let client_x = client_point.x.to_i32().unwrap_or(0); @@ -1351,8 +1358,8 @@ impl Document { let mouse_event = MouseEvent::new( &self.window, DOMString::from(event_name.as_str()), - EventBubbles::Bubbles, - EventCancelable::Cancelable, + can_bubble, + cancelable, Some(&self.window), 0i32, client_x, @@ -1430,11 +1437,22 @@ impl Document { client_point, old_target.upcast(), FireMouseEventType::Out, + EventBubbles::Bubbles, + EventCancelable::Cancelable, pressed_mouse_buttons, ); - // TODO: Fire mouseleave here only if the old target is - // not an ancestor of the new target. + if !old_target_is_ancestor_of_new_target { + let event_target = DomRoot::from_ref(old_target.upcast::()); + let moving_into = Some(DomRoot::from_ref(new_target.upcast::())); + self.handle_mouse_enter_leave_event( + client_point, + FireMouseEventType::Leave, + moving_into, + event_target, + pressed_mouse_buttons, + ); + } } // Dispatch mouseover to new target - TODO: Redundant check? @@ -1453,12 +1471,24 @@ impl Document { self.fire_mouse_event( client_point, - &new_target.upcast(), + new_target.upcast(), FireMouseEventType::Over, + EventBubbles::Bubbles, + EventCancelable::Cancelable, pressed_mouse_buttons, ); - // TODO: Fire mouseenter here. + let moving_from = prev_mouse_over_target + .get() + .map(|old_target| DomRoot::from_ref(old_target.upcast::())); + let event_target = DomRoot::from_ref(new_target.upcast::()); + self.handle_mouse_enter_leave_event( + client_point, + FireMouseEventType::Enter, + moving_from, + event_target, + pressed_mouse_buttons, + ); } } @@ -1468,6 +1498,8 @@ impl Document { client_point, new_target.upcast(), FireMouseEventType::Move, + EventBubbles::Bubbles, + EventCancelable::Cancelable, pressed_mouse_buttons, ); @@ -1480,6 +1512,49 @@ impl Document { } } + fn handle_mouse_enter_leave_event( + &self, + client_point: Point2D, + event_type: FireMouseEventType, + related_target: Option>, + event_target: DomRoot, + pressed_mouse_buttons: u16, + ) { + assert!(matches!( + event_type, + FireMouseEventType::Enter | FireMouseEventType::Leave + )); + + let common_ancestor = related_target.as_ref().map_or_else( + || DomRoot::from_ref(&*event_target), + |related_target| event_target.common_ancestor(related_target, ShadowIncluding::No), + ); + + let mut targets = vec![]; + let mut current = Some(event_target); + while let Some(node) = current { + if node == common_ancestor { + break; + } + current = node.GetParentNode(); + targets.push(node); + } + if event_type == FireMouseEventType::Enter { + targets = targets.into_iter().rev().collect(); + } + + for target in targets { + self.fire_mouse_event( + client_point, + target.upcast(), + event_type, + EventBubbles::DoesNotBubble, + EventCancelable::NotCancelable, + pressed_mouse_buttons, + ); + } + } + #[allow(unsafe_code)] pub fn handle_wheel_event( &self, From da45522085363480009f7e12cce2c00a94ec931e Mon Sep 17 00:00:00 2001 From: Utsav Oza Date: Mon, 27 Jul 2020 22:52:02 +0530 Subject: [PATCH 3/5] Fix comments for handle_mouse_move_event method --- components/script/dom/document.rs | 89 +++++++++++++++---------------- 1 file changed, 42 insertions(+), 47 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 3decbc64b18..2ca8825808b 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1406,21 +1406,16 @@ impl Document { .map_or(true, |old_target| old_target != new_target); // Here we know the target has changed, so we must update the state, - // dispatch mouseout to the previous one, mouseover to the new one, + // dispatch mouseout to the previous one, mouseover to the new one. if target_has_changed { - // Dispatch mouseout to previous target + // Dispatch mouseout and mouseleave to previous target. if let Some(old_target) = prev_mouse_over_target.get() { - let old_target_is_ancestor_of_new_target = prev_mouse_over_target - .get() - .as_ref() - .map_or(false, |old_target| { - old_target - .upcast::() - .is_ancestor_of(new_target.upcast::()) - }); + let old_target_is_ancestor_of_new_target = old_target + .upcast::() + .is_ancestor_of(new_target.upcast::()); // 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. + // completely, since the node's hover state will be reset below. if !old_target_is_ancestor_of_new_target { for element in old_target .upcast::() @@ -1432,7 +1427,6 @@ impl Document { } } - // Remove hover state to old target and its parents self.fire_mouse_event( client_point, old_target.upcast(), @@ -1455,41 +1449,38 @@ impl Document { } } - // Dispatch mouseover to new target - TODO: Redundant check? - if let Some(new_target) = maybe_new_target.as_ref() { - for element in new_target - .upcast::() - .inclusive_ancestors(ShadowIncluding::No) - .filter_map(DomRoot::downcast::) - { - if element.hover_state() { - break; - } - - element.set_hover_state(true); + // Dispatch mouseover and mouseenter to new target. + for element in new_target + .upcast::() + .inclusive_ancestors(ShadowIncluding::No) + .filter_map(DomRoot::downcast::) + { + if element.hover_state() { + break; } - - self.fire_mouse_event( - client_point, - new_target.upcast(), - FireMouseEventType::Over, - EventBubbles::Bubbles, - EventCancelable::Cancelable, - pressed_mouse_buttons, - ); - - let moving_from = prev_mouse_over_target - .get() - .map(|old_target| DomRoot::from_ref(old_target.upcast::())); - let event_target = DomRoot::from_ref(new_target.upcast::()); - self.handle_mouse_enter_leave_event( - client_point, - FireMouseEventType::Enter, - moving_from, - event_target, - pressed_mouse_buttons, - ); + element.set_hover_state(true); } + + self.fire_mouse_event( + client_point, + new_target.upcast(), + FireMouseEventType::Over, + EventBubbles::Bubbles, + EventCancelable::Cancelable, + pressed_mouse_buttons, + ); + + let moving_from = prev_mouse_over_target + .get() + .map(|old_target| DomRoot::from_ref(old_target.upcast::())); + let event_target = DomRoot::from_ref(new_target.upcast::()); + self.handle_mouse_enter_leave_event( + client_point, + FireMouseEventType::Enter, + moving_from, + event_target, + pressed_mouse_buttons, + ); } // Send mousemove event to topmost target, unless it's an iframe, in which case the @@ -1503,10 +1494,9 @@ impl Document { pressed_mouse_buttons, ); + // If the target has changed then store the current mouse over target for next frame. if target_has_changed { - // Store the current mouse over target for next frame. prev_mouse_over_target.set(maybe_new_target.as_deref()); - self.window .reflow(ReflowGoal::Full, ReflowReason::MouseEvent); } @@ -1530,6 +1520,8 @@ impl Document { |related_target| event_target.common_ancestor(related_target, ShadowIncluding::No), ); + // We need to create a target chain in case the event target shares + // its boundaries with its ancestors. let mut targets = vec![]; let mut current = Some(event_target); while let Some(node) = current { @@ -1539,6 +1531,9 @@ impl Document { current = node.GetParentNode(); targets.push(node); } + + // The order for dispatching mouseenter events starts from the topmost + // common ancestor of the event target and the related target. if event_type == FireMouseEventType::Enter { targets = targets.into_iter().rev().collect(); } From 962e6205293b3466df173889cd56ba435cdaa233 Mon Sep 17 00:00:00 2001 From: Utsav Oza Date: Thu, 30 Jul 2020 16:31:36 +0530 Subject: [PATCH 4/5] Do not panic on navigating across documents --- components/script/dom/document.rs | 13 ++++++++----- components/script/dom/node.rs | 6 +++--- components/script/dom/range.rs | 1 + 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 2ca8825808b..fef552b326b 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -560,7 +560,8 @@ impl Document { let new_dirty_root = element .upcast::() - .common_ancestor(dirty_root.upcast(), ShadowIncluding::Yes); + .common_ancestor(dirty_root.upcast(), ShadowIncluding::Yes) + .expect("Couldn't find common ancestor"); let mut has_dirty_descendants = true; for ancestor in dirty_root @@ -1515,10 +1516,12 @@ impl Document { FireMouseEventType::Enter | FireMouseEventType::Leave )); - let common_ancestor = related_target.as_ref().map_or_else( - || DomRoot::from_ref(&*event_target), - |related_target| event_target.common_ancestor(related_target, ShadowIncluding::No), - ); + let common_ancestor = match related_target.as_ref() { + Some(related_target) => event_target + .common_ancestor(related_target, ShadowIncluding::No) + .unwrap_or_else(|| DomRoot::from_ref(&*event_target)), + None => DomRoot::from_ref(&*event_target), + }; // We need to create a target chain in case the event target shares // its boundaries with its ancestors. diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 9bfe91e3470..8883b5b33d7 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -708,16 +708,16 @@ impl Node { &self, other: &Node, shadow_including: ShadowIncluding, - ) -> DomRoot { + ) -> Option> { for ancestor in self.inclusive_ancestors(shadow_including) { if other .inclusive_ancestors(shadow_including) .any(|node| node == ancestor) { - return ancestor; + return Some(ancestor); } } - unreachable!(); + None } pub fn is_inclusive_ancestor_of(&self, parent: &Node) -> bool { diff --git a/components/script/dom/range.rs b/components/script/dom/range.rs index e9b3a2e34cb..b48e186bc3f 100644 --- a/components/script/dom/range.rs +++ b/components/script/dom/range.rs @@ -298,6 +298,7 @@ impl RangeMethods for Range { fn CommonAncestorContainer(&self) -> DomRoot { self.EndContainer() .common_ancestor(&self.StartContainer(), ShadowIncluding::No) + .expect("Couldn't find common ancestor container") } // https://dom.spec.whatwg.org/#dom-range-setstart From de20715f23706e01163c4843ccf113cbae6bf8e9 Mon Sep 17 00:00:00 2001 From: Utsav Oza Date: Thu, 30 Jul 2020 22:07:04 +0530 Subject: [PATCH 5/5] Add some manual tests for verifying mouse events order --- tests/html/test_mouse_events_order_1.html | 56 ++++++++++++++++++++ tests/html/test_mouse_events_order_2.html | 60 ++++++++++++++++++++++ tests/html/test_mouse_events_order_3.html | 62 +++++++++++++++++++++++ 3 files changed, 178 insertions(+) create mode 100644 tests/html/test_mouse_events_order_1.html create mode 100644 tests/html/test_mouse_events_order_2.html create mode 100644 tests/html/test_mouse_events_order_3.html diff --git a/tests/html/test_mouse_events_order_1.html b/tests/html/test_mouse_events_order_1.html new file mode 100644 index 00000000000..eee56954bea --- /dev/null +++ b/tests/html/test_mouse_events_order_1.html @@ -0,0 +1,56 @@ + + + + + Mouse Events Order 1 + + + + + +

Description: Tests the order of mouse events when a pointing device transitions from a + visible element A to another visible element B.

+ +
A
+
B
+ + + + + diff --git a/tests/html/test_mouse_events_order_2.html b/tests/html/test_mouse_events_order_2.html new file mode 100644 index 00000000000..45319c2f45e --- /dev/null +++ b/tests/html/test_mouse_events_order_2.html @@ -0,0 +1,60 @@ + + + + + Mouse Events Order 2 + + + + + +

Description: Tests the order of mouse events when a pointing device is moved into an element A, + and then into a nested element B and then back out again.

+ +
A +
B
+
+ + + + + diff --git a/tests/html/test_mouse_events_order_3.html b/tests/html/test_mouse_events_order_3.html new file mode 100644 index 00000000000..dd3ae1031a4 --- /dev/null +++ b/tests/html/test_mouse_events_order_3.html @@ -0,0 +1,62 @@ + + + + + Mouse Events Order 3 + + + + + +

Description: Tests the order of mouse events when a pointing device is moved into a visually + overlapped stack of elements having the same dimensions and absolute positions and then moved out again.

+ +
A +
B +
C
+
+
+ + + + +