Bug 187545 - REGRESSION(r196265): WKWebView fires mouseover, mouseenter, and mouseleave events even when it's in a background window
Summary: REGRESSION(r196265): WKWebView fires mouseover, mouseenter, and mouseleave ev...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Local Build
Hardware: Macintosh macOS 10.13
: P1 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar, Regression
Depends on: 153740
Blocks:
  Show dependency treegraph
 
Reported: 2018-07-10 19:24 PDT by opendarwin
Modified: 2018-09-20 15:05 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.53 KB, patch)
2018-09-13 12:19 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (3.42 KB, patch)
2018-09-14 00:12 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.43 MB, application/zip)
2018-09-14 01:15 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.11 MB, application/zip)
2018-09-14 01:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.02 MB, application/zip)
2018-09-14 01:55 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.26 MB, application/zip)
2018-09-14 02:08 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews205 for win-future (12.83 MB, application/zip)
2018-09-14 02:10 PDT, EWS Watchlist
no flags Details
Patch (4.86 KB, patch)
2018-09-14 08:22 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.31 MB, application/zip)
2018-09-14 09:26 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.82 MB, application/zip)
2018-09-14 09:55 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.02 MB, application/zip)
2018-09-14 10:03 PDT, EWS Watchlist
no flags Details
Patch (6.13 KB, patch)
2018-09-14 10:33 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.30 MB, application/zip)
2018-09-14 11:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.07 MB, application/zip)
2018-09-14 12:13 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.79 MB, application/zip)
2018-09-14 12:13 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (56.36 MB, application/zip)
2018-09-14 12:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (16.24 MB, application/zip)
2018-09-14 14:26 PDT, EWS Watchlist
no flags Details
Patch (6.94 KB, patch)
2018-09-14 16:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (6.94 KB, patch)
2018-09-14 16:17 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.31 MB, application/zip)
2018-09-14 17:29 PDT, EWS Watchlist
no flags Details
Patch (7.80 KB, patch)
2018-09-16 22:29 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.92 MB, application/zip)
2018-09-16 23:58 PDT, EWS Watchlist
no flags Details
Patch (7.82 KB, patch)
2018-09-17 13:47 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2018-09-17 20:14 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (8.26 KB, patch)
2018-09-20 14:35 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description opendarwin 2018-07-10 19:24:24 PDT
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.
Comment 1 Ryosuke Niwa 2018-07-16 20:14:21 PDT
Hm... I'm not seeing this on the latest macOS Mojave beta.
Comment 2 Ryosuke Niwa 2018-07-16 20:15:42 PDT
Not seeing this on Safari 11.1.1 / High Sierra either.
Comment 3 Ryosuke Niwa 2018-07-16 20:17:43 PDT
Could you tell us the exact version of Safari & macOS you're reproducing this issue with?
Comment 4 opendarwin 2018-07-16 21:14:48 PDT
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.
Comment 5 Ryosuke Niwa 2018-07-16 21:44:03 PDT
Hm... okay. Thanks for the information. We probably need sysdiagnose (sudo sysdiagnose) on your machine to diagnose this.
Comment 6 Alexey Proskuryakov 2018-07-17 09:22:53 PDT
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.
Comment 7 Leo Natan 2018-07-18 09:47:29 PDT
Reproduced this when "Show scroll bars" is set to "Always".
Comment 8 opendarwin 2018-07-18 09:51:52 PDT
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.
Comment 9 Radar WebKit Bug Importer 2018-07-19 14:54:36 PDT
<rdar://problem/42401575>
Comment 10 Sihui Liu 2018-09-13 12:19:06 PDT
Created attachment 349690 [details]
Patch
Comment 11 Chris Dumez 2018-09-13 12:26:49 PDT
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
Comment 12 Tim Horton 2018-09-13 13:06:07 PDT
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.
Comment 13 Wenson Hsieh 2018-09-13 13:17:57 PDT
(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
Comment 14 Simon Fraser (smfr) 2018-09-13 14:30:32 PDT
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?
Comment 15 Simon Fraser (smfr) 2018-09-13 14:31:19 PDT
Also why do always-on scrollbars trigger the bug? That might suggest a better way to fix it.
Comment 16 Sihui Liu 2018-09-13 17:39:29 PDT
(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.
Comment 17 Daniel Bates 2018-09-13 21:50:54 PDT
Comment on attachment 349690 [details]
Patch

This patch is wrong because it breaks macOS platform conventions for inactive windows.
Comment 18 Sihui Liu 2018-09-14 00:12:54 PDT
Created attachment 349743 [details]
Patch
Comment 19 Sihui Liu 2018-09-14 00:18:27 PDT
(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.
Comment 20 Sihui Liu 2018-09-14 00:20:13 PDT
Seems like a regression from https://bugs.webkit.org/show_bug.cgi?id=153740.
Comment 21 Ryosuke Niwa 2018-09-14 00:25:05 PDT
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.
Comment 22 EWS Watchlist 2018-09-14 01:15:49 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-09-14 01:15:51 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2018-09-14 01:32:00 PDT Comment hidden (obsolete)
Comment 25 EWS Watchlist 2018-09-14 01:32:02 PDT Comment hidden (obsolete)
Comment 26 EWS Watchlist 2018-09-14 01:55:13 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2018-09-14 01:55:15 PDT Comment hidden (obsolete)
Comment 28 EWS Watchlist 2018-09-14 02:08:22 PDT Comment hidden (obsolete)
Comment 29 EWS Watchlist 2018-09-14 02:08:24 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2018-09-14 02:10:31 PDT Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-09-14 02:10:42 PDT Comment hidden (obsolete)
Comment 32 Michael Catanzaro 2018-09-14 08:00:44 PDT
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.
Comment 33 Sihui Liu 2018-09-14 08:22:02 PDT
Created attachment 349762 [details]
Patch
Comment 34 Sihui Liu 2018-09-14 08:24:45 PDT
(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.
Comment 35 Simon Fraser (smfr) 2018-09-14 08:27:14 PDT
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.
Comment 36 Michael Catanzaro 2018-09-14 08:42:07 PDT
(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.
Comment 37 EWS Watchlist 2018-09-14 09:26:42 PDT Comment hidden (obsolete)
Comment 38 EWS Watchlist 2018-09-14 09:26:44 PDT Comment hidden (obsolete)
Comment 39 EWS Watchlist 2018-09-14 09:55:47 PDT Comment hidden (obsolete)
Comment 40 EWS Watchlist 2018-09-14 09:55:59 PDT Comment hidden (obsolete)
Comment 41 EWS Watchlist 2018-09-14 10:03:53 PDT Comment hidden (obsolete)
Comment 42 EWS Watchlist 2018-09-14 10:03:55 PDT Comment hidden (obsolete)
Comment 43 Sihui Liu 2018-09-14 10:33:57 PDT
Created attachment 349774 [details]
Patch
Comment 44 EWS Watchlist 2018-09-14 11:40:38 PDT Comment hidden (obsolete)
Comment 45 EWS Watchlist 2018-09-14 11:40:40 PDT Comment hidden (obsolete)
Comment 46 EWS Watchlist 2018-09-14 12:13:27 PDT
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
Comment 47 EWS Watchlist 2018-09-14 12:13:30 PDT Comment hidden (obsolete)
Comment 48 EWS Watchlist 2018-09-14 12:13:44 PDT Comment hidden (obsolete)
Comment 49 EWS Watchlist 2018-09-14 12:13:56 PDT Comment hidden (obsolete)
Comment 50 Chris Dumez 2018-09-14 12:36:47 PDT
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.
Comment 51 EWS Watchlist 2018-09-14 12:42:50 PDT
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
Comment 52 EWS Watchlist 2018-09-14 12:42:54 PDT
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
Comment 53 Simon Fraser (smfr) 2018-09-14 12:53:19 PDT
(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!
Comment 54 Chris Dumez 2018-09-14 13:06:47 PDT
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.
Comment 55 EWS Watchlist 2018-09-14 14:26:38 PDT Comment hidden (obsolete)
Comment 56 EWS Watchlist 2018-09-14 14:26:42 PDT Comment hidden (obsolete)
Comment 57 Sihui Liu 2018-09-14 16:11:26 PDT
(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.
Comment 58 Sihui Liu 2018-09-14 16:16:24 PDT
Created attachment 349824 [details]
Patch
Comment 59 Sihui Liu 2018-09-14 16:17:07 PDT
Created attachment 349825 [details]
Patch
Comment 60 Michael Catanzaro 2018-09-14 16:25:22 PDT
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.
Comment 61 EWS Watchlist 2018-09-14 17:29:52 PDT
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
Comment 62 EWS Watchlist 2018-09-14 17:29:54 PDT
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
Comment 63 Sihui Liu 2018-09-16 22:29:45 PDT
Created attachment 349876 [details]
Patch
Comment 64 Sihui Liu 2018-09-16 22:30:18 PDT
(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.
Comment 65 EWS Watchlist 2018-09-16 23:58:14 PDT
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
Comment 66 EWS Watchlist 2018-09-16 23:58:17 PDT
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
Comment 67 Simon Fraser (smfr) 2018-09-17 10:51:34 PDT
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()?
Comment 68 Sihui Liu 2018-09-17 13:47:12 PDT
Created attachment 349936 [details]
Patch
Comment 69 Simon Fraser (smfr) 2018-09-17 13:56:27 PDT
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;
Comment 70 Sihui Liu 2018-09-17 20:14:53 PDT
Created attachment 349993 [details]
Patch
Comment 71 Sihui Liu 2018-09-17 20:15:52 PDT
(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...
Comment 72 Ryosuke Niwa 2018-09-17 21:04:20 PDT
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.
Comment 73 Sihui Liu 2018-09-20 00:35:15 PDT
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.
Comment 74 Sihui Liu 2018-09-20 14:35:17 PDT
Created attachment 350266 [details]
Patch for landing
Comment 75 WebKit Commit Bot 2018-09-20 15:05:56 PDT
Comment on attachment 350266 [details]
Patch for landing

Clearing flags on attachment: 350266

Committed r236286: <https://trac.webkit.org/changeset/236286>
Comment 76 WebKit Commit Bot 2018-09-20 15:05:59 PDT
All reviewed patches have been landed.  Closing bug.