Fix for "Mouseovers follow the cursor even when there's a find bar in the way"

Fix for "Mouseovers follow the cursor even when there's a find bar in the way"

Perform a hit test in the render widget host view to check if the cursor is
actually on top of a different view - such as the find bar. If it is and that
view has a method -[(BOOL)nonWebContentView] that returns YES, then ignore
the event in the render widget host.

Change FindBarView to have the aforementioned method that returns YES.

Patch by Alexei Svitkine <asvitkine@chromium.org>;

BUG=22191
TEST=Leave the content area by mousing over a visible find bar. Mouseovers
     from the content area should extinguish, and no masked by the find bar
     should be triggered.
R=mark@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79438

[Mark Mentovai, 2011-03-23 20:01:25]

How does this work with mouseExited and mouseEntered when the cursor enters and
leaves the content area by transiting something like the find bar?

In the past, we had bugs where we wouldn’t send a mouseExited event down into
the content when the mouse left the content area. This seems to just swallow
mouse events that happen in things like the find bar instead of sending them
into to the page. Do we want treat these areas that encroach on the content area
as not-content, and therefore send a mouseExited event into the content when the
mouse leaves the content area by transiting a find bar, and mouseEntered when it
comes in via the same path?

http://codereview.chromium.org/6676094/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6676094/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1440: 
Don’t start a function with a blank line.

[asvitkine_google, 2011-03-24 14:18:03]

>
> In the past, we had bugs where we wouldn’t send a mouseExited event down
> into
> the content when the mouse left the content area.


What kind of problems did that cause?

-Alexei

[Mark Mentovai, 2011-03-24 14:26:41]

?

UI elements in pages would remain highlighted as though the cursor remained over
them when it had in fact gone away.

[asvitkine_google, 2011-03-24 14:48:46]

So that does sound like it would be a problem with the FindBar as well.

Any idea on a good way to solve this?

Just detecting the first event where the hitTest returns a different view
and transforming that to a mouseexited and vice-versa for mouseentered?

This wouldn't handle the case where the mouse stays still and a FindBar is
brought up in front of the cursor without any mouse movement. (And the
FindBar is currently very decoupled from the render view host to be able to
send events its way...)

Do you have any suggestions? Should I try to solve this as part of this
patch, or might be better to do it incrementally (i.e. in another patch)
since this patch is already an improvement over the status quo.

-Alexei

On Thu, Mar 24, 2011 at 10:26 AM, <mark@chromium.org> wrote:

> ?
>
> UI elements in pages would remain highlighted as though the cursor remained
> over
> them when it had in fact gone away.
>
>
> http://codereview.chromium.org/6676094/
>

[Mark Mentovai, 2011-03-24 15:02:59]

Alexei Svitkine wrote:
> So that does sound like it would be a problem with the FindBar as well.
> Any idea on a good way to solve this?
> Just detecting the first event where the hitTest returns a different view
> and transforming that to a mouseexited and vice-versa for mouseentered?

That’s my first thought.

> This wouldn't handle the case where the mouse stays still and a FindBar is
> brought up in front of the cursor without any mouse movement. (And the
> FindBar is currently very decoupled from the render view host to be able to
> send events its way...)

I don’t think that this is quite as major a problem, although the
reasoning is a bit of a cop-out. In order to bring up the find bar, in
most cases, the user will have either navigated through the menus (so
the mouse would have left the content area and not re-entered it) or
used command-F on the keyboard. In the latter case, the act of
pressing command-F currently hides the cursor (although perhaps it
shouldn’t), so it’s not like there’s a visible cursor over
non-content. Once the cursor is moved, I’d expect your view hit test
(as you suggested above) to send the mouseExited event.

> Do you have any suggestions? Should I try to solve this as part of this
> patch, or might be better to do it incrementally (i.e. in another patch)
> since this patch is already an improvement over the status quo.

I think that depends on the specific behavior that your patch implements.

