Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10966027:
Basic keyboard access for recently_closed menu on NTP. Allows using up and down arrows to navigate,… (Closed)
|
Created:
8 years, 3 months ago by aboxhall Modified:
8 years, 2 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionBasic keyboard access for recently_closed menu on NTP. Allows using up and down arrows to navigate, enter/space to activate.
Since this uses focus() to allow selecting menu items, this puts a focus ring around the selected menu item. I assume this won't be the desired behaviour, but I thought I'd find out what we would prefer before I started messing with the CSS etc.
BUG=151462
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158550
Patch Set 1 #Patch Set 2 : Get rid of pointless experimental code" #
Total comments: 18
Patch Set 3 : Pull chrome.send params out into a variable #Patch Set 4 : Add menu/menuitem role, comment for Tab character, and keep track of last selected item. #
Total comments: 6
Patch Set 5 : Don't steal keyboard events that the menu doesn't handle, and detect focusin on unrelated elements … #
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/recently_closed.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:99: var orig = e.originalEvent; nit: IMO var params = [data.sessionId, index, orig.type == 'click' ? orig.button : 0, // Explains why. orig.altKey, orig.ctrlKey, orig.metaKey, orig.shiftKey]; chrome.send('reopenTab', params); http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:112: a.addEventListener('activate', onActivated); I really wish this was [cr:ui:]menu:activate or something instead so people know on first glance it's synthetic. I thought there was actually some weird 'activate' event natively (in views, the windows native UI toolkit for chrome, focus is called activation so it didn't seem that far fetched). http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:122: this.menu.selectedIndex = 0; should the first item always be selected when shown? http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:149: this.focus(); I'm confused by this ^ http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': case 'U+0009': // Whatever the heck this key is.
http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/recently_closed.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:99: var orig = e.originalEvent; On 2012/09/21 03:44:50, Dan Beam wrote: > nit: IMO > > var params = [data.sessionId, > index, > orig.type == 'click' ? orig.button : 0, // Explains why. > orig.altKey, > orig.ctrlKey, > orig.metaKey, > orig.shiftKey]; > chrome.send('reopenTab', params); Done. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:99: var orig = e.originalEvent; On 2012/09/21 03:44:50, Dan Beam wrote: > nit: IMO > > var params = [data.sessionId, > index, > orig.type == 'click' ? orig.button : 0, // Explains why. > orig.altKey, > orig.ctrlKey, > orig.metaKey, > orig.shiftKey]; > chrome.send('reopenTab', params); Done. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:112: a.addEventListener('activate', onActivated); On 2012/09/21 03:44:50, Dan Beam wrote: > I really wish this was [cr:ui:]menu:activate or something instead so people know > on first glance it's synthetic. I thought there was actually some weird > 'activate' event natively (in views, the windows native UI toolkit for chrome, > focus is called activation so it didn't seem that far fetched). Should I try to fix that in this CL? http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:122: this.menu.selectedIndex = 0; On 2012/09/21 03:44:50, Dan Beam wrote: > should the first item always be selected when shown? The selectedIndex is set back to -1 below, so it seems reasonable to just go to the first item each time (unless we want the menu to also keep track of the previously selected item when it's closed?) http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:149: this.focus(); On 2012/09/21 03:44:50, Dan Beam wrote: > I'm confused by this ^ It puts the focus back on the menu button when the menu closes, so you can keep tabbing through the page as before.
http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/recently_closed.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:112: a.addEventListener('activate', onActivated); On 2012/09/21 06:46:00, aboxhall wrote: > On 2012/09/21 03:44:50, Dan Beam wrote: > > I really wish this was [cr:ui:]menu:activate or something instead so people > know > > on first glance it's synthetic. I thought there was actually some weird > > 'activate' event natively (in views, the windows native UI toolkit for chrome, > > focus is called activation so it didn't seem that far fetched). > > Should I try to fix that in this CL? Any CL ever would surpass the current plans :P http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:122: this.menu.selectedIndex = 0; On 2012/09/21 06:46:00, aboxhall wrote: > On 2012/09/21 03:44:50, Dan Beam wrote: > > should the first item always be selected when shown? > > The selectedIndex is set back to -1 below, so it seems reasonable to just go to > the first item each time (unless we want the menu to also keep track of the > previously selected item when it's closed?) Where is it set to -1? Either way I guess this method isn't actually toggling the visibility of the list or anything (like "showMenu" would you'd think), so I guess this is fine. You'd think this would start unselected, when their container was focused the first item (I guess) would be selected, and until something changed the selected or multiselected + lead would be the same, yes. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:149: this.focus(); On 2012/09/21 06:46:00, aboxhall wrote: > On 2012/09/21 03:44:50, Dan Beam wrote: > > I'm confused by this ^ > > It puts the focus back on the menu button when the menu closes, so you can keep > tabbing through the page as before. How does focus()ing a hidden element put the focus back where it was before? I thought focus()ing a hidden element did nothing in WebKit. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': On 2012/09/21 03:44:50, Dan Beam wrote: > case 'U+0009': // Whatever the heck this key is. Ping
As well as keeping track of the last selected item and adding the comment for the Tab key identifier, I've also added ARIA roles to the menu and menu item elements, as neither VoiceOver nor ChromeVox seemed to pick up the HTML menu element as a menu. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/recently_closed.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/recently_closed.js:112: a.addEventListener('activate', onActivated); On 2012/09/21 10:23:48, Dan Beam wrote: > On 2012/09/21 06:46:00, aboxhall wrote: > > On 2012/09/21 03:44:50, Dan Beam wrote: > > > I really wish this was [cr:ui:]menu:activate or something instead so people > > know > > > on first glance it's synthetic. I thought there was actually some weird > > > 'activate' event natively (in views, the windows native UI toolkit for > chrome, > > > focus is called activation so it didn't seem that far fetched). > > > > Should I try to fix that in this CL? > > Any CL ever would surpass the current plans :P I found 45 occurrences of 'activate' under chrome/browser/resources. I think that fixing this is beyond the scope of this change, but if you like I could have a go later on. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:122: this.menu.selectedIndex = 0; On 2012/09/21 10:23:48, Dan Beam wrote: > On 2012/09/21 06:46:00, aboxhall wrote: > > On 2012/09/21 03:44:50, Dan Beam wrote: > > > should the first item always be selected when shown? > > > > The selectedIndex is set back to -1 below, so it seems reasonable to just go > to > > the first item each time (unless we want the menu to also keep track of the > > previously selected item when it's closed?) > > Where is it set to -1? Either way I guess this method isn't actually toggling > the visibility of the list or anything (like "showMenu" would you'd think), so I > guess this is fine. You'd think this would start unselected, when their > container was focused the first item (I guess) would be selected, and until > something changed the selected or multiselected + lead would be the same, yes. It's set to -1 in the hideMenu() method below. I agree it would be better to go back to the last selected item; I'll do that. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:149: this.focus(); On 2012/09/21 10:23:48, Dan Beam wrote: > On 2012/09/21 06:46:00, aboxhall wrote: > > On 2012/09/21 03:44:50, Dan Beam wrote: > > > I'm confused by this ^ > > > > It puts the focus back on the menu button when the menu closes, so you can > keep > > tabbing through the page as before. > > How does focus()ing a hidden element put the focus back where it was before? I > thought focus()ing a hidden element did nothing in WebKit. It's not focusing a hidden element, it's focusing the menu button. |this.menu| is hidden at this point, but not |this|. http://codereview.chromium.org/10966027/diff/3001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': On 2012/09/21 10:23:48, Dan Beam wrote: > On 2012/09/21 03:44:50, Dan Beam wrote: > > case 'U+0009': // Whatever the heck this key is. > > Ping Oopsies. It's 'Tab'. Fixed.
lgtm but you should wait for someone with more a11y specific skills and understands the functional details of this change
(that would be Dominic, not me)
ARIA roles and general focus management looks great, just one thought. http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (left): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:103: case 'blur': Just checking, do we want to handle 'blur' still but maybe handle it more intelligently? For example, we shouldn't hide the menu if someone moves focus from the menu button to one of the items. But if someone moves focus to another control on the page, should we hide the menu then? After some delay? http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': // Tab Specifically trapping the tab key worries me a bit. I wonder if a slightly smarter 'blur' handler would be the least brittle solution? If the user focuses something on the page outside of the menu button or menu items, we should close the menu. But just pressing Tab might not catch everything, and it's a pretty low-level thing to check for. Things to test: * Clicking on another element on the page, or on the page background, to blur the menu * Jumping to another focusable element with a screen reader.
http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (left): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:103: case 'blur': On 2012/09/24 17:49:06, Dominic Mazzoni wrote: > Just checking, do we want to handle 'blur' still but maybe handle it more > intelligently? > > For example, we shouldn't hide the menu if someone moves focus from the menu > button to one of the items. But if someone moves focus to another control on the > page, should we hide the menu then? After some delay? This makes sense, but as I describe below, it actually already works. http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': // Tab On 2012/09/24 17:49:06, Dominic Mazzoni wrote: > Specifically trapping the tab key worries me a bit. > > I wonder if a slightly smarter 'blur' handler would be the least brittle > solution? If the user focuses something on the page outside of the menu button > or menu items, we should close the menu. But just pressing Tab might not catch > everything, and it's a pretty low-level thing to check for. > > Things to test: > * Clicking on another element on the page, or on the page background, to blur > the menu > * Jumping to another focusable element with a screen reader. So, I did have the same thought while I was working on this, and tried to implement this by checking the focused item during the blur event, but it wasn't yet set correctly at that time (it was telling me the focused element was document.body). However, when I tested it out as you suggest above (at least, with the mouse; I didn't actually check with the screen reader as I just used the 'esc' menu close shortcut), it did in fact work. I will double check when I'm in the office today, but I'm pretty sure it already behaves sensibly as you describe. To be honest, I'm not sure _how_ it works, but I could try and figure it out if you like. The Tab key is necessary here as the menu traps all keyboard events, so you can't tab out of the menu by using the Tab key unless we look for it here.
http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': // Tab On 2012/09/24 22:39:38, aboxhall wrote: > On 2012/09/24 17:49:06, Dominic Mazzoni wrote: > > Specifically trapping the tab key worries me a bit. > > > > I wonder if a slightly smarter 'blur' handler would be the least brittle > > solution? If the user focuses something on the page outside of the menu button > > or menu items, we should close the menu. But just pressing Tab might not catch > > everything, and it's a pretty low-level thing to check for. > > > > Things to test: > > * Clicking on another element on the page, or on the page background, to blur > > the menu > > * Jumping to another focusable element with a screen reader. > > So, I did have the same thought while I was working on this, and tried to > implement this by checking the focused item during the blur event, but it wasn't > yet set correctly at that time (it was telling me the focused element was > document.body). OK. If it isn't needed, that's fine. I'm still curious as to why it works - there must be code somewhere that's triggering it, can you set a breakpoint in hideMenu and figure out where it's called? > However, when I tested it out as you suggest above (at least, with the mouse; I > didn't actually check with the screen reader as I just used the 'esc' menu close > shortcut), it did in fact work. I will double check when I'm in the office > today, but I'm pretty sure it already behaves sensibly as you describe. To be > honest, I'm not sure _how_ it works, but I could try and figure it out if you > like. > > The Tab key is necessary here as the menu traps all keyboard events, so you > can't tab out of the menu by using the Tab key unless we look for it here. So does it work to just check for Tab (and maybe some other keys) and bail out of handleKeyDown without doing anything? That makes more sense to me, rather than hiding the menu bar. Trapping most keys is useful, trapping ALL keys is sometimes a bad idea.
As well as the changes below, I also made a change to my focusSelectedItem() method to set the selectedIndex to a valid value if it isn't already. http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right): http://codereview.chromium.org/10966027/diff/7001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/menu_button.js:183: case 'U+0009': // Tab On 2012/09/24 22:48:56, Dominic Mazzoni wrote: > On 2012/09/24 22:39:38, aboxhall wrote: > > On 2012/09/24 17:49:06, Dominic Mazzoni wrote: > > > Specifically trapping the tab key worries me a bit. > > > > > > I wonder if a slightly smarter 'blur' handler would be the least brittle > > > solution? If the user focuses something on the page outside of the menu > button > > > or menu items, we should close the menu. But just pressing Tab might not > catch > > > everything, and it's a pretty low-level thing to check for. > > > > > > Things to test: > > > * Clicking on another element on the page, or on the page background, to > blur > > > the menu > > > * Jumping to another focusable element with a screen reader. > > > > So, I did have the same thought while I was working on this, and tried to > > implement this by checking the focused item during the blur event, but it > wasn't > > yet set correctly at that time (it was telling me the focused element was > > document.body). > > OK. If it isn't needed, that's fine. I'm still curious as to why it works - > there must be code somewhere that's triggering it, can you set a breakpoint in > hideMenu and figure out where it's called? Ok, it's triggered by a mouse event at line 78 of this file. So it doesn't actually close when you navigate away using a screen reader. I've fixed it by listening for focusin on the document and checking whether the target is related to the button or menu. > > However, when I tested it out as you suggest above (at least, with the mouse; > I > > didn't actually check with the screen reader as I just used the 'esc' menu > close > > shortcut), it did in fact work. I will double check when I'm in the office > > today, but I'm pretty sure it already behaves sensibly as you describe. To be > > honest, I'm not sure _how_ it works, but I could try and figure it out if you > > like. > > > > The Tab key is necessary here as the menu traps all keyboard events, so you > > can't tab out of the menu by using the Tab key unless we look for it here. > > So does it work to just check for Tab (and maybe some other keys) and bail out > of handleKeyDown without doing anything? That makes more sense to me, rather > than hiding the menu bar. Trapping most keys is useful, trapping ALL keys is > sometimes a bad idea. I agree that trapping all keys seems a bit excessive (for one thing it means you can't easily open a new tab or devtools using a keyboard shortcut while the menu is open). I changed it to only preventDefault and stopPropagation if the menu does something with the keyboard event (the menu's handleKeyDown() method returns a boolean value which seems to be intended for roughly this purpose). I left the handler for Tab here, because without it Tab navigates between menu items rather than going to the next menu, and I thought it was better to have Tab move to the next button (even if the tab order of the buttons is a little counter-intuitive for now).
lgtm Thanks! I'm happy with it like this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/10966027/12001
Change committed as 158550 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
