|
Description
Jeff Johnson
2018-07-10 19:24:24 PDT
Hm... I'm not seeing this on the latest macOS Mojave beta. Not seeing this on Safari 11.1.1 / High Sierra either. Could you tell us the exact version of Safari & macOS you're reproducing this issue with? 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. Hm... okay. Thanks for the information. We probably need sysdiagnose (sudo sysdiagnose) on your machine to diagnose this. 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. Reproduced this when "Show scroll bars" is set to "Always". 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. Created attachment 349690 [details]
Patch
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 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. (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? Also why do always-on scrollbars trigger the bug? That might suggest a better way to fix it. (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 on attachment 349690 [details]
Patch
This patch is wrong because it breaks macOS platform conventions for inactive windows.
Created attachment 349743 [details]
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. Seems like a regression from https://bugs.webkit.org/show_bug.cgi?id=153740. 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 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 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
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 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
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 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
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 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
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 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. Created attachment 349762 [details]
Patch
(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. 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 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
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 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
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 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 349774 [details]
Patch
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 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
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 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
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 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
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 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 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. 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 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. Created attachment 349824 [details]
Patch
Created attachment 349825 [details]
Patch
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 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 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
Created attachment 349876 [details]
Patch
(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 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 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()? Created attachment 349936 [details]
Patch
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; Created attachment 349993 [details]
Patch
(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 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. Created attachment 350266 [details]
Patch for landing
Comment on attachment 350266 [details] Patch for landing Clearing flags on attachment: 350266 Committed r236286: <https://trac.webkit.org/changeset/236286> All reviewed patches have been landed. Closing bug. |