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.
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.
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.
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.
(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 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?
(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.
(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 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.
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
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
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
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
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
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.
(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 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.
(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.
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
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
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
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
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
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
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
(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 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.
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
(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 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.
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
(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.
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 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 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 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.
2018-09-13 12:19 PDT, Sihui Liu
2018-09-14 00:12 PDT, Sihui Liu
2018-09-14 01:15 PDT, EWS Watchlist
2018-09-14 01:32 PDT, EWS Watchlist
2018-09-14 01:55 PDT, EWS Watchlist
2018-09-14 02:08 PDT, EWS Watchlist
2018-09-14 02:10 PDT, EWS Watchlist
2018-09-14 08:22 PDT, Sihui Liu
2018-09-14 09:26 PDT, EWS Watchlist
2018-09-14 09:55 PDT, EWS Watchlist
2018-09-14 10:03 PDT, EWS Watchlist
2018-09-14 10:33 PDT, Sihui Liu
2018-09-14 11:40 PDT, EWS Watchlist
2018-09-14 12:13 PDT, EWS Watchlist
2018-09-14 12:13 PDT, EWS Watchlist
2018-09-14 12:42 PDT, EWS Watchlist
2018-09-14 14:26 PDT, EWS Watchlist
2018-09-14 16:16 PDT, Sihui Liu
2018-09-14 16:17 PDT, Sihui Liu
2018-09-14 17:29 PDT, EWS Watchlist
2018-09-16 22:29 PDT, Sihui Liu
2018-09-16 23:58 PDT, EWS Watchlist
2018-09-17 13:47 PDT, Sihui Liu
2018-09-17 20:14 PDT, Sihui Liu
2018-09-20 14:35 PDT, Sihui Liu