Bug 116691 - [GTK] It should process MouseMoved events when the parent window is not active
Summary: [GTK] It should process MouseMoved events when the parent window is not active
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-23 14:30 PDT by Claudio Saavedra
Modified: 2017-07-06 06:35 PDT (History)
17 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-05-23 14:30:49 PDT
[WK2][GTK] It should process MouseMoved events when the parent window is not active
Comment 1 Claudio Saavedra 2013-05-23 14:33:47 PDT
Created attachment 202741 [details]
Patch
Comment 2 Martin Robinson 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.
Comment 3 Claudio Saavedra 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.
Comment 4 Claudio Saavedra 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?
Comment 5 Michael Catanzaro 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.
Comment 6 Claudio Saavedra 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.
Comment 7 Michael Catanzaro 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().
Comment 8 Jan 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?
Comment 9 Claudio Saavedra 2017-05-16 06:25:55 PDT
Created attachment 310258 [details]
Patch
Comment 10 Claudio Saavedra 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.
Comment 11 Michael Catanzaro 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.
Comment 12 Michael Catanzaro 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
Comment 13 Claudio Saavedra 2017-05-18 08:56:15 PDT
Created attachment 310511 [details]
Patch
Comment 14 Michael Catanzaro 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.
Comment 15 Claudio Saavedra 2017-05-19 01:10:47 PDT
Created attachment 310630 [details]
Patch
Comment 16 Claudio Saavedra 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.
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Claudio Saavedra 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?
Comment 20 Tim Horton 2017-05-21 08:52:13 PDT
No, you can just ignore it.
Comment 21 Michael Catanzaro 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.
Comment 22 Tim Horton 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.
Comment 23 Claudio Saavedra 2017-07-05 10:13:28 PDT
Created attachment 314611 [details]
Patch
Comment 24 Claudio Saavedra 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!
Comment 25 Michael Catanzaro 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?
Comment 26 Tim Horton 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.
Comment 27 Claudio Saavedra 2017-07-06 03:26:14 PDT
Created attachment 314716 [details]
Patch
Comment 28 Claudio Saavedra 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.
Comment 29 Claudio Saavedra 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!
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2017-07-06 06:35:08 PDT
All reviewed patches have been landed.  Closing bug.