Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1815563002:
MacViews: Enable acceptsFirstMouse for BridgedContentView. (Closed)
DescriptionMacViews: Enable acceptsFirstMouse for BridgedContentView.
This allows an initial mouse click on an inactive window to be accepted
immediately, so two clicks are not required (i.e. one to activate the view and
the second to send the mouse event to the view itself).
See linked bugs for certificate viewer and tab dragging, both which require
this to be enabled.
BUG=594079, 587239
Committed: https://crrev.com/2d62862c2faaa841bbeaf9a455df499e62c26476
Cr-Commit-Position: refs/heads/master@{#382240}
Patch Set 1 #Patch Set 2 : Linting, update comment. #
Total comments: 8
Patch Set 3 : Address review comments. #Messages
Total messages: 13 (3 generated)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
PTAL, thanks :)
https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:871: // To maximise consistency with the Cocoa browser (mac_views_browser=0), accept nit: maximize https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:872: // mouse clicks immediately so that clicking Chrome from an inactive window will `clicking Chrome from an` -> `clicking on` https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:873: // allow the event to be processed. processed, rather than merely activate the window. https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:874: - (BOOL)acceptsFirstMouse:(NSEvent*)theEvent { So for ordering, this should come after // NSView implementation. since it's overriding something from NSView. Then, we generally try to follow the declaration order from the class that's being overridden. For acceptsFirstMouse, that's between - (void)drawRect: and - (NSTextInputContext*)inputContext
Thanks, all fixed! https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:871: // To maximise consistency with the Cocoa browser (mac_views_browser=0), accept On 2016/03/17 23:57:35, tapted wrote: > nit: maximize Done. https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:872: // mouse clicks immediately so that clicking Chrome from an inactive window will On 2016/03/17 23:57:35, tapted wrote: > `clicking Chrome from an` -> `clicking on` Done. https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:873: // allow the event to be processed. On 2016/03/17 23:57:34, tapted wrote: > processed, rather than merely activate the window. Done. https://codereview.chromium.org/1815563002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:874: - (BOOL)acceptsFirstMouse:(NSEvent*)theEvent { On 2016/03/17 23:57:35, tapted wrote: > So for ordering, this should come after // NSView implementation. since it's > overriding something from NSView. Then, we generally try to follow the > declaration order from the class that's being overridden. > > For acceptsFirstMouse, that's between - (void)drawRect: and - > (NSTextInputContext*)inputContext Done.
lgtm
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1815563002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815563002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Enable acceptsFirstMouse for BridgedContentView. This allows an initial mouse click on an inactive window to be accepted immediately, so two clicks are not required (i.e. one to activate the view and the second to send the mouse event to the view itself). See linked bugs for certificate viewer and tab dragging, both which require this to be enabled. BUG=594079,587239 ========== to ========== MacViews: Enable acceptsFirstMouse for BridgedContentView. This allows an initial mouse click on an inactive window to be accepted immediately, so two clicks are not required (i.e. one to activate the view and the second to send the mouse event to the view itself). See linked bugs for certificate viewer and tab dragging, both which require this to be enabled. BUG=594079,587239 Committed: https://crrev.com/2d62862c2faaa841bbeaf9a455df499e62c26476 Cr-Commit-Position: refs/heads/master@{#382240} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2d62862c2faaa841bbeaf9a455df499e62c26476 Cr-Commit-Position: refs/heads/master@{#382240}
Message was sent while issue was closed.
Citing the comment in committed diff: > To maximize consistency with the Cocoa browser (mac_views_browser=0), accept > mouse clicks immediately so that clicking on Chrome from an inactive window > will allow the event to be processed, rather than merely activate the window. But this is not really consistent with the Cocoa browser, as acceptsFirstMouse: is enabled only for some views: RenderWidgetHostViewCocoa, TabView, TabStripView, PanelTitlebarViewCocoa and OneClickHyperlinkTextView. And by default when you display a normal webpage in a tab, make the browser inactive, click the link — there will be no navigation on the first click: https://www.dropbox.com/s/dfk4asqja2dypqk/2016-03-21-inactive-link-clicking.m... Do you plan to address this MacViews browser?
Message was sent while issue was closed.
On 2016/03/21 09:27:18, themblsha wrote: > Citing the comment in committed diff: > > To maximize consistency with the Cocoa browser (mac_views_browser=0), accept > > mouse clicks immediately so that clicking on Chrome from an inactive window > > will allow the event to be processed, rather than merely activate the window. > > But this is not really consistent with the Cocoa browser, as acceptsFirstMouse: > is enabled only for some views: RenderWidgetHostViewCocoa, TabView, > TabStripView, PanelTitlebarViewCocoa and OneClickHyperlinkTextView. > > And by default when you display a normal webpage in a tab, make the browser > inactive, click the link — there will be no navigation on the first click: > > https://www.dropbox.com/s/dfk4asqja2dypqk/2016-03-21-inactive-link-clicking.m... > > Do you plan to address this MacViews browser? I don't think there's any regression here, but please do try it out, and let us know if you notice anything strange. Currently, MacViewsBrowser still uses RenderWidgetHostViewCocao; RWHVC is responsible for handling clicks on links in the content area, so that behaviour should be unchanged. Then, the other views describe the bulk of the UI in Chrome. So, it seems simpler at this point to switch modes and then see if we need special cases later.
Message was sent while issue was closed.
On 2016/03/22 05:26:20, tapted wrote: > Currently, MacViewsBrowser still uses RenderWidgetHostViewCocao; RWHVC is > responsible for handling clicks on links in the content area, so that behaviour > should be unchanged. Then, the other views describe the bulk of the UI in > Chrome. So, it seems simpler at this point to switch modes and then see if we > need special cases later. Whoa, clicking on links in MacViews works the same as in Cocoa. So it'll be fine until Chromium switches away from RWHVC :) |