If the mouseExited event isn’t sent when the mouse enters the find
bar, but is sent when the mouse leaves the find bar without reentering
the content area, then there’s no regression and we can review your
patch as-is, but I’d like to see a follow-up patch to further improve
the behavior.

If there’s no mouseExited event at all when the mouse leaves the
content area via the find bar and subsequently leaves the find bar
without reentering the content area, then it’s a regression and I
wouldn’t accept this patch as-is without improvement.

[Alexei Svitkine (slow), 2011-03-24 17:49:22]

I've updated the patch based on your feedback so that it does keep track of
enters/exits.

-Alexei

On Thu, Mar 24, 2011 at 11:02 AM, Mark Mentovai <mark@chromium.org> wrote:

> Alexei Svitkine wrote:
> > So that does sound like it would be a problem with the FindBar as well.
> > Any idea on a good way to solve this?
> > Just detecting the first event where the hitTest returns a different view
> > and transforming that to a mouseexited and vice-versa for mouseentered?
>
> That’s my first thought.
>
> > This wouldn't handle the case where the mouse stays still and a FindBar
> is
> > brought up in front of the cursor without any mouse movement. (And the
> > FindBar is currently very decoupled from the render view host to be able
> to
> > send events its way...)
>
> I don’t think that this is quite as major a problem, although the
> reasoning is a bit of a cop-out. In order to bring up the find bar, in
> most cases, the user will have either navigated through the menus (so
> the mouse would have left the content area and not re-entered it) or
> used command-F on the keyboard. In the latter case, the act of
> pressing command-F currently hides the cursor (although perhaps it
> shouldn’t), so it’s not like there’s a visible cursor over
> non-content. Once the cursor is moved, I’d expect your view hit test
> (as you suggested above) to send the mouseExited event.
>
> > Do you have any suggestions? Should I try to solve this as part of this
> > patch, or might be better to do it incrementally (i.e. in another patch)
> > since this patch is already an improvement over the status quo.
>
> I think that depends on the specific behavior that your patch implements.
>
> If the mouseExited event isn’t sent when the mouse enters the find
> bar, but is sent when the mouse leaves the find bar without reentering
> the content area, then there’s no regression and we can review your
> patch as-is, but I’d like to see a follow-up patch to further improve
> the behavior.
>
> If there’s no mouseExited event at all when the mouse leaves the
> content area via the find bar and subsequently leaves the find bar
> without reentering the content area, then it’s a regression and I
> wouldn’t accept this patch as-is without improvement.
>

[Mark Mentovai, 2011-03-24 18:44:40]

LGTM

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1423:
mouseEventWasIgnored_ = NO;
Is NO really the right thing to initialize this to? I guess the find view can’t
be shown yet when the RWHVC is created.

You don’t need to initialize things to NO, NULL, 0, or nil. See
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Initi...
. You can get rid of lines 1420 and 1421, too.

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1441: // Use hitTest
to check whether we're the frontmost view for this mouse
s/we're/this is/

[Alexei Svitkine (slow), 2011-03-24 19:04:56]

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1423:
mouseEventWasIgnored_ = NO;
On 2011/03/24 18:44:40, Mark Mentovai wrote:
> Is NO really the right thing to initialize this to? I guess the find view
can’t
> be shown yet when the RWHVC is created.
> 
> You don’t need to initialize things to NO, NULL, 0, or nil. See
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Initi...
> . You can get rid of lines 1420 and 1421, too.

Done.

http://codereview.chromium.org/6676094/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1441: // Use hitTest
to check whether we're the frontmost view for this mouse
On 2011/03/24 18:44:40, Mark Mentovai wrote:
> s/we're/this is/

Done.

[Mark Mentovai, 2011-03-24 19:07:33]

Let’s give this a try. LGTM.

[Alexei Svitkine (slow), 2011-03-24 20:33:43]

Failed some tests on the try server. Going to investigate.

http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/19044

-Alexei

On Thu, Mar 24, 2011 at 3:07 PM, <mark@chromium.org> wrote:

> Let’s give this a try. LGTM.
>
>
> http://codereview.chromium.org/6676094/
>

