116691 – [GTK] It should process MouseMoved events when the parent window is not active

RESOLVED FIXED 116691

[GTK] It should process MouseMoved events when the parent window is not active

https://bugs.webkit.org/show_bug.cgi?id=116691

Summary [GTK] It should process MouseMoved events when the parent window is not active

Claudio Saavedra

Reported 2013-05-23 14:30:49 PDT

[WK2][GTK] It should process MouseMoved events when the parent window is not active

Attachments
Patch (2.84 KB, patch) 2013-05-23 14:33 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Patch (2.75 KB, patch) 2017-05-16 06:25 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Patch (5.29 KB, patch) 2017-05-18 08:56 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Patch (5.29 KB, patch) 2017-05-19 01:10 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews113 for mac-elcapitan (1.70 MB, application/zip) 2017-05-19 03:52 PDT, Build Bot	no flags	Details
Patch (6.34 KB, patch) 2017-07-05 10:13 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Patch (5.79 KB, patch) 2017-07-06 03:26 PDT, Claudio Saavedra	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Claudio Saavedra

Comment 1 2013-05-23 14:33:47 PDT

Created attachment 202741 [details] Patch

Martin Robinson

Comment 2 2013-05-23 14:47:08 PDT

Comment on attachment 202741 [details] Patch Does web content expect this though? I think it's pretty important that we behave like other web browsers if there's consistency.

Claudio Saavedra

Comment 3 2013-05-23 14:52:07 PDT

I can tell that at least Chrome and Firefox in Linux do behave like this and so does WK1 with the GTK+ port.

Claudio Saavedra

Comment 4 2013-05-27 14:06:01 PDT

I know this patch is probably not ready for landing as the way it is implemented -- adding #idefs in shared code -- is not very clean, but I welcome suggestions on how to actually do it. Same as to what's the rationale behind not passing mouse moved events in an inactive window. Is this the standard behavior for Mac applications?

Michael Catanzaro

Comment 5 2016-01-02 08:42:15 PST

