Bug 187545 – REGRESSION(r196265): WKWebView fires mouseover, mouseenter, and mouseleave events even when it's in a background window

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Jeff Johnson]

Steps to reproduce:
1. Open Safari on macOS
2. Open https://github.com/WebKit/webkit
3. Hover mouse over table rows, notice that background color changes
4. Open TextEdit, making it the active app, leaving Safari window in the background
5. Move mouse out of TextEdit window and over Safari window

Expected results: Table row background color does not change on hover

Actual results: Table row background color does change on hover

Notes:

* These are JavaScript mouseover events. Disable JavaScript to see that the background color stops changing entirely.

* This behavior does not occur in Firefox or Google Chrome.

* When Safari is in the background, web sites should not be able to capture mouse events. This basically allows sites to spy on user activity. You may be moving your mouse across the screen for any reason and just accidentally cross the Safari window.

[Ryosuke Niwa]

Hm... I'm not seeing this on the latest macOS Mojave beta.

[Ryosuke Niwa]

Not seeing this on Safari 11.1.1 / High Sierra either.

[Ryosuke Niwa]

Could you tell us the exact version of Safari & macOS you're reproducing this issue with?

[Jeff Johnson]

I can 100% reproduce this on macOS 10.13.6 with Safari 11.1.2, Safari Technology Preview 60, and my own local build of WebKit. I can also reproduce on Mojave beta 4 just updated today.

I understand that you can't personally reproduce this for some reason, but it's disheartening to have a week old bug marked as resolved after I've taken unpaid time out of my life to file it.

[Ryosuke Niwa]

Hm... okay. Thanks for the information. We probably need sysdiagnose (sudo sysdiagnose) on your machine to diagnose this.

[Alexey Proskuryakov]

I recommend against attaching a sysdiagnose to bugzilla, as it contains information that I would personally not like to have publicly visible. Probably better to open an Apple bug at bugpreport.apple.com for that.

[Leo Natan]

Reproduced this when "Show scroll bars" is set to "Always".

[Jeff Johnson]

I've discovered, with the help of Leo and others, that an additional step to reproduce is to set System Preferences, General, Show scroll bars to "Always". The issue might not be seen if that's set to "Automatically based on mouse of trackpad" for example.

[Radar WebKit Bug Importer]

<rdar://problem/42401575>

[Sihui Liu]

Created attachment 349690 [details]
Patch

[Chris Dumez]

Comment on attachment 349690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349690&action=review

Is this not testable?

> Source/WebKit/ChangeLog:9
> +        Don't send mouse event to webpage if view's window is not acitive.

Typo: acitive

[Tim Horton]

Comment on attachment 349690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349690&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:1944
> +    if (!isViewWindowActive())
> +        return;

Do overlay scrollbars still glow when hovered in the background if you do this? This seems quite aggressive.

[Wenson Hsieh]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 349690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349690&action=review
> 
> Is this not testable?

