Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't fire a mousemove event when a modifier key is pressed #14221

Merged
merged 1 commit into from May 24, 2023

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented May 23, 2023

f84b233

Don't fire a mousemove event when a modifier key is pressed
https://bugs.webkit.org/show_bug.cgi?id=16271
rdar://81287778

Reviewed by Wenson Hsieh.

Previously, we dispatched mousemove events whenever a modifier key was
pressed. This was used to call the UIClient delegate callback
mouseDidMoveOverElement, which downstream clients could use to modify
various behaviors based on whether or not modifier keys were pressed.

Unfortunately, the act of dispatching a mousemove event meant that
pressing modifier keys could cause unexpected mouse behavior. One notable
example is Slack. When using Slack through Minibrowser, it is possible
to experience spontaneous mouse selection when a cursor was originally
placed behind a completion list, since our hover state gets constantly
re-evaluated.

In this patch, we introduce a mechanism to avoid the need to fire a
mousemove event when modifier keys are pressed. This is achieved by
adding a new message from the UI process to the web process where we
request that a given mouse event's hit test be performed. With the hit
test result and user data associated with said mouse event, we can
simply call into the UIClient delegate callback mouseDidMoveOverElement,
avoiding the regular mouse event dispatch dance altogether.

* LayoutTests/fast/events/no-mousemove-on-modifier-key-press-expected.txt: Added.
* LayoutTests/fast/events/no-mousemove-on-modifier-key-press.html: Added.

Layout test that verifies pressing the modifier keys does not fire a
MouseMove event.

* Source/WebCore/page/Chrome.h:

Export (and make public) Chrome::getToolTip to access from the web
process.

* Source/WebCore/page/EventHandler.cpp:
(WebCore::EventHandler::getHitTypeForMouseMoveEvent):
(WebCore::EventHandler::getHitTestResultForMouseEvent):
(WebCore::EventHandler::handleMouseMoveEvent):

Introduce two functions getHitTypeForMouseMoveEvent and
getHitTestResultForMouseEvent. The latter strips out some common code
from handleMouseMoveEvent so that it can be called independently to
identify a hitType, whereas the former is a handy way to produce a
hitTestResult without bringing the MouseEventWithHitTestResults type
into web process land.

* Source/WebCore/page/EventHandler.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::dispatchMouseDidMoveOverElementAsynchronously):

Introduce a method that communicates the need for user data and a hit
test on a mouse event without actually dispatching said mouse event to
the web process. Once we round trip from the web process, it feeds the
returned data to the client delegate callback mouseDidMoveOverElement.

* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(WebKit::WebViewImpl::viewDidMoveToWindow):
(WebKit::WebViewImpl::fireMouseDidMoveOverElement):
(WebKit::WebViewImpl::postFakeMouseMovedEventForFlagsChangedEvent): Deleted.

Rename postFakeMouseMovedEventForFlagsChangedEvent to
fireMouseDidMoveOverElement to better represent our signaling intent.

fireMouseDidMoveOverElement then communicates with the web page to
receive user data and hit test results before passing that over to the
client delegate callback.

* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::performHitTestAndGetUserData):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPage.messages.in:

Introduce a new message that receives a mouse event from the UI process
and calls into the page's event handler and chrome clients to perform a
hit test on the received mouse event. This allows us to circumvent the
usual dance with a dispatched mouse event while returning the user data
and hit test result associated with a mouse event.

Canonical link: https://commits.webkit.org/264455@main

1fd5225

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 ❌ πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@aprotyas aprotyas self-assigned this May 23, 2023
@aprotyas aprotyas added the UI Events For bugs related to user interactions like keyboard, mouse, and touch events. label May 23, 2023
@pxlcoder
Copy link
Member

rdar://81287778
https://bugs.webkit.org/show_bug.cgi?id=16271

Bugzilla link should come before radar.

@aprotyas
Copy link
Member Author

rdar://81287778
https://bugs.webkit.org/show_bug.cgi?id=16271

Bugzilla link should come before radar.

Fixed! 73bdded

