>>> My Info: Win7_64, Nightly 50, 32bit, ID 20160726080520 (2016-07-26) STR_1: 1. Open devtools 2. Focus iframes toolbarbutton using keyboard 3. Press Alt+Down AR: No visible action ER: List of iframes should appear, because dropmarker near button is clear indication that Alt+Down should open the list This is regression from bug 1266419. Regression range: > https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=3a87296fe4145138c2ce15512bb31f76fe869cb4&tochange=42fab251fe111d5f891c9bde0ee1fb6f7f946a50@ Jan Honza Odvarko [:Honza] (PTO: 12/09-01/01): It seems that this is a regresion caused by your change. Please have a look.

Honza, maybe this is an easy fix you could mentor?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1) > Honza, maybe this is an easy fix you could mentor? Sure, I can do it. So here is how I think it should be done: 1) Append a onKeyDown event handler prop to the `button` element. Something like as follows: return button({ ... onKeyDown: (event) => { ... } }); It should be here (in ToolboxToolbar React component): https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/devtools/client/framework/components/toolbox-toolbar.js#146 2) This even handler should delegate the event to a callback specified in `command` object. This object represents/describes the toolbar-button. Of course the callback doesn't have to be specified if the button doesn't handle key down events. E.g. onKeyDown: (event) => { command.onKeyDown(event); } 3) The 'list of frames' button is built here (i.e. the `command` object description) https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/devtools/client/framework/toolbox.js#1065 It should specify the `onKeyDown` callback and implement it (i.e. support for Alt+Down). The implementation should probably utilize KeyShortcuts module ("devtools/client/shared/key-shortcuts"). It's already included in toolbox.js file. https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/devtools/client/framework/toolbox.js#55 --- @Greg: since you implemented the ToolboxToolbar React component, does this make sense? Honza

i want to work on this bug, is any body already working on it ?

(In reply to Abhishek Patel from comment #3) > i want to work on this bug, is any body already working on it ? Feel free to get started! If you have questions, use the needinfo field on the bug set to "mentor" or you can ask in #devtools on IRC.

Honza, that all sounds reasonable!

Hey, I don't see how to "focus iframes toolbar button using keyboard". I browsed the list of shortcuts here: https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts Closest I've gotten is to cycle tools with "ctrl + ]", but it skips over the iframe button. Could you help clarify the repro steps, Honza? Thanks.

(In reply to Steve Jarvis [:sajarvis] from comment #6) > Hey, I don't see how to "focus iframes toolbar button using keyboard". I > browsed the list of shortcuts here: > https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts > > Closest I've gotten is to cycle tools with "ctrl + ]", but it skips over the > iframe button. Could you help clarify the repro steps, Honza? Thanks. You can click on the button, press escape to close the drop-down. At this point, the button should be focused and you can press Alt+Down to check whether the drop-down opens again. (I don't see a way how to do this using keyboard) Honza

Too late for a fix for 53, fix-optional for 54, minor carryover regression.

Comment on attachment 8906353 [details] Bug 1327971 - Add support to show list of frames on key-press of "Alt+Down" on the "iframes" button. https://reviewboard.mozilla.org/r/178078/#review183740 Looks grat, thanks for working on this! Just a few nit comments: R+ assuming my comments are resolved and the try is green. try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f882d8463921e009bd0d5b66ed4e42e1a70b4a15 Honza ::: devtools/client/framework/components/toolbox-toolbar.js:110 (Diff revision 1) > } > > return div({id: `toolbox-buttons-${isStart ? "start" : "end"}`}, > ...visibleButtons.map(command => { > - const {id, description, onClick, isChecked, className: buttonClass} = command; > + const {id, description, onClick, isChecked, > + className: buttonClass, onKeyDown} = command; nit: please put every var on separate line ::: devtools/client/framework/toolbox.js:116 (Diff revision 1) > this._toolRegistered = this._toolRegistered.bind(this); > this._toolUnregistered = this._toolUnregistered.bind(this); > this._refreshHostTitle = this._refreshHostTitle.bind(this); > this._toggleNoAutohide = this._toggleNoAutohide.bind(this); > this.showFramesMenu = this.showFramesMenu.bind(this); > + this.handleKeyDownOnFramesBtn = this.handleKeyDownOnFramesBtn.bind(this); nit: please rename to `handleKeyDownOnFramesButton` (to avoid btn shortcut) ::: devtools/client/framework/toolbox.js:693 (Diff revision 1) > * the button should be displayed as toggled on. > */ > _createButtonState: function (options) { > let isCheckedValue = false; > const { id, className, description, onClick, isInStartContainer, setup, teardown, > - isTargetSupported, isChecked } = options; > + isTargetSupported, isChecked, onKeyDown } = options; nit: please put every var on separate line

Hi Honza, Thanks for the review. I've updated the moz-review request by incorporating your comments.

Thanks for the update, ready to land! Honza

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cb9cbed526b9 Add support to show list of frames on key-press of "Alt+Down" on the "iframes" button. r=Honza

Is "Alt+Down" supposed to be localized, or kept in English (like "CmdOrCtrl+Shift+D")?

I think this was reported for Win7_64, so am not sure if OSX has this issue.

(In reply to Abhinav Koppula from comment #17) > I think this was reported for Win7_64, so am not sure if OSX has this issue. You didn't answer the question: if a locale uses "Alt+sometranslation", is it still going to work? Is it a shortcut definition or a shortcut description? Trying with :honza

(In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than usual) from comment #18) > (In reply to Abhinav Koppula from comment #17) > > I think this was reported for Win7_64, so am not sure if OSX has this issue. > > You didn't answer the question: if a locale uses "Alt+sometranslation", is > it still going to work? Is it a shortcut definition or a shortcut > description? Trying with :honza It's a shortcut definition, which is being parsed here: http://searchfox.org/mozilla-central/source/devtools/client/shared/key-shortcuts.js#96 So, if a locale uses different key, it'll work with different key. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #19) > It's a shortcut definition, which is being parsed here: > http://searchfox.org/mozilla-central/source/devtools/client/shared/key- > shortcuts.js#96 > > So, if a locale uses different key, it'll work with different key. Thanks, we should make sure to add a comment in these cases. I'm going to fix a few locales that translated it.

I have reproduced this bug according to (2017-01-01) Fixing bug is verified on Latest Nightly-- Build ID :20171223100103 User Agent :Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0 Tested OS-- Windows7 32bit

(In reply to Saheda Reza [:Antora] from comment #21) > I have reproduced this bug according to (2017-01-01) > > Fixing bug is verified on Latest Nightly-- > Build ID :20171223100103 > User Agent :Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0 Also verified on Latest Beta-- Build ID :20171218174357 User Agent :Mozilla/5.0 (Windows NT 6.1; rv:58.0) Gecko/20100101 Firefox/58.0 Tested OS-- Windows7 32bit