I think this should be testable, either via API tests (by manipulating the NSWindow directly) or by layout tests (which would require adding a new hook/plumbing in UIScriptController to hide or show the view's NSWindow).

> 
> > Source/WebKit/ChangeLog:9
> > +        Don't send mouse event to webpage if view's window is not acitive.
> 
> Typo: acitive

[Simon Fraser (smfr)]

Comment on attachment 349690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349690&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1944
>> +        return;
> 
> Do overlay scrollbars still glow when hovered in the background if you do this? This seems quite aggressive.

Do file upload fields still respond to being dragged over in the background with this change?

[Simon Fraser (smfr)]

Also why do always-on scrollbars trigger the bug? That might suggest a better way to fix it.

[Sihui Liu]

(In reply to Simon Fraser (smfr) from comment #15)
> Also why do always-on scrollbars trigger the bug? That might suggest a
> better way to fix it.

When we change the "Show Scrollbars" to "Always" in System preference -> General, NSTrackingAreaOptions for the tracking area is set to NSTrackingActiveAlways (See PageClientImpl::recommendedScrollbarStyleDidChange and NSTrackingAreaOptions trackingAreaOptions()). 


Since the behavior we want here is, based on the bug description: when the page's window is not the active, we should not send any mouse event to web process/page, I made this patch.

[Daniel Bates]

Comment on attachment 349690 [details]
Patch

This patch is wrong because it breaks macOS platform conventions for inactive windows.

[Sihui Liu]

Created attachment 349743 [details]
Patch

[Sihui Liu]

(In reply to Daniel Bates from comment #17)
> Comment on attachment 349690 [details]
> Patch
> 
> This patch is wrong because it breaks macOS platform conventions for
> inactive windows.

Yes, I've been told we need some mouse events for inactive windows.

[Sihui Liu]

Seems like a regression from https://bugs.webkit.org/show_bug.cgi?id=153740.

[Ryosuke Niwa]

Comment on attachment 349743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349743&action=review

> Source/WebCore/ChangeLog:9
> +        We should only update the scrollbar for for mouse event if the window is not active.

Please mention that this is a regression from r196265.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:12
> +	var target = document.getElementById("target");
> +	target.addEventListener('mouseenter',() => log("Mouse enters target."));
> +	target.addEventListener('mouseleave',() => log("Mouse leaves target."));
> +	document.body.addEventListener('mousemove',() => log("Mouse moves."));

Please explicitly check that these events didn't fire after eventSender
instead of relying on the absence of these logs to mean the test had passed.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:19
> +});

Include js-test.js as done in other tests, and then do something like:
target.addEventListener('mouseenter',() => mouseenterCount++);
shouldBe('mouseenterCount', '0');
Note that mouseenterCount needs to be declared in the global scope.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:23
> +<body id=body>

Please add an instruction on how to run this test manually in the browser.
This will help us verify that the bug is fixed without having to run it under WebKitTestRunner/DumpRenderTree.

[EWS Watchlist]

Comment on attachment 349743 [details]
Patch

Attachment 349743 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9211557

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349748 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349743 [details]
Patch

Attachment 349743 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9211643

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349750 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349743 [details]
Patch

Attachment 349743 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9211674

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349752 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349743 [details]
Patch

Attachment 349743 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9211700

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349753 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 349743 [details]
Patch

Attachment 349743 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9211814

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349754 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

Comment on attachment 349743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349743&action=review

Please also skip the test in LayoutTests/platform/gtk/TestExpectations, at the bottom of section 2 (EXPECTED FAILURES)

> Source/WebCore/page/EventHandler.cpp:1969
> +#if PLATFORM(GTK)
>              updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);
> +#endif

Sorry about this regression.

[Sihui Liu]

Created attachment 349762 [details]
Patch

[Sihui Liu]

(In reply to Ryosuke Niwa from comment #21)
> Comment on attachment 349743 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349743&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        We should only update the scrollbar for for mouse event if the window is not active.
> 
> Please mention that this is a regression from r196265.
> 
Changelog modified.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:12
> > +	var target = document.getElementById("target");
> > +	target.addEventListener('mouseenter',() => log("Mouse enters target."));
> > +	target.addEventListener('mouseleave',() => log("Mouse leaves target."));
> > +	document.body.addEventListener('mousemove',() => log("Mouse moves."));
> 
> Please explicitly check that these events didn't fire after eventSender
> instead of relying on the absence of these logs to mean the test had passed.
> 
> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:19
> > +});
> 
> Include js-test.js as done in other tests, and then do something like:
> target.addEventListener('mouseenter',() => mouseenterCount++);
> shouldBe('mouseenterCount', '0');
> Note that mouseenterCount needs to be declared in the global scope.
> 
Done.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:23
> > +<body id=body>
> 
> Please add an instruction on how to run this test manually in the browser.
> This will help us verify that the bug is fixed without having to run it
> under WebKitTestRunner/DumpRenderTree.
Done.

[Simon Fraser (smfr)]

Comment on attachment 349762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349762&action=review

> Source/WebCore/page/EventHandler.cpp:1969
> +#if PLATFORM(GTK)
>              updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);
> +#endif

This #ifdef is very mysterious out of context. Why is GTK special here? It's much better to write self-documenting code and avoid platform #ifdefs.

[Michael Catanzaro]