[Alexei Svitkine (slow), 2011-03-25 14:57:54]

Looks like this approach is no-good.

An AcceleratedPluginView is the view that is returned by the hitTest when
viewing youtube, for example, and in that case we still need to send the
events to the render view host.

One thing I can do is traverse the superview hierarchy of the view returned
by the hitTest and check if I encounter a FindBarView specifically (e.g.
via isKindOfClass:[FindBarView class]). If I take this approach, changes
will be needed if we'll ever need to have other views that obscure the
render view - we could have a common superclass for all such views and do
the isKindOfClass: check for that class instead, then.

(Note: The superview hierarchy needs to be traversed because if you're over
the text field in the FindBarView, the hitTest will return the NSTextView
and not the FindBarView).

 Thoughts?

-Alexei

On Thu, Mar 24, 2011 at 4:33 PM, Alexei Svitkine <asvitkine@chromium.org>wrote:

> Failed some tests on the try server. Going to investigate.
>
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/19044
>
> -Alexei
>
>
> On Thu, Mar 24, 2011 at 3:07 PM, <mark@chromium.org> wrote:
>
>> Let’s give this a try. LGTM.
>>
>>
>> http://codereview.chromium.org/6676094/
>>
>
>

[Mark Mentovai, 2011-03-25 16:38:52]

Alexei Svitkine wrote:
> Looks like this approach is no-good.
> An AcceleratedPluginView is the view that is returned by the hitTest when
> viewing youtube, for example, and in that case we still need to send the
> events to the render view host.

Of course. Too bad.

> One thing I can do is traverse the superview hierarchy of the view returned
> by the hitTest and check if I encounter a FindBarView specifically (e.g.
> via isKindOfClass:[FindBarView class]). If I take this approach, changes
> will be needed if we'll ever need to have other views that obscure the
> render view - we could have a common superclass for all such views and do
> the isKindOfClass: check for that class instead, then.

I’d go with the common superclass. Use either -isKindOfClass:, or come
up with a custom selector, use -respondsToSelector:, and then call it
to see how to behave. I think I have a slight preference for the
selector-based approach. Something like -(BOOL)nonWebContentView?

> (Note: The superview hierarchy needs to be traversed because if you're over
> the text field in the FindBarView, the hitTest will return the NSTextView
> and not the FindBarView).

Hopefully this won’t become too cumbersome.

Worth a shot, though. Let’s see how the code turns out.

[Alexei Svitkine (slow), 2011-03-25 17:26:52]

Updated the CL based on your suggestions.

[Mark Mentovai, 2011-03-25 17:43:52]

LGTM

http://codereview.chromium.org/6676094/diff/8001/chrome/browser/ui/cocoa/find...
File chrome/browser/ui/cocoa/find_bar/find_bar_view.mm (right):

http://codereview.chromium.org/6676094/diff/8001/chrome/browser/ui/cocoa/find...
chrome/browser/ui/cocoa/find_bar/find_bar_view.mm:133: - (BOOL)nonWebContentView
{
Can you declare this in the @interface?

[Alexei Svitkine (slow), 2011-03-25 19:12:23]

Changes passed the try server tests.

This is my first CL for Chrome, so I don't have commit access.

Can you commit the changes?

Thanks.

http://codereview.chromium.org/6676094/diff/8001/chrome/browser/ui/cocoa/find...
File chrome/browser/ui/cocoa/find_bar/find_bar_view.mm (right):

http://codereview.chromium.org/6676094/diff/8001/chrome/browser/ui/cocoa/find...
chrome/browser/ui/cocoa/find_bar/find_bar_view.mm:133: - (BOOL)nonWebContentView
{
On 2011/03/25 17:43:52, Mark Mentovai wrote:
> Can you declare this in the @interface?

Done.

[Mark Mentovai, 2011-03-25 19:28:28]

Committed r79438. Nice work.

[Mark Mentovai, 2011-03-25 19:28:58]

Note that I slightly revised the checkin comment to match Chromium
specifications. I put the BUG= line at the end and added a TEST= line.