MacViews: Fix sending mouse exit event and releasing capture on D&D.

MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792
Committed: https://crrev.com/48134b46f0a22980774d9b9cb074e04b4971ada5
Cr-Commit-Position: refs/heads/master@{#420925}

[snake, 2016-09-14 11:58:31]

Description was changed from

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

BUG=646792
==========

to

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

BUG=646792
==========

[snake, 2016-09-15 18:27:47]

art-snake@yandex-team.ru changed reviewers:
+ dcheng@chromium.org, tapted@chromium.org, tfarina@chromium.org

[tapted, 2016-09-19 00:31:01]

tapted@chromium.org changed reviewers:
- tfarina@chromium.org

[tapted, 2016-09-19 00:31:01]

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
File ui/base/cocoa/base_view.h (left):

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#o...
ui/base/cocoa/base_view.h:31: base::scoped_nsobject<NSEvent> pendingExitEvent_;
spqchan is right - we can't change BaseView -- too much other stuff relies on
it, and it's unlikely there is good test coverage for the UI interactions.

Can you explain why it's necessary to remove the `pendingExitEvent_` logic in
order to fix the bug?

Then, if pendingExitEvent_ really is useless for any situation involving
BridgedContentView, then it can be disabled via overriding. E.g.

 - Add a method `handleMouseEvent` which switches on [event type] and does the
updates of dragging_ and pendingExitEvent_ all in the one method
 - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
instead of mouseEvent
 - override handleMouseEvent in BridgedContentView to just call mouseEvent so
the logic is skipped.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:308: -
(void)resendCapturedMouseEvent:(NSEvent*)theEvent {
I think this method has no effect in combination with your deletes in
base_view.mm, since the new methods now all just call mouseEvent. Perhaps I'm
missing something.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
Can you explain this change? Does it correspond to something that's done in Aura
that we forgot to add?

views::Widget calls this in Widget::OnMouseEvent() when it gets
ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop() as
well.

[snake, 2016-09-19 13:22:53]

On 2016/09/19 00:31:01, tapted wrote:
> https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
> File ui/base/cocoa/base_view.h (left):
> 
>
https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#o...
> ui/base/cocoa/base_view.h:31: base::scoped_nsobject<NSEvent>
pendingExitEvent_;
> spqchan is right - we can't change BaseView -- too much other stuff relies on
> it, and it's unlikely there is good test coverage for the UI interactions.
> 

The original change (http://codereview.chromium.org/1774014 ) was fixing the
problem with NPAPI-plugins, which should be long gone at this moment. And I was
unable to find other problems in a couple weeks' of testing.


> Can you explain why it's necessary to remove the `pendingExitEvent_` logic in
> order to fix the bug?

In some cases, we do not receive MouseUp event (when initiating a drag from the
bookmark bar, or showing menu on mouseDown: for example.)
As result, the MouseExit event are always ignored, until we click (and release
it) in that same BaseView again.

> 
> Then, if pendingExitEvent_ really is useless for any situation involving
> BridgedContentView, then it can be disabled via overriding. E.g.
> 
>  - Add a method `handleMouseEvent` which switches on [event type] and does the
> updates of dragging_ and pendingExitEvent_ all in the one method
>  - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
> instead of mouseEvent
>  - override handleMouseEvent in BridgedContentView to just call mouseEvent so
> the logic is skipped.

We already tried this approach internally, and I think the current proposed
solution is the better one, because it's more explicit this way (and the
original CL really seems to be outdated).

> 
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
> File ui/views/cocoa/bridged_content_view.mm (right):
> 
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
> ui/views/cocoa/bridged_content_view.mm:308: -
> (void)resendCapturedMouseEvent:(NSEvent*)theEvent {
> I think this method has no effect in combination with your deletes in
> base_view.mm, since the new methods now all just call mouseEvent. Perhaps I'm
> missing something.
> 

This was from my first attemt on fixing this issue. Yes, with the current
proposed fix it's not strictly necessary, but if someone will modify the
BaseView and change one of the specific mouse events, then it's not guaranteed
that the BridgedContentView will do the right thing, and this could lead to a
potential inconsistency.

Another option would be to create a separate BaseView specifically for MacViews
that would do less mouse event hackery.

>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
> File ui/views/cocoa/drag_drop_client_mac.mm (right):
> 
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
> ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
> Can you explain this change? Does it correspond to something that's done in
Aura
> that we forgot to add?
> 
> views::Widget calls this in Widget::OnMouseEvent() when it gets
> ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop() as
> well.

As I researched, when we start D&D under Aura on Windows the MouseDown event
handler calls NativeWidgetAura::SetCapture().
But NativeWidgetAura::ReleaseCapture() is never called after that.

The Windows OS releases the capture for native window automatically in this
case. In case Mac OS, it is not happening, and we should call ReleaseCapture().

[tapted, 2016-09-20 07:26:41]

can you move these threads back into context in the code? you can do this by
clicking on the comment that appears inline in the diff and replying on each
thread there, rather than to the email as a whole.

On 2016/09/19 13:22:53, snake wrote:
> On 2016/09/19 00:31:01, tapted wrote:
> > https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
> > File ui/base/cocoa/base_view.h (left):
> > 
> >
>
https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#o...
> > ui/base/cocoa/base_view.h:31: base::scoped_nsobject<NSEvent>
> pendingExitEvent_;
> > spqchan is right - we can't change BaseView -- too much other stuff relies
on
> > it, and it's unlikely there is good test coverage for the UI interactions.
> > 
> 
> The original change (http://codereview.chromium.org/1774014 ) was fixing the
> problem with NPAPI-plugins, which should be long gone at this moment. And I
was
> unable to find other problems in a couple weeks' of testing.

The CL description says "likely also fixes similar bugs in HTML" and the bug
doesn't seem NPAPI specific - just "plugin" specific, which may include PPAPI
plugins like Flash and NaCl. Did you try the repro case on
http://crbug.com/33100?

> 
> 
> > Can you explain why it's necessary to remove the `pendingExitEvent_` logic
in
> > order to fix the bug?
> 
> In some cases, we do not receive MouseUp event (when initiating a drag from
the
> bookmark bar, or showing menu on mouseDown: for example.)
> As result, the MouseExit event are always ignored, until we click (and release
> it) in that same BaseView again.

What happens to the event? (Why isn't it delivered on the mouseUp in the way the
code in BaseView suggests it should be?)


> 
> > 
> > Then, if pendingExitEvent_ really is useless for any situation involving
> > BridgedContentView, then it can be disabled via overriding. E.g.
> > 
> >  - Add a method `handleMouseEvent` which switches on [event type] and does
the
> > updates of dragging_ and pendingExitEvent_ all in the one method
> >  - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
> > instead of mouseEvent
> >  - override handleMouseEvent in BridgedContentView to just call mouseEvent
so
> > the logic is skipped.
> 
> We already tried this approach internally, and I think the current proposed
> solution is the better one, because it's more explicit this way (and the
> original CL really seems to be outdated).

I agree it would be nice to remove this extra stuff from BaseView. However, I
don't know if it's worth the risk. If you can address the first comment, then we
should put up a separate CL that _just_ makes the base_view.mm change and land
that first - then we can bisect any regressions down to that if necessary.

> 
> > 
> >
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
> > File ui/views/cocoa/bridged_content_view.mm (right):
> > 
> >
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
> > ui/views/cocoa/bridged_content_view.mm:308: -
> > (void)resendCapturedMouseEvent:(NSEvent*)theEvent {
> > I think this method has no effect in combination with your deletes in
> > base_view.mm, since the new methods now all just call mouseEvent. Perhaps
I'm
> > missing something.
> > 
> 
> This was from my first attemt on fixing this issue. Yes, with the current
> proposed fix it's not strictly necessary, but if someone will modify the
> BaseView and change one of the specific mouse events, then it's not guaranteed
> that the BridgedContentView will do the right thing, and this could lead to a
> potential inconsistency.

We typically don't add unnecessary code until we're sure it's needed. I think
going forward base_view.mm isn't likely to change, so I wouldn't worry about
that. For MacViews, there should be tests to catch a regression in any case.

> 
> Another option would be to create a separate BaseView specifically for
MacViews
> that would do less mouse event hackery.

Sadly, objectiveC doesn't make this easy - e.g. we would also need a MacViews
specific ToolTipBaseView. But ToolTipBaseView could be made into a component
instead of a class injected into the hierarchy without too much trouble. So I'd
be supportive of this.


> 
> >
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
> > File ui/views/cocoa/drag_drop_client_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
> > ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
> > Can you explain this change? Does it correspond to something that's done in
> Aura
> > that we forgot to add?
> > 
> > views::Widget calls this in Widget::OnMouseEvent() when it gets
> > ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop() as
> > well.
> 
> As I researched, when we start D&D under Aura on Windows the MouseDown event
> handler calls NativeWidgetAura::SetCapture().
> But NativeWidgetAura::ReleaseCapture() is never called after that.
> 
> The Windows OS releases the capture for native window automatically in this
> case. In case Mac OS, it is not happening, and we should call
ReleaseCapture().

So does nothing invoke this code during a drag?

void Widget::OnMouseEvent(ui::MouseEvent* event) {
  View* root_view = GetRootView();
  switch (event->type()) {
    /* snip */
    case ui::ET_MOUSE_RELEASED:
      last_mouse_event_was_move_ = false;
      is_mouse_button_pressed_ = false;
      // Release capture first, to avoid confusion if OnMouseReleased blocks.
      if (auto_release_capture_ && native_widget_->HasCapture()) {
        base::AutoReset<bool> resetter(&ignore_capture_loss_, true);
        native_widget_->ReleaseCapture();
      }

auto_release_capture_ defaults to true and the `resetter` only seems to mess
with views::RootView.

Is that dead code? Or is it a bug that events are not making it through to this
logic in order to call ReleaseCapture()?

[snake, 2016-09-20 15:04:27]

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
File ui/base/cocoa/base_view.h (left):

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#o...
ui/base/cocoa/base_view.h:31: base::scoped_nsobject<NSEvent> pendingExitEvent_;
On 2016/09/19 00:31:01, tapted wrote:
> spqchan is right - we can't change BaseView -- too much other stuff relies on
> it, and it's unlikely there is good test coverage for the UI interactions.
> 
> Can you explain why it's necessary to remove the `pendingExitEvent_` logic in
> order to fix the bug?
> 
> Then, if pendingExitEvent_ really is useless for any situation involving
> BridgedContentView, then it can be disabled via overriding. E.g.
> 
>  - Add a method `handleMouseEvent` which switches on [event type] and does the
> updates of dragging_ and pendingExitEvent_ all in the one method
>  - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
> instead of mouseEvent
>  - override handleMouseEvent in BridgedContentView to just call mouseEvent so
> the logic is skipped.
> 

> > The original change (http://codereview.chromium.org/1774014 ) was fixing the
> > problem with NPAPI-plugins, which should be long gone at this moment. And I
was
> > unable to find other problems in a couple weeks' of testing.

> The CL description says "likely also fixes similar bugs in HTML" and the bug
> doesn't seem NPAPI specific - just "plugin" specific, which may include PPAPI
> plugins like Flash and NaCl. Did you try the repro case on
http://crbug.com/33100?

I can not repro case on http://crbug.com/33100, because TestCase.swf is not
correctly opened by current flash version.
I did try reproduce this, using other plugin (Chromium PDF plugin and Yandex PDF
plugin).

> > 
> > 
> > > Can you explain why it's necessary to remove the `pendingExitEvent_` logic
in
> > > order to fix the bug?
> > 
> > In some cases, we do not receive MouseUp event (when initiating a drag from
the
> > bookmark bar, or showing menu on mouseDown: for example.)
> > As result, the MouseExit event are always ignored, until we click (and
release
> > it) in that same BaseView again.

> What happens to the event? (Why isn't it delivered on the mouseUp in the way
the
> code in BaseView suggests it should be?)

1) Currently In MacViews on any click the MouseUp event are delivered to
BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:. This
happens, beacause on MouseDown we call native_widget_->SetCapture(); in
views::Widget::OnMouseEvent, and after that all messages are delivered using
other way.

2) When we create new NSWindow while processing MouseDown (menu for example),
the MouseUp is never received.

3) When we start D&D using NSView::beginDraggingSession[with item]: , the
MouseUp event will be processed by system, and we receive only
NSView::performDragOperation: . 


> > 
> > > 
> > > Then, if pendingExitEvent_ really is useless for any situation involving
> > > BridgedContentView, then it can be disabled via overriding. E.g.
> > > 
> > >  - Add a method `handleMouseEvent` which switches on [event type] and does
the
> > > updates of dragging_ and pendingExitEvent_ all in the one method
> > >  - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
> > > instead of mouseEvent
> > >  - override handleMouseEvent in BridgedContentView to just call mouseEvent
so
> > > the logic is skipped.
> 
> > We already tried this approach internally, and I think the current proposed
> > solution is the better one, because it's more explicit this way (and the
> > original CL really seems to be outdated).

> I agree it would be nice to remove this extra stuff from BaseView. However, I
> don't know if it's worth the risk. If you can address the first comment, then
we
> should put up a separate CL that _just_ makes the base_view.mm change and land
> that first - then we can bisect any regressions down to that if necessary.

Should I create separate CL for changes in base_view.* ?

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:308: -
(void)resendCapturedMouseEvent:(NSEvent*)theEvent {
On 2016/09/19 00:31:01, tapted wrote:
> > > I think this method has no effect in combination with your deletes in
> > > base_view.mm, since the new methods now all just call mouseEvent. Perhaps
I'm
> > > missing something.

> > This was from my first attemt on fixing this issue. Yes, with the current
> > proposed fix it's not strictly necessary, but if someone will modify the
> > BaseView and change one of the specific mouse events, then it's not
guaranteed
> > that the BridgedContentView will do the right thing, and this could lead to
a
> > potential inconsistency.

> We typically don't add unnecessary code until we're sure it's needed. I think
> going forward base_view.mm isn't likely to change, so I wouldn't worry about
> that. For MacViews, there should be tests to catch a regression in any case.
> 
> > 
> > Another option would be to create a separate BaseView specifically for
> > MacViews that would do less mouse event hackery.
> 
> Sadly, objectiveC doesn't make this easy - e.g. we would also need a MacViews
> specific ToolTipBaseView. But ToolTipBaseView could be made into a component
> instead of a class injected into the hierarchy without too much trouble. So
I'd
> be supportive of this.

We can use #if !defined(TOOLKIT_VIEWS) macros in base_view.mm

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
On 2016/09/19 00:31:01, tapted wrote:
> Can you explain this change? Does it correspond to something that's done in
Aura
> that we forgot to add?
> 
> views::Widget calls this in Widget::OnMouseEvent() when it gets
> ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop() as
> well.

> > The Windows OS releases the capture for native window automatically in this
> > case. In case Mac OS, it is not happening, and we should call
ReleaseCapture().

> So does nothing invoke this code during a drag?

Yes. 

> void Widget::OnMouseEvent(ui::MouseEvent* event) {
>   View* root_view = GetRootView();
>   switch (event->type()) {
>     /* snip */
>     case ui::ET_MOUSE_RELEASED:
>       last_mouse_event_was_move_ = false;
>       is_mouse_button_pressed_ = false;
>       // Release capture first, to avoid confusion if OnMouseReleased blocks.
>       if (auto_release_capture_ && native_widget_->HasCapture()) {
>         base::AutoReset<bool> resetter(&ignore_capture_loss_, true);
>         native_widget_->ReleaseCapture();
>       }
> 
> auto_release_capture_ defaults to true and the `resetter` only seems to mess
with views::RootView.
> 
> Is that dead code? Or is it a bug that events are not making it through to
this
> logic in order to call ReleaseCapture()?

No, this is not dead code, it is called on MouseUp event, but not called on D&D.

[tapted, 2016-09-21 10:07:04]

tapted@chromium.org changed reviewers:
+ avi@chromium.org
- dcheng@chromium.org

[tapted, 2016-09-21 10:07:05]

+avi (moved dcheng to cc) - see below.

Thanks for the extra info! Let's put some into the CL description (see later
comment). I think I'm mostly convinced, but I'm still uneasy about the
base_view.mm change without more certainty that http://crbug.com/33100 or
something similar isn't regressing. But it would be certainly nice to remove
this stuff from base_view.mm if nothing needs it.

It's a big ask... but maybe avi has an inkling of something that may break if we
remove this stuff from base_view.mm, or knows the right way to manually test for
a regression?

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
File ui/base/cocoa/base_view.h (left):

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#o...
ui/base/cocoa/base_view.h:31: base::scoped_nsobject<NSEvent> pendingExitEvent_;
On 2016/09/20 15:04:26, snake wrote:
> On 2016/09/19 00:31:01, tapted wrote:
> > spqchan is right - we can't change BaseView -- too much other stuff relies
on
> > it, and it's unlikely there is good test coverage for the UI interactions.
> > 
> > Can you explain why it's necessary to remove the `pendingExitEvent_` logic
in
> > order to fix the bug?
> > 
> > Then, if pendingExitEvent_ really is useless for any situation involving
> > BridgedContentView, then it can be disabled via overriding. E.g.
> > 
> >  - Add a method `handleMouseEvent` which switches on [event type] and does
the
> > updates of dragging_ and pendingExitEvent_ all in the one method
> >  - Change all the BaseView [foo]MouseBar methods to call handleMouseEvent
> > instead of mouseEvent
> >  - override handleMouseEvent in BridgedContentView to just call mouseEvent
so
> > the logic is skipped.
> > 
> 
> > > The original change (http://codereview.chromium.org/1774014 ) was fixing
the
> > > problem with NPAPI-plugins, which should be long gone at this moment. And
I
> was
> > > unable to find other problems in a couple weeks' of testing.
> 
> > The CL description says "likely also fixes similar bugs in HTML" and the bug
> > doesn't seem NPAPI specific - just "plugin" specific, which may include
PPAPI
> > plugins like Flash and NaCl. Did you try the repro case on
> http://crbug.com/33100
> 
> I can not repro case on http://crbug.com/33100, because TestCase.swf is not
> correctly opened by current flash version.
> I did try reproduce this, using other plugin (Chromium PDF plugin and Yandex
PDF
> plugin).
> 
> > > 
> > > 
> > > > Can you explain why it's necessary to remove the `pendingExitEvent_`
logic
> in
> > > > order to fix the bug?
> > > 
> > > In some cases, we do not receive MouseUp event (when initiating a drag
from
> the
> > > bookmark bar, or showing menu on mouseDown: for example.)
> > > As result, the MouseExit event are always ignored, until we click (and
> release
> > > it) in that same BaseView again.
> 
> > What happens to the event? (Why isn't it delivered on the mouseUp in the way
> the
> > code in BaseView suggests it should be?)
> 
> 1) Currently In MacViews on any click the MouseUp event are delivered to
> BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:.
This
> happens, beacause on MouseDown we call native_widget_->SetCapture(); in
> views::Widget::OnMouseEvent, and after that all messages are delivered using
> other way.
> 
> 2) When we create new NSWindow while processing MouseDown (menu for example),
> the MouseUp is never received.
> 
> 3) When we start D&D using NSView::beginDraggingSession[with item]: , the
> MouseUp event will be processed by system, and we receive only
> NSView::performDragOperation: . 

So a possible alternative fix may be to generate a synthetic mouseUp event
that's sent once the dragging session ends. That may even be "nicer", if it
gives views::Views a way to tidy up their state. But if that doesn't happen on
Aura, then it's not likely that Views are expecting that. Also it's more
complicated. So let's go with this.

> 
> 
> > > 
> > > > 
> > > > Then, if pendingExitEvent_ really is useless for any situation involving
> > > > BridgedContentView, then it can be disabled via overriding. E.g.
> > > > 
> > > >  - Add a method `handleMouseEvent` which switches on [event type] and
does
> the
> > > > updates of dragging_ and pendingExitEvent_ all in the one method
> > > >  - Change all the BaseView [foo]MouseBar methods to call
handleMouseEvent
> > > > instead of mouseEvent
> > > >  - override handleMouseEvent in BridgedContentView to just call
mouseEvent
> so
> > > > the logic is skipped.
> > 
> > > We already tried this approach internally, and I think the current
proposed
> > > solution is the better one, because it's more explicit this way (and the
> > > original CL really seems to be outdated).
> 
> > I agree it would be nice to remove this extra stuff from BaseView. However,
I
> > don't know if it's worth the risk. If you can address the first comment,
then
> we
> > should put up a separate CL that _just_ makes the base_view.mm change and
land
> > that first - then we can bisect any regressions down to that if necessary.
> 
> Should I create separate CL for changes in base_view.* ?
> 

Hm, I guess there's not much left after removing the unnecessary code - let's
leave it.

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h
File ui/base/cocoa/base_view.h (right):

https://codereview.chromium.org/2337233004/diff/1/ui/base/cocoa/base_view.h#n...
ui/base/cocoa/base_view.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
CL description - can you flesh it out with some of the info you provided.
There's a formula for change list descriptions at -
http://dev.chromium.org/developers/contributing-code#TOC-Writing-change-list-...

E.g. this one should go something like.

Currently, clicks immediately after a drag in a MacViews window are ignored.
This is because initiating
a drag and drop session with -[NSView beginDraggingSessionWithItems:..]
suppresses the mouse events that would result in capture being released. To fix,
explicitly release Widget capture before starting the dragging session.

It's also possible for captured events to be incorrectly suppressed by stale
logic in BaseView that's no longer needed. Remove it.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.h:74: // Resend a mouse event captured while
the widget had global mouse capture int
(remove - see below)

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:308: -
(void)resendCapturedMouseEvent:(NSEvent*)theEvent {
On 2016/09/20 15:04:26, snake wrote:
> On 2016/09/19 00:31:01, tapted wrote:
> > > > I think this method has no effect in combination with your deletes in
> > > > base_view.mm, since the new methods now all just call mouseEvent.
Perhaps
> I'm
> > > > missing something.
> 
> > > This was from my first attemt on fixing this issue. Yes, with the current
> > > proposed fix it's not strictly necessary, but if someone will modify the
> > > BaseView and change one of the specific mouse events, then it's not
> guaranteed
> > > that the BridgedContentView will do the right thing, and this could lead
to
> a
> > > potential inconsistency.
> 
> > We typically don't add unnecessary code until we're sure it's needed. I
think
> > going forward base_view.mm isn't likely to change, so I wouldn't worry about
> > that. For MacViews, there should be tests to catch a regression in any case.
> > 
> > > 
> > > Another option would be to create a separate BaseView specifically for
> > > MacViews that would do less mouse event hackery.
> > 
> > Sadly, objectiveC doesn't make this easy - e.g. we would also need a
MacViews
> > specific ToolTipBaseView. But ToolTipBaseView could be made into a component
> > instead of a class injected into the hierarchy without too much trouble. So
> I'd
> > be supportive of this.
> 
> We can use #if !defined(TOOLKIT_VIEWS) macros in base_view.mm

TOOLKIT_VIEWS is always defined on Mac, so that won't have an effect.

Anyway, let's remove part of the diff - having it makes stacks more complex and
there's no guarantee that it can protect against all hypothetical changes.

Plus with this gone, there's so little on top of the base_view.mm change that
I'm fine with landing them together.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
On 2016/09/20 15:04:26, snake wrote:
> On 2016/09/19 00:31:01, tapted wrote:
> > Can you explain this change? Does it correspond to something that's done in
> Aura
> > that we forgot to add?
> > 
> > views::Widget calls this in Widget::OnMouseEvent() when it gets
> > ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop() as
> > well.
> 
> > > The Windows OS releases the capture for native window automatically in
this
> > > case. In case Mac OS, it is not happening, and we should call
> ReleaseCapture().
> 
> > So does nothing invoke this code during a drag?
> 
> Yes. 
> 
> > void Widget::OnMouseEvent(ui::MouseEvent* event) {
> >   View* root_view = GetRootView();
> >   switch (event->type()) {
> >     /* snip */
> >     case ui::ET_MOUSE_RELEASED:
> >       last_mouse_event_was_move_ = false;
> >       is_mouse_button_pressed_ = false;
> >       // Release capture first, to avoid confusion if OnMouseReleased
blocks.
> >       if (auto_release_capture_ && native_widget_->HasCapture()) {
> >         base::AutoReset<bool> resetter(&ignore_capture_loss_, true);
> >         native_widget_->ReleaseCapture();
> >       }
> > 
> > auto_release_capture_ defaults to true and the `resetter` only seems to mess
> with views::RootView.
> > 
> > Is that dead code? Or is it a bug that events are not making it through to
> this
> > logic in order to call ReleaseCapture()?
> 
> No, this is not dead code, it is called on MouseUp event, but not called on
D&D.

OK, if we don't fix it a different way, we'll need to add a comment here
outlining the case (3) you identified. E.g.

// Release capture before beginning the dragging session. Capture may have been
acquired on the mouseDown, but capture is not required during the dragging
session and the mouseUp that would release it will be suppressed.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac_unittest.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:227: // Tests that widget
releases mouse capture when drag'n'drop is started.
Since StartDragAndDrop() spins the runloop, the test is actually testing that
capture is released by the time the drag ends - but that's what we want :). E.g.
this should say

// Ensure that capture is released before the end of a drag and drop operation.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:231: // Generate mouse press to
toggle dragging_ flag in BaseView.
this comment is out of date

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:238: bridge_->AcquireCapture();
Be aware that capture on Windows typically can't be tested in non-interactive
tests - the problem otherwise is that other tests may be running in parallel,
posting their own mouse events that will be captured. For Mac.. I suppose this
is OK since the drag should exit before any events could get captured. But
that's worth a comment here. E.g.

// Although this is not an interactive UI test, acquiring capture should be OK
// since the runloop will exit before the system has any opportunity to capture
anything.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:258: // It will call
ReleaseCapture(), and BaseView will no longer think it's
update comment.

[snake, 2016-09-21 12:15:27]

Description was changed from

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

BUG=646792
==========

to

==========
Currently, clicks immediately after a drag in a MacViews window are ignored.
This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events that
would
result in capture being released. To fix, explicitly release Widget capture
before starting the dragging session.

It's also possible for captured events to be incorrectly suppressed by 
stale logic in BaseView that's no longer needed. Remove it.

BUG=646792
==========

[snake, 2016-09-21 12:29:52]

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.h:74: // Resend a mouse event captured while
the widget had global mouse capture int
On 2016/09/21 10:07:05, tapted wrote:
> (remove - see below)

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:308: -
(void)resendCapturedMouseEvent:(NSEvent*)theEvent {
On 2016/09/21 10:07:05, tapted wrote:
> On 2016/09/20 15:04:26, snake wrote:
> > On 2016/09/19 00:31:01, tapted wrote:
> > > > > I think this method has no effect in combination with your deletes in
> > > > > base_view.mm, since the new methods now all just call mouseEvent.
> Perhaps
> > I'm
> > > > > missing something.
> > 
> > > > This was from my first attemt on fixing this issue. Yes, with the
current
> > > > proposed fix it's not strictly necessary, but if someone will modify the
> > > > BaseView and change one of the specific mouse events, then it's not
> > guaranteed
> > > > that the BridgedContentView will do the right thing, and this could lead
> to
> > a
> > > > potential inconsistency.
> > 
> > > We typically don't add unnecessary code until we're sure it's needed. I
> think
> > > going forward base_view.mm isn't likely to change, so I wouldn't worry
about
> > > that. For MacViews, there should be tests to catch a regression in any
case.
> > > 
> > > > 
> > > > Another option would be to create a separate BaseView specifically for
> > > > MacViews that would do less mouse event hackery.
> > > 
> > > Sadly, objectiveC doesn't make this easy - e.g. we would also need a
> MacViews
> > > specific ToolTipBaseView. But ToolTipBaseView could be made into a
component
> > > instead of a class injected into the hierarchy without too much trouble.
So
> > I'd
> > > be supportive of this.
> > 
> > We can use #if !defined(TOOLKIT_VIEWS) macros in base_view.mm
> 
> TOOLKIT_VIEWS is always defined on Mac, so that won't have an effect.
> 
> Anyway, let's remove part of the diff - having it makes stacks more complex
and
> there's no guarantee that it can protect against all hypothetical changes.
> 
> Plus with this gone, there's so little on top of the base_view.mm change that
> I'm fine with landing them together.

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac.mm:82: bridge_->ReleaseCapture();
On 2016/09/21 10:07:05, tapted wrote:
> On 2016/09/20 15:04:26, snake wrote:
> > On 2016/09/19 00:31:01, tapted wrote:
> > > Can you explain this change? Does it correspond to something that's done
in
> > Aura
> > > that we forgot to add?
> > > 
> > > views::Widget calls this in Widget::OnMouseEvent() when it gets
> > > ET_MOUSE_RELEASED - it seems strange to call it during StartDragAndDrop()
as
> > > well.
> > 
> > > > The Windows OS releases the capture for native window automatically in
> this
> > > > case. In case Mac OS, it is not happening, and we should call
> > ReleaseCapture().
> > 
> > > So does nothing invoke this code during a drag?
> > 
> > Yes. 
> > 
> > > void Widget::OnMouseEvent(ui::MouseEvent* event) {
> > >   View* root_view = GetRootView();
> > >   switch (event->type()) {
> > >     /* snip */
> > >     case ui::ET_MOUSE_RELEASED:
> > >       last_mouse_event_was_move_ = false;
> > >       is_mouse_button_pressed_ = false;
> > >       // Release capture first, to avoid confusion if OnMouseReleased
> blocks.
> > >       if (auto_release_capture_ && native_widget_->HasCapture()) {
> > >         base::AutoReset<bool> resetter(&ignore_capture_loss_, true);
> > >         native_widget_->ReleaseCapture();
> > >       }
> > > 
> > > auto_release_capture_ defaults to true and the `resetter` only seems to
mess
> > with views::RootView.
> > > 
> > > Is that dead code? Or is it a bug that events are not making it through to
> > this
> > > logic in order to call ReleaseCapture()?
> > 
> > No, this is not dead code, it is called on MouseUp event, but not called on
> D&D.
> 
> OK, if we don't fix it a different way, we'll need to add a comment here
> outlining the case (3) you identified. E.g.
> 
> // Release capture before beginning the dragging session. Capture may have
been
> acquired on the mouseDown, but capture is not required during the dragging
> session and the mouseUp that would release it will be suppressed.

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
File ui/views/cocoa/drag_drop_client_mac_unittest.mm (right):

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:227: // Tests that widget
releases mouse capture when drag'n'drop is started.
On 2016/09/21 10:07:05, tapted wrote:
> Since StartDragAndDrop() spins the runloop, the test is actually testing that
> capture is released by the time the drag ends - but that's what we want :).
E.g.
> this should say
> 
> // Ensure that capture is released before the end of a drag and drop
operation.

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:231: // Generate mouse press to
toggle dragging_ flag in BaseView.
On 2016/09/21 10:07:05, tapted wrote:
> this comment is out of date

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:238: bridge_->AcquireCapture();
On 2016/09/21 10:07:05, tapted wrote:
> Be aware that capture on Windows typically can't be tested in non-interactive
> tests - the problem otherwise is that other tests may be running in parallel,
> posting their own mouse events that will be captured. For Mac.. I suppose this
> is OK since the drag should exit before any events could get captured. But
> that's worth a comment here. E.g.
> 
> // Although this is not an interactive UI test, acquiring capture should be OK
> // since the runloop will exit before the system has any opportunity to
capture
> anything.

Done.

https://codereview.chromium.org/2337233004/diff/1/ui/views/cocoa/drag_drop_cl...
ui/views/cocoa/drag_drop_client_mac_unittest.mm:258: // It will call
ReleaseCapture(), and BaseView will no longer think it's
On 2016/09/21 10:07:05, tapted wrote:
> update comment.

Done.

[Avi (use Gerrit), 2016-09-21 14:51:55]

Wow. No, I really don't remember that CL nor its context.

Of what I remember, the whole "mouse enter/leave" thing was evident by mouse
pointer changes, if that helps. I wouldn't be surprised if there's nothing left
that needs it.

[tapted, 2016-09-21 23:59:16]

Description was changed from

==========
Currently, clicks immediately after a drag in a MacViews window are ignored.
This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events that
would
result in capture being released. To fix, explicitly release Widget capture
before starting the dragging session.

It's also possible for captured events to be incorrectly suppressed by 
stale logic in BaseView that's no longer needed. Remove it.

BUG=646792
==========

to

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are ignored.
This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events that
would
result in capture being released. To fix, explicitly release Widget capture
before starting the dragging session.

It's also possible for captured events to be incorrectly suppressed by 
stale logic in BaseView that's no longer needed. Remove it.

BUG=646792
==========

[tapted, 2016-09-22 00:32:37]

Finally had a bit more time to patch this in and play around some more.

I found one rather bad regression with text selection. Currently in Chrome you
can be highlighting text in the WebContents area with a mouse drag and the mouse
can leave the window, then re-enter - text selection continues.

With this patched in, text selection is cancelled when the mouse leaves the
window - the selection no longer updates after re-entering the window.

So I think we still need pendingExitEvent_.

Do you want to land the CL with just the drag_drop_client_mac* change? I think
that's still making progress, and we can do a focused fix for the event type
switching.

[snake, 2016-09-22 18:22:39]

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:560: - (void)mouseDown:(NSEvent*)theEvent
{
What about this MacViews specific fix, for now?

[tapted, 2016-09-23 07:10:21]

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:560: - (void)mouseDown:(NSEvent*)theEvent
{
On 2016/09/22 18:22:39, snake wrote:
> What about this MacViews specific fix, for now?

The code looks good, but I tried commenting this out and can't tell the
difference in behaviour. You explained earlier that the MouseExit could be
ignored - I tried dragging from the bookmarks bar and showing menus, but it
seems to work fine without this. I'm probably missing something.

As well as saying what the override is doing (as you have), can you explain in
the comment why it's necessary? There was a chromium-dev post about this
recently -- see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MY4e_wJY6yY/cOVLt...

The CL description needs to be updated too.

[snake, 2016-09-26 10:28:48]

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2337233004/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:560: - (void)mouseDown:(NSEvent*)theEvent
{
On 2016/09/23 07:10:21, tapted wrote:
> On 2016/09/22 18:22:39, snake wrote:
> > What about this MacViews specific fix, for now?
> 
> The code looks good, but I tried commenting this out and can't tell the
> difference in behaviour. You explained earlier that the MouseExit could be
> ignored - I tried dragging from the bookmarks bar and showing menus, but it
> seems to work fine without this. I'm probably missing something.
> 
> As well as saying what the override is doing (as you have), can you explain in
> the comment why it's necessary? There was a chromium-dev post about this
> recently -- see
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MY4e_wJY6yY/cOVLt...
> 
> The CL description needs to be updated too.

hm.. I will investigate this. I did remove this for now.

Done.

[tapted, 2016-09-26 11:16:36]

Description was changed from

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are ignored.
This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events that
would
result in capture being released. To fix, explicitly release Widget capture
before starting the dragging session.

It's also possible for captured events to be incorrectly suppressed by 
stale logic in BaseView that's no longer needed. Remove it.

BUG=646792
==========

to

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792
==========

[tapted, 2016-09-26 11:19:17]

lgtm - thanks

note I removed the part of the CL description about BaseView - you can commit if
you're happy with it

[snake, 2016-09-26 17:19:34]

The CQ bit was checked by art-snake@yandex-team.ru

[commit-bot: I haz the power, 2016-09-26 17:20:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 17:51:58]

Description was changed from

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792
==========

to

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792
==========

[commit-bot: I haz the power, 2016-09-26 17:51:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-26 17:54:57]

Description was changed from

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792
==========

to

==========
MacViews: Fix sending mouse exit event and releasing capture on D&D.

Currently, clicks immediately after a drag in a MacViews window are
ignored. This is because initiating a drag and drop session with
-[NSView beginDraggingSessionWithItems:..] suppresses the mouse events
that would result in capture being released. To fix, explicitly release
Widget capture before starting the dragging session.

BUG=646792

Committed: https://crrev.com/48134b46f0a22980774d9b9cb074e04b4971ada5
Cr-Commit-Position: refs/heads/master@{#420925}
==========

[commit-bot: I haz the power, 2016-09-26 17:54:58]

Patchset 4 (id:??) landed as
https://crrev.com/48134b46f0a22980774d9b9cb074e04b4971ada5
Cr-Commit-Position: refs/heads/master@{#420925}