(In reply to Simon Fraser (smfr) from comment #35) 
> This #ifdef is very mysterious out of context. Why is GTK special here? It's
> much better to write self-documenting code and avoid platform #ifdefs.

The expected behavior is different. We made this change to get mouse events in background windows, since that's the expected behavior for GTK apps. Evidently it's not expected for macOS.

To make it self-documenting, we could add a function named something like shouldSendPointerCrossingEventsToBackgroundWindows() and put the #if there.

[EWS Watchlist]

Comment on attachment 349762 [details]
Patch

Attachment 349762 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9216545

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349765 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349762 [details]
Patch

Attachment 349762 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9216804

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349768 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 349762 [details]
Patch

Attachment 349762 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9216610

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349770 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Sihui Liu]

Created attachment 349774 [details]
Patch

[EWS Watchlist]

Comment on attachment 349774 [details]
Patch

Attachment 349774 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9218105

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349786 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349774 [details]
Patch

Attachment 349774 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9218147

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349788 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349774 [details]
Patch

Attachment 349774 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9218367

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349789 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Chris Dumez]

Comment on attachment 349774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349774&action=review

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:9
> +

jsTestIsAsync = true;

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:33
> +        	testRunner.notifyDone();

finishJSTest();

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:44
> +        testRunner.waitUntilDone();

Not needed.

[EWS Watchlist]

Comment on attachment 349774 [details]
Patch

Attachment 349774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9218332

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349790 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Simon Fraser (smfr)]