Source/WebCore/page/EventHandler.cpp Outdated Show resolved Hide resolved
Source/WebCore/page/EventHandler.cpp Outdated Show resolved Hide resolved
Source/WebCore/page/EventHandler.cpp Outdated Show resolved Hide resolved
Source/WebKit/UIProcess/WebPageProxy.cpp Outdated Show resolved Hide resolved
Source/WebKit/UIProcess/WebPageProxy.cpp Outdated Show resolved Hide resolved
@aprotyas aprotyas requested a review from whsieh May 23, 2023 03:43
Copy link
Member

@whsieh whsieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with Tim's comments too.

Source/WebKit/WebProcess/WebPage/WebPage.cpp Outdated Show resolved Hide resolved
Source/WebKit/WebProcess/WebPage/WebPage.cpp Outdated Show resolved Hide resolved
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 23, 2023
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label May 23, 2023
@hortont424 hortont424 added the merge-queue Applied to send a pull request to merge-queue label May 24, 2023
https://bugs.webkit.org/show_bug.cgi?id=16271
rdar://81287778

Reviewed by Wenson Hsieh.

Previously, we dispatched mousemove events whenever a modifier key was
pressed. This was used to call the UIClient delegate callback
mouseDidMoveOverElement, which downstream clients could use to modify
various behaviors based on whether or not modifier keys were pressed.

Unfortunately, the act of dispatching a mousemove event meant that
pressing modifier keys could cause unexpected mouse behavior. One notable
example is Slack. When using Slack through Minibrowser, it is possible
to experience spontaneous mouse selection when a cursor was originally
placed behind a completion list, since our hover state gets constantly
re-evaluated.

In this patch, we introduce a mechanism to avoid the need to fire a
mousemove event when modifier keys are pressed. This is achieved by
adding a new message from the UI process to the web process where we
request that a given mouse event's hit test be performed. With the hit
test result and user data associated with said mouse event, we can
simply call into the UIClient delegate callback mouseDidMoveOverElement,
avoiding the regular mouse event dispatch dance altogether.

* LayoutTests/fast/events/no-mousemove-on-modifier-key-press-expected.txt: Added.
* LayoutTests/fast/events/no-mousemove-on-modifier-key-press.html: Added.

Layout test that verifies pressing the modifier keys does not fire a
MouseMove event.

* Source/WebCore/page/Chrome.h:

Export (and make public) Chrome::getToolTip to access from the web
process.

* Source/WebCore/page/EventHandler.cpp:
(WebCore::EventHandler::getHitTypeForMouseMoveEvent):
(WebCore::EventHandler::getHitTestResultForMouseEvent):
(WebCore::EventHandler::handleMouseMoveEvent):

Introduce two functions getHitTypeForMouseMoveEvent and
getHitTestResultForMouseEvent. The latter strips out some common code
from handleMouseMoveEvent so that it can be called independently to
identify a hitType, whereas the former is a handy way to produce a
hitTestResult without bringing the MouseEventWithHitTestResults type
into web process land.

* Source/WebCore/page/EventHandler.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::dispatchMouseDidMoveOverElementAsynchronously):

Introduce a method that communicates the need for user data and a hit
test on a mouse event without actually dispatching said mouse event to
the web process. Once we round trip from the web process, it feeds the
returned data to the client delegate callback mouseDidMoveOverElement.

* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(WebKit::WebViewImpl::viewDidMoveToWindow):
(WebKit::WebViewImpl::fireMouseDidMoveOverElement):
(WebKit::WebViewImpl::postFakeMouseMovedEventForFlagsChangedEvent): Deleted.

Rename postFakeMouseMovedEventForFlagsChangedEvent to
fireMouseDidMoveOverElement to better represent our signaling intent.

fireMouseDidMoveOverElement then communicates with the web page to
receive user data and hit test results before passing that over to the
client delegate callback.

* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::performHitTestAndGetUserData):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPage.messages.in:

Introduce a new message that receives a mouse event from the UI process
and calls into the page's event handler and chrome clients to perform a
hit test on the received mouse event. This allows us to circumvent the
usual dance with a dispatched mouse event while returning the user data
and hit test result associated with a mouse event.

Canonical link: https://commits.webkit.org/264455@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264455@main (f84b233): https://commits.webkit.org/264455@main

Reviewed commits have been landed. Closing PR #14221 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit f84b233 into WebKit:main May 24, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 24, 2023
@aprotyas aprotyas deleted the eng/16271 branch May 24, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Events For bugs related to user interactions like keyboard, mouse, and touch events.
Projects
None yet
7 participants