Comment on attachment 202741 [details] Patch Does this fix some practical issue? (In reply to comment #4) > I know this patch is probably not ready for landing as the way it is > implemented -- adding #idefs in shared code -- is not very clean, but I > welcome suggestions on how to actually do it. This seems like an appropriate use of #ifdefs to me. Sometimes it's just the cleanest way to handle things. Since this patch is quite old, I'm going to remove the r? -- please reset it if you still want to land this.

Claudio Saavedra

Comment 6 2016-01-05 06:42:21 PST

(In reply to comment #5) > Comment on attachment 202741 [details] > Patch > > Does this fix some practical issue? If you hover a page in a window that is not active you'd expect the contents to be updated (in which ever way the web content developers designed it), not only when the page is active. Same with changing the cursor to a hand when hovering a link.

Michael Catanzaro

Comment 7 2016-01-05 07:25:14 PST

(In reply to comment #6) > If you hover a page in a window that is not active you'd expect the contents > to be updated (in which ever way the web content developers designed it), > not only when the page is active. Same with changing the cursor to a hand > when hovering a link. OK. I agree, we should do this. (In reply to comment #5) > This seems like an appropriate use of #ifdefs to me. Sometimes it's just the > cleanest way to handle things. Thinking about this some more, we could do it more cleanly... you could put the #ifdef inside a function shouldProcessEventsWhenInactive(), or have a platformShouldProcessEventsWhenInactive().

Jan

Comment 8 2016-05-19 17:45:23 PDT

So what is the current status of this? Still planed to change the behavior of webkitgtk? The suggested changes to the patch don't look like too much work (to my inexperienced eyes). Would it help if I sent in a patch? If so, where would the function shouldProcessEventsWhenInactive or platformShouldProcessEventsWhenInactive go?

Claudio Saavedra

Comment 9 2017-05-16 06:25:55 PDT

Created attachment 310258 [details] Patch

Claudio Saavedra

Comment 10 2017-05-16 06:27:30 PDT

Updating this to get it out of my way. > (In reply to comment #4) > > This seems like an appropriate use of #ifdefs to me. Sometimes it's just the > cleanest way to handle things. Looking at the rest of the #ifdefs in the file I think this is definitely a reasonable approach, so I just updated the patch and changelog a bit.

Michael Catanzaro

Comment 11 2017-05-16 10:14:56 PDT

(In reply to Michael Catanzaro from comment #7) > Thinking about this some more, we could do it more cleanly... you could put > the #ifdef inside a function shouldProcessEventsWhenInactive() This is what I'd do, just because that makes the rationale here more self-documenting, and makes it easier to see that this is a choice that different platforms get to make. I'm also not completely sure about using PLATFORM(GTK) here... it might make more sense to use !PLATFORM(COCOA), since I guess most other ports probably want this behavior too. Not sure. But I think it's OK.

Michael Catanzaro

Comment 12 2017-05-16 10:15:15 PDT

Comment on attachment 310258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310258&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2372 > +#elif #else

Claudio Saavedra

Comment 13 2017-05-18 08:56:15 PDT

Created attachment 310511 [details] Patch

Michael Catanzaro

Comment 14 2017-05-18 13:05:00 PDT

Comment on attachment 310511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310511&action=review OK, this looks fine to me. You'll need to ask a WK2 owner to provide the final review, though. > Source/WebKit2/ChangeLog:11 > + specially those targetting Linux, these events should be processed especially, targeting > Source/WebKit2/UIProcess/WebPageProxy.cpp:4886 > +#if PLATFORM(COCOA) Or maybe it should be #if PLATFORM(MAC), since this specifically relates to mouse events. > Source/WebKit2/UIProcess/WebPageProxy.cpp:4891 > + return true; I bet this will trigger an unreachable code warning on Mac, so put this in an #else case. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2318 > +#if PLATFORM(COCOA) Maybe #if PLATFORM(MAC)? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2319 > + // We need to do a full, normal hit test during this mouse event if the page is active or if a mouse I see you got a nice minor cleanup from moving this code, cool.

Claudio Saavedra

Comment 15 2017-05-19 01:10:47 PDT

Created attachment 310630 [details] Patch

Claudio Saavedra

Comment 16 2017-05-19 03:33:43 PDT

(In reply to Michael Catanzaro from comment #14) > OK, this looks fine to me. You'll need to ask a WK2 owner to provide the > final review, though. Adding a couple of owners as per Michael's suggestion based on suggest-reviewers output.

Build Bot

Comment 17 2017-05-19 03:52:10 PDT

Comment on attachment 310630 [details] Patch Attachment 310630 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3775637 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-buffered.html

Build Bot

Comment 18 2017-05-19 03:52:11 PDT

Created attachment 310646 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Claudio Saavedra

Comment 19 2017-05-21 05:53:39 PDT

(In reply to Build Bot from comment #17) > New failing tests: > imported/w3c/web-platform-tests/media-source/mediasource-buffered.html This looks like an unrelated test that has been flaky? Do I need to trigger the mac-debug ews again somehow?

Tim Horton

Comment 20 2017-05-21 08:52:13 PDT

No, you can just ignore it.

Michael Catanzaro

Comment 21 2017-07-02 09:04:50 PDT

Comment on attachment 310630 [details] Patch Owners have had plenty of time to object by now, so r=me. But it would be nice to wait a couple more days from this reminder before pushing, to give owners even more time.

Tim Horton

Comment 22 2017-07-02 09:47:33 PDT

Comment on attachment 310630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310630&action=review Either way, wk2r=me > Source/WebKit2/UIProcess/WebPageProxy.cpp:4889 > + return pageClient.isViewWindowActive(); Perhaps it would be better to keep the platform-specific code in e.g. PageClient, and add a PageClient::shouldSetCursor? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2318 > +#if PLATFORM(MAC) Maybe make these PLATFORM(COCOA)? I’m not sure if they’re used on iOS (I think the synthesized click stuff is deeper than this, but I’m not sure). Probably has no impact, but it’s less change.

Claudio Saavedra

Comment 23 2017-07-05 10:13:28 PDT

Created attachment 314611 [details] Patch

Claudio Saavedra

Comment 24 2017-07-05 10:15:14 PDT

Comment on attachment 314611 [details] Patch This should address Tim's comments. Please review again to make sure I got it right? Thanks!

Michael Catanzaro

Comment 25 2017-07-05 11:42:40 PDT

I like this better, but the cost is an extra virtual function call. Tim, do you want to review it again?

Tim Horton

Comment 26 2017-07-05 11:46:06 PDT

Comment on attachment 314611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314611&action=review > Source/WebKit2/ChangeLog:3 > + Extra space here. > Source/WebKit2/UIProcess/WebPageProxy.cpp:4856 > + if (m_pageClient.shouldSetCursor()) Now that I *see* it, is there any reason for this not just to be folded into the implementation of setCursor? That addresses Michael's (very minor) concern and leaves even less madness here.

Claudio Saavedra

Comment 27 2017-07-06 03:26:14 PDT

Created attachment 314716 [details] Patch

Claudio Saavedra

Comment 28 2017-07-06 03:27:00 PDT

Comment on attachment 314716 [details] Patch Since this already has r+ I'll wait for the EWS to complete before landing it.

Claudio Saavedra

Comment 29 2017-07-06 06:07:05 PDT

Comment on attachment 314716 [details] Patch All bots green, landing as is. Please let me know if I missed something after Tim's last comments. Thanks!

WebKit Commit Bot

Comment 30 2017-07-06 06:35:06 PDT

Comment on attachment 314716 [details] Patch Clearing flags on attachment: 314716 Committed r219195: <http://trac.webkit.org/changeset/219195>

WebKit Commit Bot

Comment 31 2017-07-06 06:35:08 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Claudio Saavedra

Reported

2013-05-23 14:30 PDT

Modified

2017-07-06 06:35 PDT History

CC List

17 users Show

URL

Keywords

Depends on

Blocks