Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448173002:
Fix processing of mouse events on MacViews.
DescriptionFix processing of mouse events on MacViews.
BUG=659959
Before, In MacViews on any click the MouseUp event are delivered to BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:, and BaseView::dragging_ flag not reset. This happens, beacause on MouseDown we call native_widget_->SetCapture() in views::Widget::OnMouseEvent, and after that all messages are delivered using other way. As result, some times, any mouse exit events are blocked in BaseView::MouseExited, because BaseView::dragging_ still is true. (and hovers are broken).
To fix this, in BridgedContentView, implemented logic like in BaseView, considering with mouse capturing.
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fix processing of mouse events on MacViews. #Patch Set 3 : Fix processing of mouse events on MacViews. #
Total comments: 8
Patch Set 4 : Fix review issues. #
Messages
Total messages: 18 (4 generated)
Description was changed from ========== Fix processing of mouse events on MacViews. fix test. fix mouse enter/exit logic for MacViews. BUG= ========== to ========== Fix processing of mouse events on MacViews. BUG=646792 Before, In MacViews on any click the MouseUp event are delivered to BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:, and BaseView::dragging_ flag not reset. This happens, beacause on MouseDown we call native_widget_->SetCapture() in views::Widget::OnMouseEvent, and after that all messages are delivered using other way. As result, some times, any mouse exit events are blocked in BaseView::MouseExited, because BaseView::dragging_ still is true. (and hovers are broken). To fix this, in BridgedContentView, implemented logic like in BaseView, considering with mouse capturing. ==========
art-snake@yandex-team.ru changed reviewers: + avi@chromium.org, tapted@chromium.org
avi@chromium.org changed reviewers: - avi@chromium.org
I stopped review about halfway through. First, there seem to be quite a few changes that don't look related. Was there a technical issue where you created this change against an earlier version of the code but didn't pick up the changes that were made since then? Second, I haven't been working on MacViews, so I'm going to defer to Trent who knows this area much more than I do. https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:46: return ET_MOUSEWHEEL; This was recently changed by Trent in r421082; why are you changing this back? https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn File ui/views/BUILD.gn (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn#newcode880 ui/views/BUILD.gn:880: "//testing/gmock", Why are you changing this? https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:78: This seems unrelated. https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:58: #if 0 // BROWSER-55005 Don't commit commented-out code.
https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:46: return ET_MOUSEWHEEL; On 2016/10/25 15:34:50, Avi wrote: > This was recently changed by Trent in r421082; why are you changing this back? views::Widget is not supported ET_SCROLL, only ET_MOUSEWHEEL. see https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1244 Alse see test, added in this CL: NativeWidgetMacTest.MouseWheelEvent It is failed without this. https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn File ui/views/BUILD.gn (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn#newcode880 ui/views/BUILD.gn:880: "//testing/gmock", On 2016/10/25 15:34:50, Avi wrote: > Why are you changing this? For using gmock in tests https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:78: On 2016/10/25 15:34:50, Avi wrote: > This seems unrelated. On mouse exit, we should hide tooltip, in other case, it can be shown in incorrect place (outside browser bounds), this is also related to processing mouseexit event. https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:58: #if 0 // BROWSER-55005 On 2016/10/25 15:34:50, Avi wrote: > Don't commit commented-out code. Done.
https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2448173002/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:46: return ET_MOUSEWHEEL; On 2016/10/25 15:46:18, snake wrote: > On 2016/10/25 15:34:50, Avi wrote: > > This was recently changed by Trent in r421082; why are you changing this back? > views::Widget is not supported ET_SCROLL, only ET_MOUSEWHEEL. > see > https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1244 > Alse see test, added in this CL: NativeWidgetMacTest.MouseWheelEvent > It is failed without this. Reverted.
There's too much going on here to review - please split it up. (see comments). Also you previously had a patch in https://codereview.chromium.org/2337233004 for http://crbug.com/646792. that bug is marked fixed. What's different now? Can you file a new bug so that we can get a clear idea whether this is the right approach? https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn File ui/views/BUILD.gn (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/BUILD.gn#newcode880 ui/views/BUILD.gn:880: "//testing/gmock", On 2016/10/25 15:46:18, snake wrote: > On 2016/10/25 15:34:50, Avi wrote: > > Why are you changing this? > > For using gmock in tests gmock isn't allowed under src/ui/views -- ui/views/DEPS has include_rules = [ ... "-testing/gmock", ] the OWNERS have decided that delegate, observer, and TestApi patterns are the preferred way of testing. https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:78: On 2016/10/25 15:46:18, snake wrote: > On 2016/10/25 15:34:50, Avi wrote: > > This seems unrelated. > > On mouse exit, we should hide tooltip, in other case, it can be shown in > incorrect place (outside browser bounds), this is also related to processing > mouseexit event. Is this related to http://crbug.com/592085 ? Perhaps the fix should be in ToolTipBaseView instead, but it should be its own CL rather than being mixed in here. https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { Event handling doesn't belong in BridgedNativeWidget -- please keep it in BridgedContentView, or explain why it's needed here. But also this hit test masking stuff doesn't seem related to the rest of the change - please move it to a separate CL. https://codereview.chromium.org/2448173002/diff/40001/ui/views/test/event_gen... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/test/event_gen... ui/views/test/event_generator_delegate_mac.mm:175: [responder mouseEntered:event]; EventGenerator is for cross-platform testing of views::Views. If you need to send mouseEntered/Exited, you should use cocoa_test_event_utils https://codereview.chromium.org/2448173002/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:647: class MockView : public View { gmock isn't allowed here. You'll need to use a different testing approach. Perhaps try something from ui/views/test/test_views.h
>Also you previously had a patch in >https://codereview.chromium.org/2337233004 >for http://crbug.com/646792. that bug is marked fixed. >What's different now? Can >you file a new bug so that we can get a clear idea whether >this is the right approach? Some times the hover problem from http://crbug.com/646792 is reproduced. It is in base_view.mm (like i say in https://codereview.chromium.org/2337233004#msg7) To check this you can add DHECK(!dragging_); in BaseView:mouseDown before set dragging_= YES; And try to using MacViews browser with this for some time. When this check will be happened, the hovers will be broken. https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2448173002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:78: On 2016/10/26 00:57:00, tapted wrote: > On 2016/10/25 15:46:18, snake wrote: > > On 2016/10/25 15:34:50, Avi wrote: > > > This seems unrelated. > > > > On mouse exit, we should hide tooltip, in other case, it can be shown in > > incorrect place (outside browser bounds), this is also related to processing > > mouseexit event. > > Is this related to http://crbug.com/592085 ? Perhaps the fix should be in > ToolTipBaseView instead, but it should be its own CL rather than being mixed in > here. No, this is not related. Because i have changed the logic of processing mouse events, i should do additional call to hide tooltip. I other case it will be bug (related to this CL). And as result i can not move this in separate CL. (JFYI: But http://crbug.com/592085 may be fixed after this CL, because mouse events are processed more correctly in it). https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { On 2016/10/26 00:57:00, tapted wrote: > Event handling doesn't belong in BridgedNativeWidget -- please keep it in > BridgedContentView, or explain why it's needed here. BridgedNativeWidget is more appropriate place for processing events, in it we have capturing info and get captured messages. It is intersection point of all data, which needs to process events. > But also this hit test > masking stuff doesn't seem related to the rest of the change - please move it to > a separate CL. For correctly send MouseExit I should check hit test and widget bounds. Is not it? https://codereview.chromium.org/2448173002/diff/40001/ui/views/test/event_gen... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/test/event_gen... ui/views/test/event_generator_delegate_mac.mm:175: [responder mouseEntered:event]; On 2016/10/26 00:57:00, tapted wrote: > EventGenerator is for cross-platform testing of views::Views. If you need to > send mouseEntered/Exited, you should use cocoa_test_event_utils Yes, this needs for testing MouseEnter/Exit evenet in views::Views, not just in RootView. For example, views::Widget are processing MouseExitEvent. https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1238 https://codereview.chromium.org/2448173002/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:647: class MockView : public View { On 2016/10/26 00:57:00, tapted wrote: > gmock isn't allowed here. You'll need to use a different testing approach. > Perhaps try something from ui/views/test/test_views.h Done.
On 2016/10/26 12:44:57, snake wrote: > >Also you previously had a patch in > >https://codereview.chromium.org/2337233004 > >for http://crbug.com/646792. that bug is marked fixed. > >What's different now? Can > >you file a new bug so that we can get a clear idea whether > >this is the right approach? Please file a new bug with repro steps. I might be able to help you find a much simpler fix. Currently this is adding a lot of complexity. Maybe the fix is something simple like having -[BridgedContentView processCapturedMouseEvent:] call a new method, -[BaseView clearDrag] With the number of codepaths that you're changing I would say that first we should just cut out BaseView from the hierarchy, or modify it with a neat way to disable some of its functionality. Of course, to keep TooltipBaseView, we would need to make that a component rather than a class injected into the hierarchy. But maybe none of this is needed. Please file a new bug. https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { On 2016/10/26 12:44:56, snake wrote: > On 2016/10/26 00:57:00, tapted wrote: > > Event handling doesn't belong in BridgedNativeWidget -- please keep it in > > BridgedContentView, or explain why it's needed here. > > BridgedNativeWidget is more appropriate place for processing events, in it we > have capturing info and get captured messages. What about [BridgedContentView processCapturedMouseEvent]? BridgedContentView also interacts with Widget already -- it has {Set,Release,Has}Capture. BridgedContentView currently has _very_ limited interaction with BridgedNativeWidget because their lifetimes are not in step. Capture in this sense is a "window" concept -- on other platforms we ask the OS to deliver events to a particular window, but it's still up to the window how to route those events. It's uncommon for the window to handle them itself. > It is intersection point of all > data, which needs to process events. This is a bad reason. "intersecting" functionality sounds like the opposite of what we want to do in well designed programs. It is better to have more classes with specific functionality, and clear interfaces between them. BridgedNativeWidget manages the window, not events. If you need to group together a set of data to fix this bug, it sounds like it should be a new class. If it's related specifically to dragging, then perhaps views::DragDropClientMac is where it should belong. This class already exists and BridgedContentView already has a way to obtain one. But I still do not know whether this is necessary. > > But also this hit test > > masking stuff doesn't seem related to the rest of the change - please move it > to > > a separate CL. > > For correctly send MouseExit I should check hit test and widget bounds. Is not > it? > ui::ET_MOUSE_EXITED and ui::ET_MOUSE_ENTERED are generated by RootView::OnMouseMoved(const ui::MouseEvent& event) If we need to generate them a different way on MacViews, then that needs to be a separate bug, and a separate CL. It is not clear to me why this is needed currently. Please file a bug.
Description was changed from ========== Fix processing of mouse events on MacViews. BUG=646792 Before, In MacViews on any click the MouseUp event are delivered to BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:, and BaseView::dragging_ flag not reset. This happens, beacause on MouseDown we call native_widget_->SetCapture() in views::Widget::OnMouseEvent, and after that all messages are delivered using other way. As result, some times, any mouse exit events are blocked in BaseView::MouseExited, because BaseView::dragging_ still is true. (and hovers are broken). To fix this, in BridgedContentView, implemented logic like in BaseView, considering with mouse capturing. ========== to ========== Fix processing of mouse events on MacViews. BUG=659959 Before, In MacViews on any click the MouseUp event are delivered to BridgedContentView::processCapturedMouseEvent: , not to BaseView::MouseUp:, and BaseView::dragging_ flag not reset. This happens, beacause on MouseDown we call native_widget_->SetCapture() in views::Widget::OnMouseEvent, and after that all messages are delivered using other way. As result, some times, any mouse exit events are blocked in BaseView::MouseExited, because BaseView::dragging_ still is true. (and hovers are broken). To fix this, in BridgedContentView, implemented logic like in BaseView, considering with mouse capturing. ==========
> > >Also you previously had a patch in > > >https://codereview.chromium.org/2337233004 > > >for http://crbug.com/646792. that bug is marked fixed. > > >What's different now? Can > > >you file a new bug so that we can get a clear idea whether > > >this is the right approach? > > Please file a new bug with repro steps. I might be able to help you find a much > simpler fix. > Ok, i have created new bug for this CL. > Currently this is adding a lot of complexity. Maybe the fix is something simple > like having -[BridgedContentView processCapturedMouseEvent:] call a new method, > -[BaseView clearDrag] > > > With the number of codepaths that you're changing I would say that first we > should just cut out BaseView from the hierarchy, or modify it with a neat way to > disable some of its functionality. Of course, to keep TooltipBaseView, we would > need to make that a component rather than a class injected into the hierarchy. > But maybe none of this is needed. Please file a new bug. I did try like this in my prev CL https://codereview.chromium.org/2337233004, But overriding the methods of baseView was yous idea. https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { On 2016/10/27 02:39:10, tapted wrote: > On 2016/10/26 12:44:56, snake wrote: > > On 2016/10/26 00:57:00, tapted wrote: > > > Event handling doesn't belong in BridgedNativeWidget -- please keep it in > > > BridgedContentView, or explain why it's needed here. > > > > BridgedNativeWidget is more appropriate place for processing events, in it we > > have capturing info and get captured messages. > > What about [BridgedContentView processCapturedMouseEvent]? > > BridgedContentView also interacts with Widget already -- it has > {Set,Release,Has}Capture. Hm... I do not see any Capturing methods in BridgedContentView code. > BridgedContentView currently has _very_ limited > interaction with BridgedNativeWidget because their lifetimes are not in step. > BridgedNativeWidget owns BridgedContentView, is not it? > Capture in this sense is a "window" concept -- on other platforms we ask the OS > to deliver events to a particular window, but it's still up to the window how to > route those events. It's uncommon for the window to handle them itself. > We already have helper (CocoaMouseCapture) for this in BridgedNativeWidget (not in BridgedContentView). > > It is intersection point of all > > data, which needs to process events. > > This is a bad reason. "intersecting" functionality sounds like the opposite of > what we want to do in well designed programs. It is better to have more classes > with specific functionality, and clear interfaces between them. > > BridgedNativeWidget manages the window, not events. If you need to group > together a set of data to fix this bug, it sounds like it should be a new class. > ok, what about new class NativeEventsRouter ? > If it's related specifically to dragging, then perhaps views::DragDropClientMac > is where it should belong. This class already exists and BridgedContentView > already has a way to obtain one. But I still do not know whether this is > necessary. > This is related to mouse down/up logic with capturing events (the D&D is special case of this.) > > > But also this hit test > > > masking stuff doesn't seem related to the rest of the change - please move > it > > to > > > a separate CL. > > > > For correctly send MouseExit I should check hit test and widget bounds. Is not > > it? > > > > ui::ET_MOUSE_EXITED and ui::ET_MOUSE_ENTERED are generated by > RootView::OnMouseMoved(const ui::MouseEvent& event) > > If we need to generate them a different way on MacViews, then that needs to be a > separate bug, and a separate CL. It is not clear to me why this is needed > currently. Please file a bug. Native NSMouseExitEvent should be processed in views::__WIDGET__::OnMouseEvent, BEFORE AND NOT IN RootView!!! Also this way like on AURA + Windows.
Also i have updated description with new bug id.
On 2016/10/27 10:54:22, snake wrote: > > > >Also you previously had a patch in > > > >https://codereview.chromium.org/2337233004 > > > >for http://crbug.com/646792. that bug is marked fixed. > > > >What's different now? Can > > > >you file a new bug so that we can get a clear idea whether > > > >this is the right approach? > > > > Please file a new bug with repro steps. I might be able to help you find a > much > > simpler fix. > > > Ok, i have created new bug for this CL. > > > Currently this is adding a lot of complexity. Maybe the fix is something > simple > > like having -[BridgedContentView processCapturedMouseEvent:] call a new > method, > > -[BaseView clearDrag] > > > > > > With the number of codepaths that you're changing I would say that first we > > should just cut out BaseView from the hierarchy, or modify it with a neat way > to > > disable some of its functionality. Of course, to keep TooltipBaseView, we > would > > need to make that a component rather than a class injected into the hierarchy. > > But maybe none of this is needed. Please file a new bug. > I did try like this in my prev CL https://codereview.chromium.org/2337233004, You suggested it, but didn't upload code. I said it is not simple, but I'd be supportive of that approach. https://codereview.chromium.org/2337233004#msg6 > But overriding the methods of baseView was yous idea. I suggested overriding methods after saying "if pendingExitEvent_ really is useless for any situation involving BridgedContentView [then we can override methods]". We found that pendingExitEvent_ is not useless. We did a thorough code review and found that a bad regression would have been caused by removing it. > > https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... > File ui/views/cocoa/bridged_native_widget.mm (right): > > https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { > On 2016/10/27 02:39:10, tapted wrote: > > On 2016/10/26 12:44:56, snake wrote: > > > On 2016/10/26 00:57:00, tapted wrote: > > > > Event handling doesn't belong in BridgedNativeWidget -- please keep it in > > > > BridgedContentView, or explain why it's needed here. > > > > > > BridgedNativeWidget is more appropriate place for processing events, in it > we > > > have capturing info and get captured messages. > > > > What about [BridgedContentView processCapturedMouseEvent]? > > > > BridgedContentView also interacts with Widget already -- it has > > {Set,Release,Has}Capture. > Hm... I do not see any Capturing methods in BridgedContentView code. Is this because you deleted processCapturedMouseEvent in your local branch? > > > BridgedContentView currently has _very_ limited > > interaction with BridgedNativeWidget because their lifetimes are not in step. > > > BridgedNativeWidget owns BridgedContentView, is not it? No. BridgedContentView is an Objective C object. BridgedNativeWidget is a C++ object. A C++ object can only take a reference to an Objective C object. BridgedContentView is _also_ owned by the native NSView hierarchy, and potentially any other class in AppKit that decides to take a reference, such as an NSEvent still sitting in the event queue. So for example, GetBridgeForNativeWindow may return null when called in BridgedContentView. It's possible that will only occur when hostedView_ is also null (I haven't verified this), but it's much easier to rule out the lifetime being a problem by simply handling events in BridgedContentView or things that it has control over. > > > Capture in this sense is a "window" concept -- on other platforms we ask the > OS > > to deliver events to a particular window, but it's still up to the window how > to > > route those events. It's uncommon for the window to handle them itself. > > > We already have helper (CocoaMouseCapture) for this in BridgedNativeWidget (not > in BridgedContentView). This is partly because the lifetime of CocoaMouseCapture needs to be very predictable, so it is tied to a C++ class. The role BridgedNativeWidget plays in the event processing is minimal -- it just passes the event on to BridgedContentView. > > > > It is intersection point of all > > > data, which needs to process events. > > > > This is a bad reason. "intersecting" functionality sounds like the opposite of > > what we want to do in well designed programs. It is better to have more > classes > > with specific functionality, and clear interfaces between them. > > > > BridgedNativeWidget manages the window, not events. If you need to group > > together a set of data to fix this bug, it sounds like it should be a new > class. > > > ok, what about new class NativeEventsRouter ? Let me take a look at the bug tomorrow (it's late here). I don't know yet whether a class is appropriate. "NativeEventsRouter" sounds a lot like what BridgedContentView already is. It inherits from "NSResponder" which Apple documents as "an abstract class that forms the basis of event and command processing in AppKit. The core classes—NSApplication, NSWindow, and NSView—inherit from NSResponder, as must any class that handles events." - https://developer.apple.com/reference/appkit/nsresponder > > > If it's related specifically to dragging, then perhaps > views::DragDropClientMac > > is where it should belong. This class already exists and BridgedContentView > > already has a way to obtain one. But I still do not know whether this is > > necessary. > > > This is related to mouse down/up logic with capturing events (the D&D is special > case of this.) > > > > > But also this hit test > > > > masking stuff doesn't seem related to the rest of the change - please move > > it > > > to > > > > a separate CL. > > > > > > For correctly send MouseExit I should check hit test and widget bounds. Is > not > > > it? > > > > > > > ui::ET_MOUSE_EXITED and ui::ET_MOUSE_ENTERED are generated by > > RootView::OnMouseMoved(const ui::MouseEvent& event) > > > > If we need to generate them a different way on MacViews, then that needs to be > a > > separate bug, and a separate CL. It is not clear to me why this is needed > > currently. Please file a bug. > > Native NSMouseExitEvent should be processed in views::__WIDGET__::OnMouseEvent, > BEFORE AND NOT IN RootView!!! Can you expand on this? It sounds like you're highlighting a bug in the current code which needs to be addressed but isn't captured in your CL description or the bug. > Also this way like on AURA + Windows. Can you link me to the code on Aura that does this? - if there is a pattern we can follow there already, we should reference it and possibly follow it.
On 2016/10/27 12:30:13, tapted wrote: > On 2016/10/27 10:54:22, snake wrote: > > > > >Also you previously had a patch in > > > > >https://codereview.chromium.org/2337233004 > > > > >for http://crbug.com/646792. that bug is marked fixed. > > > > >What's different now? Can > > > > >you file a new bug so that we can get a clear idea whether > > > > >this is the right approach? > > > > > > Please file a new bug with repro steps. I might be able to help you find a > > much > > > simpler fix. > > > > > Ok, i have created new bug for this CL. > > > > > Currently this is adding a lot of complexity. Maybe the fix is something > > simple > > > like having -[BridgedContentView processCapturedMouseEvent:] call a new > > method, > > > -[BaseView clearDrag] > > > > > > > > > With the number of codepaths that you're changing I would say that first we > > > should just cut out BaseView from the hierarchy, or modify it with a neat > way > > to > > > disable some of its functionality. Of course, to keep TooltipBaseView, we > > would > > > need to make that a component rather than a class injected into the > hierarchy. > > > But maybe none of this is needed. Please file a new bug. > > I did try like this in my prev CL https://codereview.chromium.org/2337233004, > > You suggested it, but didn't upload code. I said it is not simple, but I'd be > supportive of that approach. https://codereview.chromium.org/2337233004#msg6 > > > But overriding the methods of baseView was yous idea. > > I suggested overriding methods after saying "if pendingExitEvent_ really is > useless for any situation involving BridgedContentView [then we can override > methods]". We found that pendingExitEvent_ is not useless. We did a thorough > code review and found that a bad regression would have been caused by removing > it. > pendingExitEvent_ is not working correctly fot BridgedContentView (because when BridgedNativeWidget capture mouse events, the mouseUp will be never delivered into BaseView, it will be delivered only in BridgedContentView::processCapturedMouseEvent) > > > > > > https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... > > File ui/views/cocoa/bridged_native_widget.mm (right): > > > > > https://codereview.chromium.org/2448173002/diff/40001/ui/views/cocoa/bridged_... > > ui/views/cocoa/bridged_native_widget.mm:1108: if (widget->HasHitTestMask()) { > > On 2016/10/27 02:39:10, tapted wrote: > > > On 2016/10/26 12:44:56, snake wrote: > > > > On 2016/10/26 00:57:00, tapted wrote: > > > > > Event handling doesn't belong in BridgedNativeWidget -- please keep it > in > > > > > BridgedContentView, or explain why it's needed here. > > > > > > > > BridgedNativeWidget is more appropriate place for processing events, in it > > we > > > > have capturing info and get captured messages. > > > > > > What about [BridgedContentView processCapturedMouseEvent]? > > > > > > BridgedContentView also interacts with Widget already -- it has > > > {Set,Release,Has}Capture. > > Hm... I do not see any Capturing methods in BridgedContentView code. > > Is this because you deleted processCapturedMouseEvent in your local branch? > I say about {Set,Release,Has}Capture, this methods exitsts only in BridgedNativeWidget, not in BridgedContentView, but for correctly send mouseEnter/Exit events, we should do some stuff within this methods. (for example while we have capturing, we should not send mouseEnter/Exit and should send mouseExit, when capturing will be released) > > ....... > BridgedContentView is an Objective C object. BridgedNativeWidget is a C++ > object. A C++ object can only take a reference to an Objective C object. > BridgedContentView is _also_ owned by the native NSView hierarchy, and > potentially any other class in AppKit that decides to take a reference, such as > an NSEvent still sitting in the event queue. > > So for example, GetBridgeForNativeWindow may return null when called in > BridgedContentView. It's possible that will only occur when hostedView_ is also > null (I haven't verified this), but it's much easier to rule out the lifetime > being a problem by simply handling events in BridgedContentView or things that > it has control over. > Got it. > > > > ....... > > This is partly because the lifetime of CocoaMouseCapture needs to be very > predictable, so it is tied to a C++ class. The role BridgedNativeWidget plays in > the event processing is minimal -- it just passes the event on to > BridgedContentView. > Got it. > > > > > > It is intersection point of all > > > > data, which needs to process events. > > > > > > This is a bad reason. "intersecting" functionality sounds like the opposite > of > > > what we want to do in well designed programs. It is better to have more > > classes > > > with specific functionality, and clear interfaces between them. > > > > > > BridgedNativeWidget manages the window, not events. If you need to group > > > together a set of data to fix this bug, it sounds like it should be a new > > class. > > > > > ok, what about new class NativeEventsRouter ? > > Let me take a look at the bug tomorrow (it's late here). I don't know yet > whether a class is appropriate. "NativeEventsRouter" sounds a lot like what > BridgedContentView already is. It inherits from "NSResponder" which Apple > documents as "an abstract class that forms the basis of event and command > processing in AppKit. The core classes—NSApplication, NSWindow, and > NSView—inherit from NSResponder, as must any class that handles events." - > https://developer.apple.com/reference/appkit/nsresponder > ok, I can do this in BridgedContentView, but should transfer capturing notifications from BridgednativeWidged into it (OnAcquireMouseCapture and OnMouseCaptureLost) > > > > > If it's related specifically to dragging, then perhaps > > views::DragDropClientMac > > > is where it should belong. This class already exists and BridgedContentView > > > already has a way to obtain one. But I still do not know whether this is > > > necessary. > > > > > This is related to mouse down/up logic with capturing events (the D&D is > special > > case of this.) > > > > > > > But also this hit test > > > > > masking stuff doesn't seem related to the rest of the change - please > move > > > it > > > > to > > > > > a separate CL. > > > > > > > > For correctly send MouseExit I should check hit test and widget bounds. Is > > not > > > > it? > > > > > > > > > > ui::ET_MOUSE_EXITED and ui::ET_MOUSE_ENTERED are generated by > > > RootView::OnMouseMoved(const ui::MouseEvent& event) > > > > > > If we need to generate them a different way on MacViews, then that needs to > be > > a > > > separate bug, and a separate CL. It is not clear to me why this is needed > > > currently. Please file a bug. > > > > Native NSMouseExitEvent should be processed in > views::__WIDGET__::OnMouseEvent, > > BEFORE AND NOT IN RootView!!! > > Can you expand on this? It sounds like you're highlighting a bug in the current > code which needs to be addressed but isn't captured in your CL description or > the bug. Sample: 1) BridgedContentView receive the NSMouseExit 2) BridgedContentView transfer it into views::Widget (as ui::ET_MOUSE_EXITED) 3) views::Widget processing it and transfer into views::RootView::OnMouseExited. see https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1239 4) views::RootView::OnMouseExited transfer event into target views::View see https://cs.chromium.org/chromium/src/ui/views/widget/root_view.cc?rcl=1477538... This is not bug. We have not other way to detect the moving of cursor outside the NSView/HWND, because no MouseMove events in this case. > > > Also this way like on AURA + Windows. > > Can you link me to the code on Aura that does this? - if there is a pattern we > can follow there already, we should reference it and possibly follow it. For AURA: 1) We generate ui::ET_MouseExit event in https://cs.chromium.org/chromium/src/ui/events/test/event_generator.cc?rcl=14... 2) Then send it into EventProcessor::OnEventFromSource (by call processor->OnEventFromSource ) call deck: https://cs.chromium.org/chromium/src/ui/events/test/event_generator.cc?rcl=14... https://cs.chromium.org/chromium/src/ui/events/test/events_test_utils.cc?rcl=... https://cs.chromium.org/chromium/src/ui/events/event_source.cc?rcl=1477538644... https://cs.chromium.org/chromium/src/ui/events/event_source.cc?rcl=1477538644... 3) Then send it into views::Widget::OnMouseEvent: call deck (too much to use links): ui::EventSource::SendEventToProcessor ui::EventSource::DeliverEventToProcessor ui::EventProcessor::OnEventFromSource ui::EventDispatcherDelegate::DispatchEvent aura::WindowEventDispatcher::PreDispatchEvent aura::WindowEventDispatcher::PreDispatchMouseEvent aura::WindowEventDispatcher::DispatchMouseEnterOrExit ui::EventDispatcherDelegate::DispatchEvent ui::EventDispatcherDelegate::DispatchEventToTarget ui::EventDispatcher::ProcessEvent ui::EventDispatcher::DispatchEvent ui::EventHandler::OnEvent views::NativeWidgetAura::OnMouseEvent views::Widget::OnMouseEvent 4) It is processd in views::Widget https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1239 (like in MacViews with my CL) 5) And now it is delivered into views::RootView https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1241 5) And processed in views::RootView https://cs.chromium.org/chromium/src/ui/views/widget/root_view.cc?rcl=1477538...
> > > > ui::ET_MOUSE_EXITED and ui::ET_MOUSE_ENTERED are generated by > > > > RootView::OnMouseMoved(const ui::MouseEvent& event) > > > > > > > > If we need to generate them a different way on MacViews, then that needs > to > > be > > > a > > > > separate bug, and a separate CL. It is not clear to me why this is needed > > > > currently. Please file a bug. > > > > > > Native NSMouseExitEvent should be processed in > > views::__WIDGET__::OnMouseEvent, > > > BEFORE AND NOT IN RootView!!! > > > > Can you expand on this? It sounds like you're highlighting a bug in the > current > > code which needs to be addressed but isn't captured in your CL description or > > the bug. > Sample: > 1) BridgedContentView receive the NSMouseExit > 2) BridgedContentView transfer it into views::Widget (as ui::ET_MOUSE_EXITED) > 3) views::Widget processing it and transfer into views::RootView::OnMouseExited. > see https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1239 > 4) views::RootView::OnMouseExited transfer event into target views::View > see > https://cs.chromium.org/chromium/src/ui/views/widget/root_view.cc?rcl=1477538... > > This is not bug. We have not other way to detect the moving of cursor outside > the NSView/HWND, because no MouseMove events in this case. I left a comment on http://crbug.com/659959 - NSTrackingMouseEnteredAndExited will hopefully fix this. > > > > > > Also this way like on AURA + Windows. > > > > Can you link me to the code on Aura that does this? - if there is a pattern we > > can follow there already, we should reference it and possibly follow it. > > For AURA: > 1) We generate ui::ET_MouseExit event in > https://cs.chromium.org/chromium/src/ui/events/test/event_generator.cc?rcl=14... > 2) Then send it into EventProcessor::OnEventFromSource (by call > processor->OnEventFromSource ) > call deck: > > https://cs.chromium.org/chromium/src/ui/events/test/event_generator.cc?rcl=14... > > https://cs.chromium.org/chromium/src/ui/events/test/events_test_utils.cc?rcl=... > > https://cs.chromium.org/chromium/src/ui/events/event_source.cc?rcl=1477538644... > > https://cs.chromium.org/chromium/src/ui/events/event_source.cc?rcl=1477538644... > 3) Then send it into views::Widget::OnMouseEvent: > call deck (too much to use links): > ui::EventSource::SendEventToProcessor > ui::EventSource::DeliverEventToProcessor > ui::EventProcessor::OnEventFromSource > ui::EventDispatcherDelegate::DispatchEvent > aura::WindowEventDispatcher::PreDispatchEvent > aura::WindowEventDispatcher::PreDispatchMouseEvent > aura::WindowEventDispatcher::DispatchMouseEnterOrExit > ui::EventDispatcherDelegate::DispatchEvent > ui::EventDispatcherDelegate::DispatchEventToTarget > ui::EventDispatcher::ProcessEvent > ui::EventDispatcher::DispatchEvent > ui::EventHandler::OnEvent > views::NativeWidgetAura::OnMouseEvent > views::Widget::OnMouseEvent > 4) It is processd in views::Widget > https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1239 > (like in MacViews with my CL) > 5) And now it is delivered into views::RootView > https://cs.chromium.org/chromium/src/ui/views/widget/widget.cc?rcl=0&l=1241 > 5) And processed in views::RootView > > https://cs.chromium.org/chromium/src/ui/views/widget/root_view.cc?rcl=1477538... We shouldn't rely too much on what EventGenerator does -- it's only used for tests, and only as-needed - it is very ad-hoc and not trying to simulate real event dispatch - just enough to simplify the job of getting test events going to particular Views.
> We shouldn't rely too much on what EventGenerator does -- it's only used for > tests, and only as-needed - it is very ad-hoc and not trying to simulate real > event dispatch - just enough to simplify the job of getting test events going to > particular Views. Just another way: I have created new CL with crossplatform test for Views: https://codereview.chromium.org/2459013002/ It is crashed under MacViews, without changes in event_generator_delegate_mac.mm. But success under AURA. Please look on it.
NSTrackingMouseEnteredAndExited added in https://codereview.chromium.org/2466263006/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
