Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627013002:
MacViews: Fix combobox keyboard shortcuts. (Closed)
|
Created:
3 years, 11 months ago by karandeepb Modified:
3 years, 11 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox opens the dropdown menu even for STYLE_ACTION.
- Home, End, Command+Up/Down open the dropdown menu.
- F4, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
Review-Url: https://codereview.chromium.org/2627013002
Cr-Commit-Position: refs/heads/master@{#444269}
Committed: https://chromium.googlesource.com/chromium/src/+/95da469f19599a040d9dd93fe443e89345a65509
Patch Set 1 #Patch Set 2 : Rebase. Add test. #Patch Set 3 : Nits. #
Total comments: 20
Patch Set 4 : Address review #
Total comments: 2
Patch Set 5 : Add a comment. #
Messages
Total messages: 26 (19 generated)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 #3 (id:40001) has been deleted
Description was changed from
==========
MacViews: Fix combobox keyboard shortcuts.
==========
to
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing.
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox with STYLE_ACTION, notifies click on key press
instead of key release.
- F4, Home, End, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
Description was changed from
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing.
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox with STYLE_ACTION, notifies click on key press
instead of key release.
- F4, Home, End, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
to
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox with STYLE_ACTION, notifies click on key press
instead of key release.
- F4, Home, End, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Can you confirm the implemented behavior is what we want. I just checked the behavior using a Cocoa Combobox control. #ifdefs do introduce some ugliness. Let me know if you can think of any better way. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: if (style_ == STYLE_ACTION) Does STYLE_ACTION have something analogous on Cocoa? Calling OnPerformAction on SPACE seemed reasonable.
yeah the #ifdefs aren't great. an alternative could be PlatformStyle::kNavigateComboboxWithMenu, but actually using that might be a bit clumsy. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: if (style_ == STYLE_ACTION) On 2017/01/13 08:28:57, karandeepb wrote: > Does STYLE_ACTION have something analogous on Cocoa? Calling OnPerformAction on > SPACE seemed reasonable. STYLE_ACTION on mac is effectively NSPopupButton. But there's no Mac equivalent for a control where one side "clicks" and the other side shows a menu (you always need to click twice: once on the button and once on the menu) -- and clicking anywhere on an NSPopupButton shows the menu. So I think SPACE should behave the same regardless of |style_|. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:652: case ui::VKEY_END: I think HOME/END should be mapped. Remember we pick "meaningful" keycodes for these so Cmd+Up is Ctrl+VKEY_HOME (whereas the Home key defaults to scrollToBeginningOfDocument: which we ignore). Then.. should we check for Control? I'm inclined not to bother - it's nicer to do "something" IMO that have it seem broken. PageUp/PageDown seem to work when the menu is showing, but not to show it so PRIOR/NEXT can be skipped. So I guess we have #if defined(OS_MACOSX) // Mac Comboboxes and PopupButtons (similar to STYLE_ACTION) show the menu // before processing any navigation commands. case UP case DOWN case SPACE case HOME case END show_menu = true; break; #endif wdyt? https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:706: text_button_->state() == Button::STATE_PRESSED) (does showing the menu instead manage to swallow the release? - we might not need this) https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:280: TEST_F(ComboboxTest, KeyTest) { nit: comment before describing test https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:280: TEST_F(ComboboxTest, KeyTest) { perhaps call this KeyTestMac? Since it's quite different -- flakiness dashboards and things should keep them separate. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:314: #else nit: // !OS_MACOSX https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:315: TEST_F(ComboboxTest, KeyTest) { nit: comment before https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:360: let's move TEST_F(ComboboxTest, KeyTest) in here too since that comment applies to that as well
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
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox with STYLE_ACTION, notifies click on key press
instead of key release.
- F4, Home, End, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
to
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox opens the dropdown menu even for STYLE_ACTION.
- Home, End, Command+Up/Down open the dropdown menu.
- F4, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
PTAL Trent. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: if (style_ == STYLE_ACTION) On 2017/01/13 19:06:39, tapted wrote: > On 2017/01/13 08:28:57, karandeepb wrote: > > Does STYLE_ACTION have something analogous on Cocoa? Calling OnPerformAction > on > > SPACE seemed reasonable. > > STYLE_ACTION on mac is effectively NSPopupButton. But there's no Mac equivalent > for a control where one side "clicks" and the other side shows a menu (you > always need to click twice: once on the button and once on the menu) -- and > clicking anywhere on an NSPopupButton shows the menu. So I think SPACE should > behave the same regardless of |style_|. Done. Although, it seems to me that the intention of STYLE_ACTION was to be a button with a rarely used dropdown. Though I think STYLE_ACTION is pretty weird. Also, it is only used once in the whole codebase. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:652: case ui::VKEY_END: On 2017/01/13 19:06:39, tapted wrote: > I think HOME/END should be mapped. Remember we pick "meaningful" keycodes for > these so Cmd+Up is Ctrl+VKEY_HOME (whereas the Home key defaults to > scrollToBeginningOfDocument: which we ignore). > > Then.. should we check for Control? I'm inclined not to bother - it's nicer to > do "something" IMO that have it seem broken. PageUp/PageDown seem to work when > the menu is showing, but not to show it so PRIOR/NEXT can be skipped. So I guess > we have > > #if defined(OS_MACOSX) > // Mac Comboboxes and PopupButtons (similar to STYLE_ACTION) show the menu > // before processing any navigation commands. > case UP > case DOWN > case SPACE > case HOME > case END > show_menu = true; > break; > #endif > > wdyt? Ohk I din't know that Cmd+Up/Down could also be used to show the menu. Done. Although with this, simple HOME/END would also show the menu, which'll be different from Cocoa. On another note, it doesn't seem right to me to map keycodes on Mac. I would have preferred the views operating on actual keycodes instead (w/o the mapping at the BCV layer). Although that may be an involved change at this stage. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:706: text_button_->state() == Button::STATE_PRESSED) On 2017/01/13 19:06:39, tapted wrote: > (does showing the menu instead manage to swallow the release? - we might not > need this) So the key release is swallowed when the menu is shown on pressing Space. However on pressing Space with an active [NS]menu, we do get a key release event here. This happens through BCV::keyUp. I would have expected the NSMenu to consume both the key press and key up in this case. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:280: TEST_F(ComboboxTest, KeyTest) { On 2017/01/13 19:06:39, tapted wrote: > perhaps call this KeyTestMac? Since it's quite different -- flakiness dashboards > and things should keep them separate. Done. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:280: TEST_F(ComboboxTest, KeyTest) { On 2017/01/13 19:06:40, tapted wrote: > nit: comment before describing test Done. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:314: #else On 2017/01/13 19:06:39, tapted wrote: > nit: // !OS_MACOSX Don't think it's needed now. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:315: TEST_F(ComboboxTest, KeyTest) { On 2017/01/13 19:06:39, tapted wrote: > nit: comment before Done. https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:360: On 2017/01/13 19:06:39, tapted wrote: > let's move TEST_F(ComboboxTest, KeyTest) in here too since that comment applies > to that as well Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a comment (see below) https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: if (style_ == STYLE_ACTION) On 2017/01/16 04:26:15, karandeepb wrote: > On 2017/01/13 19:06:39, tapted wrote: > > On 2017/01/13 08:28:57, karandeepb wrote: > > > Does STYLE_ACTION have something analogous on Cocoa? Calling OnPerformAction > > on > > > SPACE seemed reasonable. > > > > STYLE_ACTION on mac is effectively NSPopupButton. But there's no Mac > equivalent > > for a control where one side "clicks" and the other side shows a menu (you > > always need to click twice: once on the button and once on the menu) -- and > > clicking anywhere on an NSPopupButton shows the menu. So I think SPACE should > > behave the same regardless of |style_|. > > Done. Although, it seems to me that the intention of STYLE_ACTION was to be a > button with a rarely used dropdown. Though I think STYLE_ACTION is pretty weird. > Also, it is only used once in the whole codebase. yup - and even that is likely going away -- https://go/uvfmt https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:652: case ui::VKEY_END: On 2017/01/16 04:26:15, karandeepb wrote: > On 2017/01/13 19:06:39, tapted wrote: > > I think HOME/END should be mapped. Remember we pick "meaningful" keycodes for > > these so Cmd+Up is Ctrl+VKEY_HOME (whereas the Home key defaults to > > scrollToBeginningOfDocument: which we ignore). > > > > Then.. should we check for Control? I'm inclined not to bother - it's nicer to > > do "something" IMO that have it seem broken. PageUp/PageDown seem to work when > > the menu is showing, but not to show it so PRIOR/NEXT can be skipped. So I > guess > > we have > > > > #if defined(OS_MACOSX) > > // Mac Comboboxes and PopupButtons (similar to STYLE_ACTION) show the menu > > // before processing any navigation commands. > > case UP > > case DOWN > > case SPACE > > case HOME > > case END > > show_menu = true; > > break; > > #endif > > > > wdyt? > > Ohk I din't know that Cmd+Up/Down could also be used to show the menu. Done. > Although with this, simple HOME/END would also show the menu, which'll be > different from Cocoa. > On another note, it doesn't seem right to me to map keycodes on Mac. I would > have preferred the views operating on actual keycodes instead (w/o the mapping > at the BCV layer). Although that may be an involved change at this stage. yeah the mapping is to give things overriding OnKeyEvent something more cross-platfrom. Adding an edit command field to the key event (ie replace SetEditCommandForNextKeyEvent) might be nicer https://codereview.chromium.org/2627013002/diff/80001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/80001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: show_menu = true; nit: comment like "On Mac, navigation keys should always just show the menu first."
https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/60001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: if (style_ == STYLE_ACTION) On 2017/01/17 21:48:32, tapted wrote: > On 2017/01/16 04:26:15, karandeepb wrote: > > On 2017/01/13 19:06:39, tapted wrote: > > > On 2017/01/13 08:28:57, karandeepb wrote: > > > > Does STYLE_ACTION have something analogous on Cocoa? Calling > OnPerformAction > > > on > > > > SPACE seemed reasonable. > > > > > > STYLE_ACTION on mac is effectively NSPopupButton. But there's no Mac > > equivalent > > > for a control where one side "clicks" and the other side shows a menu (you > > > always need to click twice: once on the button and once on the menu) -- and > > > clicking anywhere on an NSPopupButton shows the menu. So I think SPACE > should > > > behave the same regardless of |style_|. > > > > Done. Although, it seems to me that the intention of STYLE_ACTION was to be a > > button with a rarely used dropdown. Though I think STYLE_ACTION is pretty > weird. > > Also, it is only used once in the whole codebase. > > yup - and even that is likely going away -- https://go/uvfmt Oh, that's good, should result in some code cleanup here. https://codereview.chromium.org/2627013002/diff/80001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2627013002/diff/80001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:630: show_menu = true; On 2017/01/17 21:48:32, tapted wrote: > nit: comment like "On Mac, navigation keys should always just show the menu > first." Done.
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/2627013002/#ps100001 (title: "Add a comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484709003389450,
"parent_rev": "9057f4585060c84304068226288a9aa8e5ab0e71", "commit_rev":
"95da469f19599a040d9dd93fe443e89345a65509"}
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox opens the dropdown menu even for STYLE_ACTION.
- Home, End, Command+Up/Down open the dropdown menu.
- F4, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
==========
to
==========
MacViews: Fix combobox keyboard shortcuts.
This CL changes the following behavior on MacViews:
- Return key on a focused combobox does nothing (It should perform the default
action for the dialog).
- Up/Down arrow keys open the combobox dropdown instead of changing the
selected index directly.
- Space key press on a combobox opens the dropdown menu even for STYLE_ACTION.
- Home, End, Command+Up/Down open the dropdown menu.
- F4, Page Down, Page Up on a focused combobox do nothing.
BUG=619545, 607430
Review-Url: https://codereview.chromium.org/2627013002
Cr-Commit-Position: refs/heads/master@{#444269}
Committed:
https://chromium.googlesource.com/chromium/src/+/95da469f19599a040d9dd93fe443...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/95da469f19599a040d9dd93fe443... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
