[webkit-reviews] review granted: [Bug 187545] REGRESSION(r196265): WKWebView fires mouseover, mouseenter, and mouseleave events even when it's in a background window : [Attachment 349993] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 187545: REGRESSION(r196265): WKWebView fires mouseover, mouseenter, and
mouseleave events even when it's in a background window
https://bugs.webkit.org/show_bug.cgi?id=187545

Attachment 349993: Patch

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




--- Comment #72 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 349993
  --> https://bugs.webkit.org/attachment.cgi?id=349993
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.