Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422993002:
views::Label: Implement context menu, keyboard shortcuts for copy/select all. (Closed)
|
Created:
4 years, 2 months ago by karandeepb Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews::Label: Implement context menu, keyboard shortcuts for copy/select all.
This CL is a follow-up to r430461 which implemented text selection for Labels.
This CL implements the following for Labels-
- Copy selected text on [Ctrl+C] or [Ctrl+Insert].
- Select all text on [Ctrl+A].
- Context Menu with the options Copy and Select All.
- Copy selected text using the Browser menu.
BridgedContentView's validateUserInterfaceItem method is also modified.
Currently it returns YES only when there is an active TextInputClient which can
perform the given command. But since views::Label does not implement the
TextInputClient interface, the method is modified to return YES in case the
focused view is a Label for the Copy and Select All commands. This is necessary
for Copy and Select All keyboard shortcuts and browser menu commands to work on
MacViews for labels.
Some other changes-
- The stored selection range is now correctly invalidated in Label::SetText and
Label::SetSelectable.
- Text selection is disabled for obscured labels.
BUG=630365, 437993
Committed: https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a
Cr-Commit-Position: refs/heads/master@{#432720}
Patch Set 1 : -- #
Total comments: 44
Patch Set 2 : Address comments. #Patch Set 3 : Rebase. #Patch Set 4 : Tests. Stored selection range invalidation #
Total comments: 33
Patch Set 5 : Address comments by msw@. #Patch Set 6 : Disable text selection for obscured labels. #Patch Set 7 : Address comments by Trent. #
Total comments: 12
Patch Set 8 : Address comments. #
Depends on Patchset: Messages
Total messages: 64 (47 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:150001) has been deleted
Description was changed from ========== Label selection copy. ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Undo, Cut, Copy, Paste, Delete, Select All. Of these only Copy and SelectAll become enabled. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view can handle accelerators, in case no TextInputClient is active. This is necessary for Copy and SelectAll keyboard shortcuts and browser commands to work on MacViews. BUG=630365, 437993 ==========
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@ for ui/views/controls. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:1020: context_menu_contents_.reset(new ui::SimpleMenuModel(this)); Do we want to show only Copy/SelectAll in the context menu for Labels or is this ok?
Will add tests in a subsequent patch.
Cool, I'm looking forward to some tests. https://codereview.chromium.org/2422993002/diff/170001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (left): https://codereview.chromium.org/2422993002/diff/170001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:523: // This DCHECK is more strict than a similar check in handleAction:. It can be Someone more familiar with Mac should review this CL too. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:550: // The only accelerator a Label needs to handle is the Copy command from the Not CTRL+A for select all? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:552: if(accelerator.key_code() == ui::VKEY_C && accelerator.IsCtrlDown()) { nit: space after if, git cl format https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:560: // See related comment in BrowserView::CutCopyPaste. nit: please explain a bit more here; I'm not sure what I'm supposed to be getting from that function's comment. (eg. should that explain why the returned value depends on focus, or something?) https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:565: bool Label::OnKeyPressed(const ui::KeyEvent& event) { Should this use Textfield's GetCommandForKeyEvent and then just filter the supported commands? I worry about forked/duplicated logic drifting that should probably be consistent. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:575: if (control && !alt && HasSelection()) { nit && !obscured()? here and below? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:589: if (HasSelection()) Should this be a DCHECK given the !text().empty() conditional above? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:590: UpdateSelectionClipboard(); Should this be a part of SelectAll? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:643: bool Label::GetAcceleratorForCommandId(int command_id, nit: Share a helper with textfield if feasible. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:678: context_menu_runner_.reset(new MenuRunner(context_menu_contents_.get(), Would it suffice to initialize the runner once in/after BuildContextMenuContents? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:682: ignore_result(context_menu_runner_->RunMenuAt( Perhaps we should check that the result isn't MENU_DELETED? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:791: // This allows the BrowserView to pass the copy command from the Chrome menu Should we also add CTRL+A and Ctrl+Insert? It seems odd to only add one accelerator, and just for the purpose of supporting some BrowserView behavior. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:1020: context_menu_contents_.reset(new ui::SimpleMenuModel(this)); On 2016/11/09 10:20:00, karandeepb wrote: > Do we want to show only Copy/SelectAll in the context menu for Labels or is this > ok? Yeah, I think we should only show copy and select all. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:26: public ui::SimpleMenuModel::Delegate { It would be nice to use composition instead of inheritance, and have a shared context menu model/controller class for Textfield and Label, but I don't know if that's really feasible. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:219: bool OnKeyPressed(const ui::KeyEvent& event) override; nit: match the order in view.h https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:230: // ui::SimpleMenuModel::Delegate overrides: nit: try to keep the same order as the subclass list. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:343: std::unique_ptr<ui::SimpleMenuModel> context_menu_contents_; nit: avoid unique_ptr, use plain member, if possible. (but I guess this follows Textfield, so it's okay as-is too)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:210001) has been deleted
Patchset #4 (id:250001) 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 #4 (id:270001) has been deleted
Patchset #4 (id:290001) has been deleted
Patchset #4 (id:310001) 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...
Description was changed from ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Undo, Cut, Copy, Paste, Delete, Select All. Of these only Copy and SelectAll become enabled. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view can handle accelerators, in case no TextInputClient is active. This is necessary for Copy and SelectAll keyboard shortcuts and browser commands to work on MacViews. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view can handle accelerators, in case no TextInputClient is active. This is necessary for Copy and SelectAll keyboard shortcuts and browser commands to work on MacViews. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ==========
PTAL msw@. Thanks. https://codereview.chromium.org/2422993002/diff/170001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (left): https://codereview.chromium.org/2422993002/diff/170001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:523: // This DCHECK is more strict than a similar check in handleAction:. It can be On 2016/11/09 18:32:52, msw wrote: > Someone more familiar with Mac should review this CL too. Yeah I'll get Trent to review this. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:550: // The only accelerator a Label needs to handle is the Copy command from the On 2016/11/09 18:32:52, msw wrote: > Not CTRL+A for select all? My understanding of accelerators is that they are to be used when a view wants to perform some action on a key event even if it's not focused (hence generally to be used for keyboard shortcuts). The only reason for handling Ctrl+C as an accelerator in the Label class is to get Copy from the main Chrome menu (I think it has a specific name but not sure, it's the one invoked from the button with three dots) working. See also the Textfield class, where we add accelerators for Cut/Copy/Paste, again so that these actions from the Chrome menu are handled. See https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi.... for more details. Since SelectAll is not an action in that menu, it does not need to be handled via an accelerator. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:552: if(accelerator.key_code() == ui::VKEY_C && accelerator.IsCtrlDown()) { On 2016/11/09 18:32:52, msw wrote: > nit: space after if, git cl format Done. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:560: // See related comment in BrowserView::CutCopyPaste. On 2016/11/09 18:32:53, msw wrote: > nit: please explain a bit more here; I'm not sure what I'm supposed to be > getting from that function's comment. (eg. should that explain why the returned > value depends on focus, or something?) Done. I was referring to the following comment in the function- // Any Views which want to handle the clipboard commands in the Chrome menu // should: // (a) Register ctrl-X/C/V as accelerators // (b) Implement CanHandleAccelerators() to not return true unless they're // focused, as the FocusManager will try all registered accelerator // handlers, not just the focused one. // Currently, Textfield (which covers the omnibox and find bar, and likely any // other native UI in the future that wants to deal with clipboard commands) // does the above. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:565: bool Label::OnKeyPressed(const ui::KeyEvent& event) { On 2016/11/09 18:32:52, msw wrote: > Should this use Textfield's GetCommandForKeyEvent and then just filter the > supported commands? I worry about forked/duplicated logic drifting that should > probably be consistent. Currently the TextEditCommand enum is only being used by TextInputClient and subclasses. If you think it's better, I can move Textfield's GetCommandForKeyEvent into a separate file as a free function. Maybe a file called text_utils.h in ui/views. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:575: if (control && !alt && HasSelection()) { On 2016/11/09 18:32:53, msw wrote: > nit && !obscured()? here and below? But shouldn't we consume the event (by returning true) when the Label has an active selection but has obscured text. Otherwise the event would propagate to the parent view. FWIW, the textfield class returns false when the textfield does not have an active selection or when it has obscured text, for Ctrl+C. Hence the event propagates to the parent view for the active selection but obscured text case, which is probably incorrect. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:589: if (HasSelection()) On 2016/11/09 18:32:53, msw wrote: > Should this be a DCHECK given the !text().empty() conditional above? Oh yeah, done. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:590: UpdateSelectionClipboard(); On 2016/11/09 18:32:53, msw wrote: > Should this be a part of SelectAll? Don't think so. SelectAll is a part of the public Label API. We don't want programmatic changes to the text selection to update the selection clipboard. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:643: bool Label::GetAcceleratorForCommandId(int command_id, On 2016/11/09 18:32:52, msw wrote: > nit: Share a helper with textfield if feasible. Do you think it's still necessary now that we are handling just Copy/SelectAll? Because it would lead to more boilerplate without much sharing of logic. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:678: context_menu_runner_.reset(new MenuRunner(context_menu_contents_.get(), On 2016/11/09 18:32:52, msw wrote: > Would it suffice to initialize the runner once in/after > BuildContextMenuContents? Although the MenuRunner documentation does not state this, it seems that (at least on Linux) the MenuRunner needs to be initialized every time before it is used, even if the last exit was NORMAL_EXIT. Also, weirdly this is not the case on MacViews, where the same MenuRunner can be reused. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:682: ignore_result(context_menu_runner_->RunMenuAt( On 2016/11/09 18:32:53, msw wrote: > Perhaps we should check that the result isn't MENU_DELETED? But what to do if it is MENU_DELETED? https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:791: // This allows the BrowserView to pass the copy command from the Chrome menu On 2016/11/09 18:32:53, msw wrote: > Should we also add CTRL+A and Ctrl+Insert? It seems odd to only add one > accelerator, and just for the purpose of supporting some BrowserView behavior. See my comment above for "Not CTRL+A for select all?" https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:1020: context_menu_contents_.reset(new ui::SimpleMenuModel(this)); On 2016/11/09 18:32:52, msw wrote: > On 2016/11/09 10:20:00, karandeepb wrote: > > Do we want to show only Copy/SelectAll in the context menu for Labels or is > this > > ok? > > Yeah, I think we should only show copy and select all. Done. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:26: public ui::SimpleMenuModel::Delegate { On 2016/11/09 18:32:53, msw wrote: > It would be nice to use composition instead of inheritance, and have a shared > context menu model/controller class for Textfield and Label, but I don't know if > that's really feasible. It may be feasible, but since we are just implementing Copy/Select All, unlike the text selection case, there isn't much logic to be shared between the two classes. Also, again, the controller class would need to have a delegate interface to interact with, which may lead to unnecessary boilerplate. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:219: bool OnKeyPressed(const ui::KeyEvent& event) override; On 2016/11/09 18:32:53, msw wrote: > nit: match the order in view.h Done. Order for the existing functions wasn't correct, but I guess it's ok for the new functions. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:230: // ui::SimpleMenuModel::Delegate overrides: On 2016/11/09 18:32:53, msw wrote: > nit: try to keep the same order as the subclass list. Done. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:343: std::unique_ptr<ui::SimpleMenuModel> context_menu_contents_; On 2016/11/09 18:32:53, msw wrote: > nit: avoid unique_ptr, use plain member, if possible. > (but I guess this follows Textfield, so it's okay as-is too) Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:992: if(lines_.empty()) This is because we follow this codepath ClearRenderTextLines -> HasSelection-> GetRenderTextForSelectionController -> MaybeBuildRenderTextLines. Hence calling ClearRenderTextLines repeatedly could have lead to unnecessary computation. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:148: event_generator_ = Since we are now using EventGenerator anyway, if it's not too cumbersome, I'll try and use it for mouse events as well in a follow-up, as you had recommended earlier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:550: // The only accelerator a Label needs to handle is the Copy command from the On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:52, msw wrote: > > Not CTRL+A for select all? > > My understanding of accelerators is that they are to be used when a view wants > to perform some action on a key event even if it's not focused (hence generally > to be used for keyboard shortcuts). > > The only reason for handling Ctrl+C as an accelerator in the Label class is to > get Copy from the main Chrome menu (I think it has a specific name but not sure, > it's the one invoked from the button with three dots) working. See also the > Textfield class, where we add accelerators for Cut/Copy/Paste, again so that > these actions from the Chrome menu are handled. See > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi.... > for more details. Since SelectAll is not an action in that menu, it does not > need to be handled via an accelerator. Acknowledged, thanks for the explanation. 'Chrome menu' is what i say. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:565: bool Label::OnKeyPressed(const ui::KeyEvent& event) { On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:52, msw wrote: > > Should this use Textfield's GetCommandForKeyEvent and then just filter the > > supported commands? I worry about forked/duplicated logic drifting that should > > probably be consistent. > > Currently the TextEditCommand enum is only being used by TextInputClient and > subclasses. > If you think it's better, I can move Textfield's GetCommandForKeyEvent into a > separate file as a free function. Maybe a file called text_utils.h in ui/views. Hmm, this is okay for now; worth considering some cleanup later. (maybe Label and Textfield will share quite a bit more code someday) https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:575: if (control && !alt && HasSelection()) { On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:53, msw wrote: > > nit && !obscured()? here and below? > > But shouldn't we consume the event (by returning true) when the Label has an > active selection but has obscured text. Otherwise the event would propagate to > the parent view. > > FWIW, the textfield class returns false when the textfield does not have an > active selection or when it has obscured text, for Ctrl+C. Hence the event > propagates to the parent view for the active selection but obscured text case, > which is probably incorrect. Okay, that's fair, and the CopyToClipboard's early return works fine. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:590: UpdateSelectionClipboard(); On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:53, msw wrote: > > Should this be a part of SelectAll? > > Don't think so. SelectAll is a part of the public Label API. We don't want > programmatic changes to the text selection to update the selection clipboard. Acknowledged. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:643: bool Label::GetAcceleratorForCommandId(int command_id, On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:52, msw wrote: > > nit: Share a helper with textfield if feasible. > > Do you think it's still necessary now that we are handling just Copy/SelectAll? > Because it would lead to more boilerplate without much sharing of logic. Nope, the simpler inlined impl here makes sense for now. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:678: context_menu_runner_.reset(new MenuRunner(context_menu_contents_.get(), On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:52, msw wrote: > > Would it suffice to initialize the runner once in/after > > BuildContextMenuContents? > > Although the MenuRunner documentation does not state this, it seems that (at > least on Linux) the MenuRunner needs to be initialized every time before it is > used, even if the last exit was NORMAL_EXIT. Also, weirdly this is not the case > on MacViews, where the same MenuRunner can be reused. Acknowledged. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:682: ignore_result(context_menu_runner_->RunMenuAt( On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:53, msw wrote: > > Perhaps we should check that the result isn't MENU_DELETED? > > But what to do if it is MENU_DELETED? Right, it doesn't actually matter now, since the function doesn't use any instance members after this call. This is fine for now. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.cc:791: // This allows the BrowserView to pass the copy command from the Chrome menu On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:53, msw wrote: > > Should we also add CTRL+A and Ctrl+Insert? It seems odd to only add one > > accelerator, and just for the purpose of supporting some BrowserView behavior. > > See my comment above for "Not CTRL+A for select all?" Acknowledged. https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2422993002/diff/170001/ui/views/controls/labe... ui/views/controls/label.h:26: public ui::SimpleMenuModel::Delegate { On 2016/11/15 10:54:32, karandeepb wrote: > On 2016/11/09 18:32:53, msw wrote: > > It would be nice to use composition instead of inheritance, and have a shared > > context menu model/controller class for Textfield and Label, but I don't know > if > > that's really feasible. > > It may be feasible, but since we are just implementing Copy/Select All, unlike > the text selection case, there isn't much logic to be shared between the two > classes. Also, again, the controller class would need to have a delegate > interface to interact with, which may lead to unnecessary boilerplate. Agreed, it's fine as-is for now. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:76: // Invalidate |stored_selection_range_|. nit: this comment seems unnecessary (blank line above too) https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:77: stored_selection_range_ = gfx::Range::InvalidRange(); nit: maybe do this before ResetLayout? https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:992: if(lines_.empty()) On 2016/11/15 10:54:32, karandeepb wrote: > This is because we follow this codepath ClearRenderTextLines -> HasSelection-> > GetRenderTextForSelectionController -> MaybeBuildRenderTextLines. > > Hence calling ClearRenderTextLines repeatedly could have lead to unnecessary > computation. Add a quick explanatory comment; and run git cl format (space after if). https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.h:228: FRIEND_TEST_ALL_PREFIXES(LabelSelectionTest, ContextMenuContents); Can we avoid this and add any needed helpers on LabelSelectionTest, since that's already a friend class? https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:148: event_generator_ = On 2016/11/15 10:54:32, karandeepb wrote: > Since we are now using EventGenerator anyway, if it's not too cumbersome, I'll > try and use it for mouse events as well in a follow-up, as you had recommended > earlier. Acknowledged. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:194: }; nit: no trailing semi https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:918: EXPECT_STR_EQ(" mouse drag", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE)); Avoid testing the system clipboard in unit tests. We can only reliably test the contents of an actual clipboard in interactive_ui_tests (otherwise tests running in parallel may modify the clipboard during this test execution). You might be able to side-step this with something like TestClipboard... I'm not sure. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:978: EXPECT_EQ(initial_text, GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE)); You'll need to modify/disable this test for the same clipboard concerns above. I'd encourage you to (1) fix and re-enable these tests in place using something like TestClipboard, (2) move these tests to interactive_ui_tests and re-enable them, or (3) split out the clipboard-testing to separate fixtures and disable those for now, citing a bug; leave the non-clipboard stuff enabled. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1012: // For a selectable label with a selection, both COPY and SELECT_ALL should be nit: also check that some unhandled command (eg. paste) is still not enabled. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1018: // For an obscured selectable label, only SELECT_ALL should be enabled. I wonder if we should disable select-all (or even more? make the context menu not appear? disabling selectable?) for obscured labels. What's the point of selecting the obscured text if you can't do anything with it?
tapted@chromium.org changed reviewers: + tapted@chromium.org
I haven't looked over everything yet, but one majorish thing came up in bridged_content_view.mm https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1396: // Cut/Copy/Paste edit commands in the Chrome menu using accelerators. Hrm. There are bugs around the way a Cocoa WebContents validates cut/copy/paste menus items. E.g. http://crbug.com/457087 And View::CanHandleAccelerators() seems to happily `return true` in most cases when, e.g., if a button has focus, but cut/copy/paste should be disabled in the menuBar in that case. That's what currently happens in, e.g., the bookmark bubble. This looks like it would regress that case. Are there things other than views::Label that would need to do things here? It's a little gross, but I think the better option here is to check for views::Label::kViewClassName
just nits elsewhere. And a wild idea to make key event processing nicer https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:559: const bool shift = event.IsShiftDown(); This is probably orthogonal, but I feel like there should be a way to do something like int command = context_menu_contents_.CommandForAccelerator(ui::Accelerator(event)); if (!IsCommandIdEnabled(command)) return false; ExecuteCommand(command, event.flags()); return true; But I guess that would require [Simple]MenuModel to support multiple accelerators per command. Alternatively, just add an additional menu item with MenuModel::IsVisibleAt returning false and IsEnabledAt returning true (although that's a bit tricky -- a duplicate command works for MenuModel since it is index-based, but SimpleMenuModel is command_id-based) That code fragment above could even move into SimpleMenuModel so that this method becomes just return context_menu_contents_.HandleAccelerator(ui::Accelerator(event)); https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:593: // Chrome menu. In the comment, say why? I had the same immediate reaction - "why not select all". Maybe something like // Allow the "Copy" action from the Chrome menu to be invoked. E.g., if a user // selects a Label on a web modal dialog. "Select All" doesn't appear in the // Chrome menu so isn't handled here. [and is that example right? - is there another way it might be activated?] (but, also, with my suggestion above, this could just be return context_menu_contents_.HandleAccelerator(accelerator); and we wouldn't have to worry about clarifications :) https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:782: // CanProcessEventsWithinSubtree. nit: CanProcessEventsWithinSubtree(). https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:918: EXPECT_STR_EQ(" mouse drag", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE)); On 2016/11/15 20:06:45, msw wrote: > Avoid testing the system clipboard in unit tests. We can only reliably test the > contents of an actual clipboard in interactive_ui_tests (otherwise tests running > in parallel may modify the clipboard during this test execution). You might be > able to side-step this with something like TestClipboard... I'm not sure. Yah - there is http://crbug.com/623442 about this . There are current flakes that are able to reliably recover on retries, but adding more could trip the balance.
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 #8 (id:410001) has been deleted
Patchset #7 (id:390001) 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...
Description was changed from ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view can handle accelerators, in case no TextInputClient is active. This is necessary for Copy and SelectAll keyboard shortcuts and browser commands to work on MacViews. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label which can handle the given command. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ==========
PTAL Trent, msw@. Thanks. https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1396: // Cut/Copy/Paste edit commands in the Chrome menu using accelerators. On 2016/11/16 00:51:57, tapted wrote: > Hrm. There are bugs around the way a Cocoa WebContents validates cut/copy/paste > menus items. E.g. http://crbug.com/457087 > > And View::CanHandleAccelerators() seems to happily `return true` in most cases > when, e.g., if a button has focus, but cut/copy/paste should be disabled in the > menuBar in that case. That's what currently happens in, e.g., the bookmark > bubble. This looks like it would regress that case. > > Are there things other than views::Label that would need to do things here? > > It's a little gross, but I think the better option here is to check for > views::Label::kViewClassName Yeah I was aware of this, but thought it was ok to regress since we were still matching the WebContents behavior. Also, the Cut/Copy/Paste menu actions in the Views Chrome Browser Menu also have this behavior (they are enabled, when they shouldn't be. See BrowserView::CutCopyPaste). Have changed to use class name approach. Does this look ok? But this would break for Label subclasses. Ideally it seems that the View class API should provide this functionality somehow. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:76: // Invalidate |stored_selection_range_|. On 2016/11/15 20:06:45, msw wrote: > nit: this comment seems unnecessary (blank line above too) Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:77: stored_selection_range_ = gfx::Range::InvalidRange(); On 2016/11/15 20:06:45, msw wrote: > nit: maybe do this before ResetLayout? This needs to be done after ResetLayout. ResetLayout calls ClearRenderTextLines which is what persists the selection range. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:559: const bool shift = event.IsShiftDown(); On 2016/11/16 02:45:19, tapted wrote: > This is probably orthogonal, but I feel like there should be a way to do > something like > > int command = > context_menu_contents_.CommandForAccelerator(ui::Accelerator(event)); > if (!IsCommandIdEnabled(command)) > return false; > > ExecuteCommand(command, event.flags()); > return true; > > > But I guess that would require [Simple]MenuModel to support multiple > accelerators per command. Alternatively, just add an additional menu item with > MenuModel::IsVisibleAt returning false and IsEnabledAt returning true (although > that's a bit tricky -- a duplicate command works for MenuModel since it is > index-based, but SimpleMenuModel is command_id-based) > > That code fragment above could even move into SimpleMenuModel so that this > method becomes just > > return context_menu_contents_.HandleAccelerator(ui::Accelerator(event)); So if I am understanding you correctly, the SimpleMenuModel should maintain a mapping of Accelerators<->Menu commands. Also, as you said, multiple accelerators could map to the same command. While this will help to reduce duplication of logic and code to some extent, I am not too sure whether this is the right thing to do. It seems weird for SimpleMenuModel to assist in handling keyboard events. Also I think, the use case here is not generic enough to modify SimpleMenuModel. I think if any consolidation of logic needs to happen, (say sharing some code in OnKeyPressed, GetAcceleratorForCommandId and AcceleratorPressed), I think it should happen in the Label class itself. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:593: // Chrome menu. >In the comment, say why? I had the same immediate reaction - "why not select >all". Maybe something like > > // Allow the "Copy" action from the Chrome menu to be invoked. E.g., if a user > // selects a Label on a web modal dialog. "Select All" doesn't appear in the > // Chrome menu so isn't handled here. Done. >[and is that example right? - is there another way it might be activated?] So yeah it should be applicable for any persistent dialog. [Any dialog which stays visible on opening the chrome menu.] https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:782: // CanProcessEventsWithinSubtree. On 2016/11/16 02:45:19, tapted wrote: > nit: CanProcessEventsWithinSubtree(). Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:992: if(lines_.empty()) On 2016/11/15 20:06:45, msw wrote: > On 2016/11/15 10:54:32, karandeepb wrote: > > This is because we follow this codepath ClearRenderTextLines -> HasSelection-> > > GetRenderTextForSelectionController -> MaybeBuildRenderTextLines. > > > > Hence calling ClearRenderTextLines repeatedly could have lead to unnecessary > > computation. > > Add a quick explanatory comment; and run git cl format (space after if). Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.h:228: FRIEND_TEST_ALL_PREFIXES(LabelSelectionTest, ContextMenuContents); On 2016/11/15 20:06:45, msw wrote: > Can we avoid this and add any needed helpers on LabelSelectionTest, since that's > already a friend class? Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:194: }; On 2016/11/15 20:06:45, msw wrote: > nit: no trailing semi Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:918: EXPECT_STR_EQ(" mouse drag", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE)); On 2016/11/16 02:45:19, tapted wrote: > On 2016/11/15 20:06:45, msw wrote: > > Avoid testing the system clipboard in unit tests. We can only reliably test > the > > contents of an actual clipboard in interactive_ui_tests (otherwise tests > running > > in parallel may modify the clipboard during this test execution). You might be > > able to side-step this with something like TestClipboard... I'm not sure. > > Yah - there is http://crbug.com/623442 about this . There are current flakes > that are able to reliably recover on retries, but adding more could trip the > balance. Have created https://codereview.chromium.org/2499303003/ to fix the flaky clipboard behavior in views_unittests using TestClipboard and rebased this on top of it. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:978: EXPECT_EQ(initial_text, GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE)); On 2016/11/15 20:06:45, msw wrote: > You'll need to modify/disable this test for the same clipboard concerns above. > I'd encourage you to (1) fix and re-enable these tests in place using something > like TestClipboard, (2) move these tests to interactive_ui_tests and re-enable > them, or (3) split out the clipboard-testing to separate fixtures and disable > those for now, citing a bug; leave the non-clipboard stuff enabled. Have created https://codereview.chromium.org/2499303003/ to fix the flaky clipboard behavior in views_unittests using TestClipboard and rebased this on top of it. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1012: // For a selectable label with a selection, both COPY and SELECT_ALL should be On 2016/11/15 20:06:45, msw wrote: > nit: also check that some unhandled command (eg. paste) is still not enabled. Done. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1018: // For an obscured selectable label, only SELECT_ALL should be enabled. On 2016/11/15 20:06:45, msw wrote: > I wonder if we should disable select-all (or even more? make the context menu > not appear? disabling selectable?) for obscured labels. What's the point of > selecting the obscured text if you can't do anything with it? Yeah I agree making obscured labels selectable does not make much sense. Have disabled text selection for obscured labels.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1396: // Cut/Copy/Paste edit commands in the Chrome menu using accelerators. On 2016/11/16 07:48:28, karandeepb wrote: > On 2016/11/16 00:51:57, tapted wrote: > > Hrm. There are bugs around the way a Cocoa WebContents validates > cut/copy/paste > > menus items. E.g. http://crbug.com/457087 > > > > And View::CanHandleAccelerators() seems to happily `return true` in most cases > > when, e.g., if a button has focus, but cut/copy/paste should be disabled in > the > > menuBar in that case. That's what currently happens in, e.g., the bookmark > > bubble. This looks like it would regress that case. > > > > Are there things other than views::Label that would need to do things here? > > > > It's a little gross, but I think the better option here is to check for > > views::Label::kViewClassName > > Yeah I was aware of this, but thought it was ok to regress since we were still > matching the WebContents behavior. Also, the Cut/Copy/Paste menu actions in the > Views Chrome Browser Menu also have this behavior (they are enabled, when they > shouldn't be. See BrowserView::CutCopyPaste). > > Have changed to use class name approach. Does this look ok? But this would break > for Label subclasses. Well it would break for Label subclasses that override GetClassName :) - it looks like there are two of those: VerticalCandidateLabel which is ChromeOS only, and views::Link which has its own mouse handling that's going to confound selection anyway. > > Ideally it seems that the View class API should provide this functionality > somehow. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:559: const bool shift = event.IsShiftDown(); On 2016/11/16 07:48:28, karandeepb wrote: > On 2016/11/16 02:45:19, tapted wrote: > > This is probably orthogonal, but I feel like there should be a way to do > > something like > > > > int command = > > context_menu_contents_.CommandForAccelerator(ui::Accelerator(event)); > > if (!IsCommandIdEnabled(command)) > > return false; > > > > ExecuteCommand(command, event.flags()); > > return true; > > > > > > But I guess that would require [Simple]MenuModel to support multiple > > accelerators per command. Alternatively, just add an additional menu item with > > MenuModel::IsVisibleAt returning false and IsEnabledAt returning true > (although > > that's a bit tricky -- a duplicate command works for MenuModel since it is > > index-based, but SimpleMenuModel is command_id-based) > > > > That code fragment above could even move into SimpleMenuModel so that this > > method becomes just > > > > return context_menu_contents_.HandleAccelerator(ui::Accelerator(event)); > > So if I am understanding you correctly, the SimpleMenuModel should maintain a > mapping of Accelerators<->Menu commands. Also, as you said, multiple > accelerators could map to the same command. > > While this will help to reduce duplication of logic and code to some extent, I > am not too sure whether this is the right thing to do. It seems weird for > SimpleMenuModel to assist in handling keyboard events. Also I think, the use > case here is not generic enough to modify SimpleMenuModel. It's pretty much exactly how Cocoa makes you do it (so, yes, definitely weird :p). i.e. Most accelerator handling in Cocoa UI is done by doing [[NSApp mainMenu] performKeyEquivalent:SomeKeyEvent()]. Custom accelerator remapping is all done by searching through command strings in the menuBar. The reason why a user-remapped "Copy" command works on MacViews is that the menuBar receives the event and sends it to [BridgedContentView copy:] then that maps it back to VKEY_C > > I think if any consolidation of logic needs to happen, (say sharing some code in > OnKeyPressed, GetAcceleratorForCommandId and AcceleratorPressed), I think it > should happen in the Label class itself. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:46: const int kInvalidTextMenuCommand = 0; 0 is valid according to SimpleMenuModel::ValidateItem (and -1 is separator). Rather than something arbitrary, I think we can change the signature of GetContextMenuCommandForMenuAction to return `bool`, or switch on ui::TextEditCommand after checking that GetTextEditCommandForMenuAction didn't return INVALID_COMMAND, or just remove GetContextMenuCommandForMenuAction completely (see later comments) https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:272: return IDS_APP_UNDO; I don't really like seeing IDS_ translated string ids here instead of ui::TextEditCommand -- it's too much coupling with things views::Label is responsible for. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1417: return false; nit: false -> NO https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1419: views::Label* label = I think this could safely just `return YES`. Right-clicking text on Mac is meant to highlight the current word (we don't currently do that, but in theory there should always be a selection when the menu is shown). So there should always be something to copy. And I guess the static casts scare me more than an enabled but non-functional menu item for obscured labels. Also we could get rid of GetContextMenuCommandForMenuAction() I think it's gross enough just checking views::Label::kViewClassName, but this goes a big step beyond that. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1421: return static_cast<ui::SimpleMenuModel::Delegate*>(label)->IsCommandIdEnabled( you could get rid of this static_cast by assigning to a ui::SimpleMenuModel::Delegate* on the line above (still static_cast to Label* though). But I think we should remove this, unless there's something more compelling that I've missed. (there was also base::implicit_cast<>, but it was taken away. I also don't believe in making overrides of public methods private, but others do :) ).
lgtm with a nit, and modulo Trent's feedback. https://codereview.chromium.org/2422993002/diff/430001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/430001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1028: // An obscured label would not show a context menu and both COPY only nit: s/only/and/?
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...
PTAL Trent. https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2422993002/diff/330001/ui/views/controls/labe... ui/views/controls/label.cc:559: const bool shift = event.IsShiftDown(); On 2016/11/16 08:39:07, tapted wrote: > On 2016/11/16 07:48:28, karandeepb wrote: > > On 2016/11/16 02:45:19, tapted wrote: > > > This is probably orthogonal, but I feel like there should be a way to do > > > something like > > > > > > int command = > > > context_menu_contents_.CommandForAccelerator(ui::Accelerator(event)); > > > if (!IsCommandIdEnabled(command)) > > > return false; > > > > > > ExecuteCommand(command, event.flags()); > > > return true; > > > > > > > > > But I guess that would require [Simple]MenuModel to support multiple > > > accelerators per command. Alternatively, just add an additional menu item > with > > > MenuModel::IsVisibleAt returning false and IsEnabledAt returning true > > (although > > > that's a bit tricky -- a duplicate command works for MenuModel since it is > > > index-based, but SimpleMenuModel is command_id-based) > > > > > > That code fragment above could even move into SimpleMenuModel so that this > > > method becomes just > > > > > > return context_menu_contents_.HandleAccelerator(ui::Accelerator(event)); > > > > So if I am understanding you correctly, the SimpleMenuModel should maintain a > > mapping of Accelerators<->Menu commands. Also, as you said, multiple > > accelerators could map to the same command. > > > > While this will help to reduce duplication of logic and code to some extent, I > > am not too sure whether this is the right thing to do. It seems weird for > > SimpleMenuModel to assist in handling keyboard events. Also I think, the use > > case here is not generic enough to modify SimpleMenuModel. > > It's pretty much exactly how Cocoa makes you do it (so, yes, definitely weird > :p). i.e. Most accelerator handling in Cocoa UI is done by doing [[NSApp > mainMenu] performKeyEquivalent:SomeKeyEvent()]. > > Custom accelerator remapping is all done by searching through command strings in > the menuBar. The reason why a user-remapped "Copy" command works on MacViews is > that the menuBar receives the event and sends it to [BridgedContentView copy:] > then that maps it back to VKEY_C > > > > > I think if any consolidation of logic needs to happen, (say sharing some code > in > > OnKeyPressed, GetAcceleratorForCommandId and AcceleratorPressed), I think it > > should happen in the Label class itself. > Oh ok. Din't know this. As talked offline, this can be handled subsequently if a similar recurring pattern emerges elsewhere as well. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:46: const int kInvalidTextMenuCommand = 0; On 2016/11/16 08:39:07, tapted wrote: > 0 is valid according to SimpleMenuModel::ValidateItem (and -1 is separator). > Rather than something arbitrary, I think we can change the signature of > GetContextMenuCommandForMenuAction to return `bool`, or switch on > ui::TextEditCommand after checking that GetTextEditCommandForMenuAction didn't > return INVALID_COMMAND, or just remove GetContextMenuCommandForMenuAction > completely (see later comments) Removed GetContextMenuCommandForMenuAction. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:272: return IDS_APP_UNDO; On 2016/11/16 08:39:07, tapted wrote: > I don't really like seeing IDS_ translated string ids here instead of > ui::TextEditCommand -- it's too much coupling with things views::Label is > responsible for. Removed this. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1417: return false; On 2016/11/16 08:39:07, tapted wrote: > nit: false -> NO Done. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1419: views::Label* label = >>I think this could safely just `return YES`. Right-clicking text on Mac is meant >>to highlight the current word (we don't currently do that, but in theory there >>should always be a selection when the menu is shown). So there should always be >>something to copy. Not really, left clicking a label will give focus to it. Hence on showing the menu, there's nothing to copy. >>And I guess the static casts scare me more than an enabled >>but non-functional menu item for obscured labels. Also we could get rid of >>GetContextMenuCommandForMenuAction() >> >>I think it's gross enough just checking views::Label::kViewClassName, but this >>goes a big step beyond that. Yeah I wasn't also too happy with this. Removed. https://codereview.chromium.org/2422993002/diff/430001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1421: return static_cast<ui::SimpleMenuModel::Delegate*>(label)->IsCommandIdEnabled( On 2016/11/16 08:39:07, tapted wrote: > (there was also base::implicit_cast<>, but it was taken away. I also don't > believe in making overrides of public methods private, but others do :) ). Yeah I remember seeing a thread about this earlier. Maybe it's just an artifact of not allowing private inheritance. https://codereview.chromium.org/2422993002/diff/430001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2422993002/diff/430001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:1028: // An obscured label would not show a context menu and both COPY only On 2016/11/16 18:55:16, msw wrote: > nit: s/only/and/? Done.
lgtm thanks!
Description was changed from ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label which can handle the given command. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ==========
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@.
msw already reviewed this, so rubber stamp LGTM from me
Description was changed from ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. The stored selection range is now also correctly invalidated in Label::SetText and Label::SetSelectable. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. Some other changes- - The stored selection range is now correctly invalidated in Label::SetText and Label::SetSelectable. - Text selection is disabled for obscured labels. BUG=630365, 437993 ==========
The CQ bit was unchecked by karandeepb@chromium.org
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2422993002/#ps450001 (title: "Address comments.")
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 ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. Some other changes- - The stored selection range is now correctly invalidated in Label::SetText and Label::SetSelectable. - Text selection is disabled for obscured labels. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. Some other changes- - The stored selection range is now correctly invalidated in Label::SetText and Label::SetSelectable. - Text selection is disabled for obscured labels. BUG=630365, 437993 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. Some other changes- - The stored selection range is now correctly invalidated in Label::SetText and Label::SetSelectable. - Text selection is disabled for obscured labels. BUG=630365, 437993 ========== to ========== views::Label: Implement context menu, keyboard shortcuts for copy/select all. This CL is a follow-up to r430461 which implemented text selection for Labels. This CL implements the following for Labels- - Copy selected text on [Ctrl+C] or [Ctrl+Insert]. - Select all text on [Ctrl+A]. - Context Menu with the options Copy and Select All. - Copy selected text using the Browser menu. BridgedContentView's validateUserInterfaceItem method is also modified. Currently it returns YES only when there is an active TextInputClient which can perform the given command. But since views::Label does not implement the TextInputClient interface, the method is modified to return YES in case the focused view is a Label for the Copy and Select All commands. This is necessary for Copy and Select All keyboard shortcuts and browser menu commands to work on MacViews for labels. Some other changes- - The stored selection range is now correctly invalidated in Label::SetText and Label::SetSelectable. - Text selection is disabled for obscured labels. BUG=630365, 437993 Committed: https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a Cr-Commit-Position: refs/heads/master@{#432720} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a Cr-Commit-Position: refs/heads/master@{#432720} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