(In reply to Michael Catanzaro from comment #36)
> (In reply to Simon Fraser (smfr) from comment #35) 
> > This #ifdef is very mysterious out of context. Why is GTK special here? It's
> > much better to write self-documenting code and avoid platform #ifdefs.
> 
> The expected behavior is different. We made this change to get mouse events
> in background windows, since that's the expected behavior for GTK apps.
> Evidently it's not expected for macOS.
> 
> To make it self-documenting, we could add a function named something like
> shouldSendPointerCrossingEventsToBackgroundWindows() and put the #if there.

Yes!

[Chris Dumez]

Comment on attachment 349774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349774&action=review

> Source/WebCore/page/EventHandler.cpp:2014
> +    updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);

mouseEvent / platformMouseEvent are undefined here.

> LayoutTests/fast/events/inactive_window_no_mouse_event-expected.txt:6
> +PASS mousemoveDetected is false

Having PASS lines after TEST COMPLETE is an indication that the test is not properly written and will likely be flaky.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:6
> +function log(message) {

Not needed since you're using js-test. You can just use debug().

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:14
> +document.addEventListener("DOMContentLoaded", function(event) {

We usually use onload = (event) => { };
Or do you really need  DOMContentLoaded here for some reason?

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:18
> +		log("Mouse enters target.");

debug().

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:19
> +		mouseenterCount ++;

No space before ++.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:22
> +		log("Mouse leaves target.");

debug().

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:23
> +		mouseleaveCount ++;

No space before ++.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:32
> +		if (window.testRunner) {

No curly brackets for one liners.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:37
> +	window.internals.setPageIsFocusedAndActive(false);

Should check if window.internals exists.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:39
> +	eventSender.mouseMoveTo(1, 1);

Should check if window.eventSender exists.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:43
> +	if (window.testRunner) {

No curly brackets for one liners.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:48
> +   		window.internals.setPageIsFocusedAndActive(true);

Should check for window.internals first.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:50
> +	}, 200);

Why 200?

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:54
> +<p>This tests what happens when the window is inactive. To manually run the test, open the page in Safari. Open another app like TextEdit which makes Safari window inactive. Move mouse to the red square on the page, you should see no new statements about mouse event.</p>

When using js-test, we normally use description("xxxxx") in script rather than <p>xxxxx</p>

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:57
> +<pre id="console"></pre>

Not needed. js-test takes care of this for you.

[EWS Watchlist]

Comment on attachment 349774 [details]
Patch

Attachment 349774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9219047

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349805 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Sihui Liu]

(In reply to Chris Dumez from comment #54)
> Comment on attachment 349774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349774&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2014
> > +    updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);
> 
> mouseEvent / platformMouseEvent are undefined here.
> 
Yes, I wrongly uploaded an older patch that didn't build...

> > LayoutTests/fast/events/inactive_window_no_mouse_event-expected.txt:6
> > +PASS mousemoveDetected is false
> 
> Having PASS lines after TEST COMPLETE is an indication that the test is not
> properly written and will likely be flaky.
> 
Will use jsTestIsAsync instead.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:6
> > +function log(message) {
> 
> Not needed since you're using js-test. You can just use debug().
> 
Will do.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:14
> > +document.addEventListener("DOMContentLoaded", function(event) {
> 
> We usually use onload = (event) => { };
> Or do you really need  DOMContentLoaded here for some reason?
> 
> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:18
> > +		log("Mouse enters target.");
> 
> debug().
> 
Take your advice, but using debug() seems to make manual test hard?


> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:39
> > +	eventSender.mouseMoveTo(1, 1);
> 
> Should check if window.eventSender exists.
> 
Okay.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:48
> > +   		window.internals.setPageIsFocusedAndActive(true);
> 
> Should check for window.internals first.
> 
Okay.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:50
> > +	}, 200);
> 
> Why 200?
> 
Just try to wait a short time to make sure all the mouse events handled before resetting the page state.

> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:54
> > +<p>This tests what happens when the window is inactive. To manually run the test, open the page in Safari. Open another app like TextEdit which makes Safari window inactive. Move mouse to the red square on the page, you should see no new statements about mouse event.</p>
> 
> When using js-test, we normally use description("xxxxx") in script rather
> than <p>xxxxx</p>
> 
> > LayoutTests/fast/events/inactive_window_no_mouse_event.html:57
> > +<pre id="console"></pre>
> 
> Not needed. js-test takes care of this for you.
I see.

[Sihui Liu]

Created attachment 349824 [details]
Patch

[Sihui Liu]

Created attachment 349825 [details]
Patch

[Michael Catanzaro]

Comment on attachment 349825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349825&action=review

> Source/WebCore/page/EventHandler.cpp:2010
> +void EventHandler::sendPointerCrossingEventsIfNeeded(MouseEventWithHitTestResults& mouseEvent, const PlatformMouseEvent& platformMouseEvent, bool fireMouseOverOut)

Since it only sends at most one event, it should be named sendPointerCrossingEventIfNeeded (Event rather than Events).

Also, the fireMouseOverOut parameter is not needed since it's always true.

[EWS Watchlist]

Comment on attachment 349825 [details]
Patch

Attachment 349825 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9221977

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349836 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Sihui Liu]

Created attachment 349876 [details]
Patch

[Sihui Liu]

(In reply to Michael Catanzaro from comment #60)
> Comment on attachment 349825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349825&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2010
> > +void EventHandler::sendPointerCrossingEventsIfNeeded(MouseEventWithHitTestResults& mouseEvent, const PlatformMouseEvent& platformMouseEvent, bool fireMouseOverOut)
> 
> Since it only sends at most one event, it should be named
> sendPointerCrossingEventIfNeeded (Event rather than Events).
> 
> Also, the fireMouseOverOut parameter is not needed since it's always true.

Fixed.

[EWS Watchlist]

Comment on attachment 349876 [details]
Patch

Attachment 349876 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9239960

New failing tests:
fast/events/inactive_window_no_mouse_event.html

[EWS Watchlist]

Created attachment 349878 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Comment on attachment 349876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349876&action=review

> Source/WebCore/page/EventHandler.h:496
> +    void sendPointerCrossingEventIfNeeded(MouseEventWithHitTestResults&, const PlatformMouseEvent&);

I don't think pointerCrossingEvent is a good name; I can't tell what this means. Maybe shouldSendMouseEventsToInactiveWindows()?

[Sihui Liu]

Created attachment 349936 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349936&action=review

> Source/WebCore/page/EventHandler.cpp:1967
> -            updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);
> +            shouldSendMouseEventsToInactiveWindows(mouseEvent, platformMouseEvent);

This should be:

if (shouldSendMouseEventsToInactiveWindows())
    updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);

with:

void shouldSendMouseEventsToInactiveWindows() const;

[Sihui Liu]

Created attachment 349993 [details]
Patch

[Sihui Liu]

(In reply to Simon Fraser (smfr) from comment #69)
> Comment on attachment 349936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349936&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:1967
> > -            updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent, true);
> > +            shouldSendMouseEventsToInactiveWindows(mouseEvent, platformMouseEvent);
> 
> This should be:
> 
> if (shouldSendMouseEventsToInactiveWindows())
>     updateMouseEventTargetNode(mouseEvent.targetNode(), platformMouseEvent,
> true);
> 
> with:
> 
> void shouldSendMouseEventsToInactiveWindows() const;

Sorry I misunderstood your comment...

[Ryosuke Niwa]

Comment on attachment 349993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349993&action=review

> LayoutTests/ChangeLog:11
> +        * fast/events/inactive_window_no_mouse_event.html: Added.

Please use - instead of _ in the test name.

> LayoutTests/TestExpectations:407
> +# This test only works for mac-wk2, other platforms have different behavior or not fully support EventSender.
> +fast/events/inactive_window_no_mouse_event.html [ Skip ]

Why doesn't this work in WK1? We shouldn't be sending mouse events in WK1 on macOS either.
I think a better approach is to skip this test in GTK as well as iOS where mouse pointer isn't supported.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:11
> +description("This test verifies no mouseenter, mouseleave or mousemove event sent to web page when window is inactive.");

Please add <br> and then a description as to how to run this manually in a browser.
e.g. "To manually run the test, make the window inactive then hover over the red box.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:20
> +	debug("Mouse enters target.");

You're using tab characters here. Use 4 spaces instead.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:43
> +	eventSender.mouseMoveTo(1, 1);
> +	eventSender.mouseMoveTo(300, 300);
> +	eventSender.mouseMoveTo(1, 1);

Instead of absolutely positioning elements using left/top.
You can simply query the location of the element via target.offsetTop / offsetLeft.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:48
> +		window.internals.setPageIsFocusedAndActive(true);

Nit: indentation is wrong.

> LayoutTests/fast/events/inactive_window_no_mouse_event.html:51
> +		eventSender.mouseMoveTo(0, 0);

Ditto.

[Sihui Liu]

Comment on attachment 349993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349993&action=review

>> LayoutTests/ChangeLog:11
>> +        * fast/events/inactive_window_no_mouse_event.html: Added.
> 
> Please use - instead of _ in the test name.

Done.

>> LayoutTests/TestExpectations:407
>> +fast/events/inactive_window_no_mouse_event.html [ Skip ]
> 
> Why doesn't this work in WK1? We shouldn't be sending mouse events in WK1 on macOS either.
> I think a better approach is to skip this test in GTK as well as iOS where mouse pointer isn't supported.

window.internals.setPageIsFocusedAndActive doesn't work in WK1. WK1 checks if it is active window by calling [NSWindow isKeyWindow] in (void)_updateMouseoverWithEvent:(NSEvent *)event.

>> LayoutTests/fast/events/inactive_window_no_mouse_event.html:11
>> +description("This test verifies no mouseenter, mouseleave or mousemove event sent to web page when window is inactive.");
> 
> Please add <br> and then a description as to how to run this manually in a browser.
> e.g. "To manually run the test, make the window inactive then hover over the red box.

Okay.

>> LayoutTests/fast/events/inactive_window_no_mouse_event.html:20
>> +	debug("Mouse enters target.");
> 
> You're using tab characters here. Use 4 spaces instead.

Okay.

>> LayoutTests/fast/events/inactive_window_no_mouse_event.html:43
>> +	eventSender.mouseMoveTo(1, 1);
> 
> Instead of absolutely positioning elements using left/top.
> You can simply query the location of the element via target.offsetTop / offsetLeft.

Got it.

>> LayoutTests/fast/events/inactive_window_no_mouse_event.html:48
>> +		window.internals.setPageIsFocusedAndActive(true);
> 
> Nit: indentation is wrong.

Okay.

>> LayoutTests/fast/events/inactive_window_no_mouse_event.html:51
>> +		eventSender.mouseMoveTo(0, 0);
> 
> Ditto.

Okay.

[Sihui Liu]

Created attachment 350266 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350266 [details]
Patch for landing

Clearing flags on attachment: 350266

Committed r236286: <https://trac.webkit.org/changeset/236286>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.