Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475173002:
MacViews: Fix window dragging on Sierra. (Closed)
|
Created:
4 years, 1 month ago by karandeepb Modified:
4 years, 1 month ago Reviewers:
tapted CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix window dragging on Sierra.
To allow window dragging on custom window regions on MacViews, an event monitor
is currently used. The event monitor looks for any mouse down events which
should cause window dragging, makes the window draggable and then reposts these
events. Since AppKit caches the draggable regions, we use a hack to invalidate
this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra.
To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:]
introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is
introduced which returns true if the drag event monitor approach should be used
when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified
to intercept mouse events and perform window dragging when necessary.
Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag
event monitor is used and when it isn't).
BUG=660270
TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install
https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngnaofnfjpajdcbdmkegiakec/.
Add a titlebar (draggable region) and ensure window dragging works.
TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is
draggable.
Committed: https://crrev.com/d22dbe7d44e81b40c2c1415a13ff7965693150ed
Cr-Commit-Position: refs/heads/master@{#431166}
Patch Set 1 : -- #Patch Set 2 : Tests #
Total comments: 16
Patch Set 3 : Address comments. #
Total comments: 3
Patch Set 4 : Address comments. #Patch Set 5 : Rebase. Update comment. #include=>#import. #
Messages
Total messages: 38 (25 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Sierra window drag. ========== to ========== MacViews: Fix window dragging on Sierra. BUG=660270 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
Can you give this a quick look Trent. Will try to add a test. There's already BridgedNativeWidgetUITest.HitTest, but it's quite broken even on OS < 10.12. https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (left): https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:177: [delegate shouldRepostPendingLeftMouseDown:[ns_event locationInWindow]]; Don't think this should be routed through the window delegate.
looks pretty neat - nothing really stood out https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/native_... File ui/views/cocoa/native_widget_mac_nswindow.mm (right): https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/native_... ui/views/cocoa/native_widget_mac_nswindow.mm:124: DCHECK(bridge); I don't think we can guarantee that the window hasn't been torn down while |event| was sitting in the event queue
Patchset #3 (id:160001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:240001) has been deleted
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
Description was changed from ========== MacViews: Fix window dragging on Sierra. BUG=660270 ========== to ========== MacViews: Fix window dragging on Sierra. To allow window dragging on custom window regions on MacViews, an event monitor is currently used. The event monitor looks for any mouse down events which should cause window dragging, makes the window draggable and then reposts these events. Since AppKit caches the draggable regions, we use a hack to invalidate this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra. To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:] introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is introduced which returns true if the drag event monitor approach should be used when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified to intercept mouse events and perform window dragging when necessary. Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag event monitor is used and when it isn't). BUG=660270 TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngna.... Add a titlebar (draggable region) and ensure window dragging works. TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is draggable. ==========
On 2016/11/04 04:58:54, tapted wrote: > looks pretty neat - nothing really stood out > > https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/native_... > File ui/views/cocoa/native_widget_mac_nswindow.mm (right): > > https://codereview.chromium.org/2475173002/diff/140001/ui/views/cocoa/native_... > ui/views/cocoa/native_widget_mac_nswindow.mm:124: DCHECK(bridge); > I don't think we can guarantee that the window hasn't been torn down while > |event| was sitting in the event queue Done. Looks like I accidentally deleted the patch with your comment.
PTAL Trent. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; What would be a good workaround to avoid this, since this may bite us while adding tests in future as well. One alternative is to not do the visibility suppression for tests using the UNIT_TEST macro. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:503: EXPECT_FALSE([ns_observer wait]); This causes a RunLoop to run till the timeout. Can remove this but then we are not testing anything since it will pass even with HTCAPTION.
https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:908: HTCAPTION) { nit: no curlies https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:918: NSPointInRect(location_in_window, [subview frame])) { nit: no curlies https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; On 2016/11/08 02:59:03, karandeepb wrote: > What would be a good workaround to avoid this, since this may bite us while > adding tests in future as well. One alternative is to not do the visibility > suppression for tests using the UNIT_TEST macro. I think if WindowResizeHelperMac::Get()->task_runner() returns null then we shouldn't do the delayed-visibility stuff. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:367: base::scoped_nsobject<WindowedNSNotificationObserver> ns_observer( maybe we want to rename this to, e.g., move_observer -- it's easy to lose track of what it's waiting for later on https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:503: EXPECT_FALSE([ns_observer wait]); On 2016/11/08 02:59:03, karandeepb wrote: > This causes a RunLoop to run till the timeout. Can remove this but then we are > not testing anything since it will pass even with HTCAPTION. Hrm. Doesn't that make this effectively ::sleep(test_timeouts::action_timeout()) then? Does base::RunLoop().RunUntilIdle() work instead? Or can you just insert an unrelated event into the queue and wait for it to be processed? (i.e. assume that an event processed after means all events that were in the queue before it have been processed).
Patchset #3 (id:320001) has been deleted
PTAL Trent. I am not really sure about the test though. It hangs/flakes for me sometimes on Sierra (with the window not receiving the initial mouse down), but I can't reproduce it consistently to investigate. It works ok on 10.11 for both cases (when the drag event monitor is being used and when it isn't). https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:908: HTCAPTION) { On 2016/11/08 05:03:16, tapted wrote: > nit: no curlies Done. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:918: NSPointInRect(location_in_window, [subview frame])) { On 2016/11/08 05:03:16, tapted wrote: > nit: no curlies Done. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; On 2016/11/08 05:03:17, tapted wrote: > On 2016/11/08 02:59:03, karandeepb wrote: > > What would be a good workaround to avoid this, since this may bite us while > > adding tests in future as well. One alternative is to not do the visibility > > suppression for tests using the UNIT_TEST macro. > > I think if WindowResizeHelperMac::Get()->task_runner() returns null then we > shouldn't do the delayed-visibility stuff. Weirdly, even after commenting this out, I can't seem to remove the [window setIgnoresMouseEvents: NO] call in the test. That is, even with an initial value of NO for ignoresMouseEvents, I still need the [window setIgnoresMouseEvents: NO] call in the test. So somehow the call is having some side effect, which is necessary for the test. This is also true for making it pass on master. I found this http://www.cocoabuilder.com/archive/cocoa/313246-transparent-window-not-getti... but the call is still needed when the window is opaque and has alpha value of 1. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:367: base::scoped_nsobject<WindowedNSNotificationObserver> ns_observer( On 2016/11/08 05:03:17, tapted wrote: > maybe we want to rename this to, e.g., move_observer -- it's easy to lose track > of what it's waiting for later on Done. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:503: EXPECT_FALSE([ns_observer wait]); On 2016/11/08 05:03:17, tapted wrote: > On 2016/11/08 02:59:03, karandeepb wrote: > > This causes a RunLoop to run till the timeout. Can remove this but then we are > > not testing anything since it will pass even with HTCAPTION. > > Hrm. Doesn't that make this effectively ::sleep(test_timeouts::action_timeout()) > then? Yeah it does. > Does base::RunLoop().RunUntilIdle() work instead? > > Or can you just insert an unrelated event into the queue and wait for it to be > processed? (i.e. assume that an event processed after means all events that were > in the queue before it have been processed). Both of these didn't work. However, I am now using the NSWindowWillMoveNotification count. Hence the test will fail if we were to use HTCAPTION above. https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... File ui/views/cocoa/native_widget_mac_nswindow.mm (right): https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... ui/views/cocoa/native_widget_mac_nswindow.mm:128: [[NSNotificationCenter defaultCenter] Have added this notification. I am not sure if it's considered ok to send system notifications, but it helps with the tests and makes things consistent.
https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; On 2016/11/09 04:44:20, karandeepb wrote: > On 2016/11/08 05:03:17, tapted wrote: > > On 2016/11/08 02:59:03, karandeepb wrote: > > > What would be a good workaround to avoid this, since this may bite us while > > > adding tests in future as well. One alternative is to not do the visibility > > > suppression for tests using the UNIT_TEST macro. > > > > I think if WindowResizeHelperMac::Get()->task_runner() returns null then we > > shouldn't do the delayed-visibility stuff. > > Weirdly, even after commenting this out, I can't seem to remove the [window > setIgnoresMouseEvents: NO] call in the test. That is, even with an initial value > of NO for ignoresMouseEvents, I still need the [window setIgnoresMouseEvents: > NO] call in the test. So somehow the call is having some side effect, which is > necessary for the test. This is also true for making it pass on master. > > I found this > http://www.cocoabuilder.com/archive/cocoa/313246-transparent-window-not-getti... > but the call is still needed when the window is opaque and has alpha value of 1. yah - OSX uses the alpha channel of the window for hit testing. I don't have an explanation for why the event isn't getting through when there is no alpha though. Perhaps the CALayer's alpha is influencing the hit testing as well as the window alpha. (waiting for a frame may fix this, but we can't do that without a GPU process. We could also set the layer's background color to white instead of clear so we don't need to wait for an opaque frame -- something like [[BridgedNativeWidget::compositor_superview_ layer] setBackgroundColor:CGColorGetConstantColor(kCGColorWhite];). If we can't get to the bottom of it, can you file a bug about this and update the comment above? (E.g. maybe we should still check WindowResizeHelperMac::Get()->task_runner() in SetVisibilityState() - there might be surprises lurking there for tests later on). https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... File ui/views/cocoa/native_widget_mac_nswindow.mm (right): https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... ui/views/cocoa/native_widget_mac_nswindow.mm:128: [[NSNotificationCenter defaultCenter] On 2016/11/09 04:44:20, karandeepb wrote: > Have added this notification. I am not sure if it's considered ok to send system > notifications, but it helps with the tests and makes things consistent. Ooh nice - this seems legit. So long as we track whether AppKit decides to start sending them later, which I think your test does.
PTAL Trent. https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; >>yah - OSX uses the alpha channel of the window for hit testing. I don't have an >>explanation for why the event isn't getting through when there is no alpha >>though. Perhaps the CALayer's alpha is influencing the hit testing as well as >>the window alpha. (waiting for a frame may fix this, but we can't do that >>without a GPU process. We could also set the layer's background color to white >>instead of clear so we don't need to wait for an opaque frame -- something like >>[[BridgedNativeWidget::compositor_superview_ layer] >>setBackgroundColor:CGColorGetConstantColor(kCGColorWhite];). >> >>If we can't get to the bottom of it, can you file a bug about this and update >>the comment above? Yeah, you are correct. CALayer's alpha is also influencing hit testing. Updated the comment to reflect the same. >>(E.g. maybe we should still check WindowResizeHelperMac::Get()->task_runner() in >>SetVisibilityState() - there might be surprises lurking there for tests later >>on). Done. Although it seems a bit weird to use WindowResizeHelperMac when our end goal is to just disable initial visibility suppression during tests. Also, due to the CALayer being transparent, we would need an explicit [window setIgnoresMouseEvents: NO] in the tests anyway for cases where we send events directly to the window server. It may be useful to have a method called EnableMouseEventsForTesting on the BNW since this may cause confusion while writing tests in the future. https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... File ui/views/cocoa/native_widget_mac_nswindow.mm (right): https://codereview.chromium.org/2475173002/diff/340001/ui/views/cocoa/native_... ui/views/cocoa/native_widget_mac_nswindow.mm:128: [[NSNotificationCenter defaultCenter] On 2016/11/09 05:06:56, tapted wrote: > On 2016/11/09 04:44:20, karandeepb wrote: > > Have added this notification. I am not sure if it's considered ok to send > system > > notifications, but it helps with the tests and makes things consistent. > > Ooh nice - this seems legit. So long as we track whether AppKit decides to start > sending them later, which I think your test does. Yes correct.
lgtm https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; On 2016/11/09 06:33:22, karandeepb wrote: > >>yah - OSX uses the alpha channel of the window for hit testing. I don't have > an > >>explanation for why the event isn't getting through when there is no alpha > >>though. Perhaps the CALayer's alpha is influencing the hit testing as well as > >>the window alpha. (waiting for a frame may fix this, but we can't do that > >>without a GPU process. We could also set the layer's background color to white > >>instead of clear so we don't need to wait for an opaque frame -- something > like > >>[[BridgedNativeWidget::compositor_superview_ layer] > >>setBackgroundColor:CGColorGetConstantColor(kCGColorWhite];). > >> > >>If we can't get to the bottom of it, can you file a bug about this and update > >>the comment above? > > Yeah, you are correct. CALayer's alpha is also influencing hit testing. Updated > the comment to reflect the same. > > >>(E.g. maybe we should still check WindowResizeHelperMac::Get()->task_runner() > in > >>SetVisibilityState() - there might be surprises lurking there for tests later > >>on). > > Done. Although it seems a bit weird to use WindowResizeHelperMac when our end > goal is to just disable initial visibility suppression during tests. No task runner means no GPU process, which could happen for other reasons, and doesn't always happen in tests (e.g. browser_tests will have a task_runner). E.g. it could also happen if we get the in-process context factory working, and views_examples_exe (instead of just views_examples_with_content_exe). > > Also, due to the CALayer being transparent, we would need an explicit [window > setIgnoresMouseEvents: NO] in the tests anyway for cases where we send events > directly to the window server. It may be useful to have a method called > EnableMouseEventsForTesting on the BNW since this may cause confusion while > writing tests in the future. I think this is OK - this test is kinda special. Things like ui::EventGenerator don't rely on the window server to dispatch events.
Patchset #5 (id:380001) has been deleted
https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right): https://codereview.chromium.org/2475173002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:339: [window setIgnoresMouseEvents:NO]; On 2016/11/10 00:43:20, tapted wrote: > On 2016/11/09 06:33:22, karandeepb wrote: > > >>yah - OSX uses the alpha channel of the window for hit testing. I don't have > > an > > >>explanation for why the event isn't getting through when there is no alpha > > >>though. Perhaps the CALayer's alpha is influencing the hit testing as well > as > > >>the window alpha. (waiting for a frame may fix this, but we can't do that > > >>without a GPU process. We could also set the layer's background color to > white > > >>instead of clear so we don't need to wait for an opaque frame -- something > > like > > >>[[BridgedNativeWidget::compositor_superview_ layer] > > >>setBackgroundColor:CGColorGetConstantColor(kCGColorWhite];). > > >> > > >>If we can't get to the bottom of it, can you file a bug about this and > update > > >>the comment above? > > > > Yeah, you are correct. CALayer's alpha is also influencing hit testing. > Updated > > the comment to reflect the same. > > > > >>(E.g. maybe we should still check > WindowResizeHelperMac::Get()->task_runner() > > in > > >>SetVisibilityState() - there might be surprises lurking there for tests > later > > >>on). > > > > Done. Although it seems a bit weird to use WindowResizeHelperMac when our end > > goal is to just disable initial visibility suppression during tests. > > No task runner means no GPU process, which could happen for other reasons, and > doesn't always happen in tests (e.g. browser_tests will have a task_runner). > E.g. it could also happen if we get the in-process context factory working, and > views_examples_exe (instead of just views_examples_with_content_exe). > > > > > Also, due to the CALayer being transparent, we would need an explicit [window > > setIgnoresMouseEvents: NO] in the tests anyway for cases where we send events > > directly to the window server. It may be useful to have a method called > > EnableMouseEventsForTesting on the BNW since this may cause confusion while > > writing tests in the future. > > I think this is OK - this test is kinda special. Things like ui::EventGenerator > don't rely on the window server to dispatch events. Acknowledged. Also, updated the comment regarding WindowResizeHelperMac.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2475173002/#ps400001 (title: "Rebase. Update comment. #include=>#import.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix window dragging on Sierra. To allow window dragging on custom window regions on MacViews, an event monitor is currently used. The event monitor looks for any mouse down events which should cause window dragging, makes the window draggable and then reposts these events. Since AppKit caches the draggable regions, we use a hack to invalidate this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra. To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:] introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is introduced which returns true if the drag event monitor approach should be used when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified to intercept mouse events and perform window dragging when necessary. Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag event monitor is used and when it isn't). BUG=660270 TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngna.... Add a titlebar (draggable region) and ensure window dragging works. TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is draggable. ========== to ========== MacViews: Fix window dragging on Sierra. To allow window dragging on custom window regions on MacViews, an event monitor is currently used. The event monitor looks for any mouse down events which should cause window dragging, makes the window draggable and then reposts these events. Since AppKit caches the draggable regions, we use a hack to invalidate this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra. To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:] introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is introduced which returns true if the drag event monitor approach should be used when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified to intercept mouse events and perform window dragging when necessary. Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag event monitor is used and when it isn't). BUG=660270 TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngna.... Add a titlebar (draggable region) and ensure window dragging works. TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is draggable. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix window dragging on Sierra. To allow window dragging on custom window regions on MacViews, an event monitor is currently used. The event monitor looks for any mouse down events which should cause window dragging, makes the window draggable and then reposts these events. Since AppKit caches the draggable regions, we use a hack to invalidate this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra. To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:] introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is introduced which returns true if the drag event monitor approach should be used when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified to intercept mouse events and perform window dragging when necessary. Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag event monitor is used and when it isn't). BUG=660270 TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngna.... Add a titlebar (draggable region) and ensure window dragging works. TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is draggable. ========== to ========== MacViews: Fix window dragging on Sierra. To allow window dragging on custom window regions on MacViews, an event monitor is currently used. The event monitor looks for any mouse down events which should cause window dragging, makes the window draggable and then reposts these events. Since AppKit caches the draggable regions, we use a hack to invalidate this cache in BridgedNativeWidget::SetDraggable. However, this broke on Sierra. To fix, use the new window dragging API [NSWindow performWindowDragWithEvent:] introduced in Mac OS 10.11. BridgedNativeWidget::ShouldUseDragEventMonitor is introduced which returns true if the drag event monitor approach should be used when the API is not available. NativeWidgetMacNSWindow's sendEvent is modified to intercept mouse events and perform window dragging when necessary. Also, BridgedNativeWidgetUITest.HitTest is fixed for both the cases (when a drag event monitor is used and when it isn't). BUG=660270 TEST=On Sierra, enable chrome://flags/#mac-views-native-app-windows. Install https://chrome.google.com/webstore/detail/frameless-window-sample/hjjdaddngna.... Add a titlebar (draggable region) and ensure window dragging works. TEST=Build with mac_views_browser=true on Sierra. Ensure the browser window is draggable. Committed: https://crrev.com/d22dbe7d44e81b40c2c1415a13ff7965693150ed Cr-Commit-Position: refs/heads/master@{#431166} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d22dbe7d44e81b40c2c1415a13ff7965693150ed Cr-Commit-Position: refs/heads/master@{#431166} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
