Closed Bug 1327971 Opened 3 years ago Closed 3 years ago

Alt+Down shortcut no longer opens list of iframes on the button with dropmarker

Categories

(DevTools :: Framework, defect, P3)

49 Branch
defect

Tracking

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- verified

People

(Reporter: arni2033, Assigned: abhinav.koppula, Mentored)

References

Details

(Keywords: regression)

Attachments

(1 file)

>>>   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.
No longer blocks: 1277113
Component: Untriaged → Developer Tools: Framework
Honza, maybe this is an easy fix you could mentor?
Flags: needinfo?(odvarko)
Priority: -- → P3
(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
Mentor: odvarko
Flags: needinfo?(odvarko) → needinfo?(gtatum)
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!
Flags: needinfo?(gtatum)
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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
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
Attachment #8906353 - Flags: review?(odvarko) → review+
Hi Honza,
Thanks for the review.
I've updated the moz-review request by incorporating your comments.
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb9cbed526b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Is "Alt+Down" supposed to be localized, or kept in English (like "CmdOrCtrl+Shift+D")?
Flags: needinfo?(abhinav.koppula)
I think this was reported for Win7_64, so am not sure if OSX has this issue.
Flags: needinfo?(abhinav.koppula)
(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
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
(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
QA Whiteboard: [bugday-20171220]
(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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.