1457067 - [Tab Bar Navigation] The "Settings F1" button will not close the Settings Panel after its opened

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P3)

Description

[Rares Doghi, Desktop QA]

[Affected versions]: 
Nightly 61.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Windows 7 x 32, Mac OS X 10.13 and Ubuntu 16.04 x64.

[Steps to reproduce]:

1. Open Firefox 
2. Open the Inspector by pressing F12 
3. Click on the Meatball Menu.
4. Click the "Settings F1" button from the Meatball Menu.
5. After the Settings panel opens Click the "Settings F1" button from the Meatball Menu.

[Expected result]:
Clicking the Settings F1 button a second time should close the Settings panel.

[Actual result]:
Clicking the Settings F1 button a second time will not close the Settings Panel and the user has to click F1 directly in order to close it.

Comment 1

[Patrick Brosset <:pbro>]

Good point. The settings panel used to be a tab panel jus like the inspector.
It used to have a button in the toolbar, and once you selected it, the button would become "selected", in the same way that the inspector tab becomes selected when you click on it.
So, it made sense that it wouldn't close if you clicked the button again.

Now, with bug 1444299, the settings panel is just an option in the meatball menu. So it might make sense to implement the expected results in comment 0.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

Review commit: https://reviewboard.mozilla.org/r/243024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243024/

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1457067 - Part 2: Add test whether options panel toggled by key event and Settings on the meatball menu.

Review commit: https://reviewboard.mozilla.org/r/243026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243026/

Comment 4

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19e393ab132c690974c3cda8757eafc8458d2001

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8974639 [details]
Bug 1457067 - Part 2: Add test whether options panel toggled by key event and Settings on the meatball menu.

https://reviewboard.mozilla.org/r/243026/#review248844

Very nice test, thanks!

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8974639 [details]
Bug 1457067 - Part 2: Add test whether options panel toggled by key event and Settings on the meatball menu.

https://reviewboard.mozilla.org/r/243026/#review248844

Very nice test, thanks!

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8974638 [details]
Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

https://reviewboard.mozilla.org/r/243024/#review248840

Looks good, thanks Daisuke. Only one small suggestion.

::: devtools/client/framework/toolbox.js:845
(Diff revision 1)
> -        this.selectTool(this.lastUsedToolId, "toggle_settings_off");
> -      } else {
> -        this.selectTool("options", "toggle_settings_on");
> -      }
> -    };
>      this.shortcuts.on(L10N.getStr("toolbox.help.key"), selectOptions);

nit: we could directly use this.toggleOptions here, the intermediary variable doesn't bring much.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8974638 [details]
Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

https://reviewboard.mozilla.org/r/243024/#review248840

> nit: we could directly use this.toggleOptions here, the intermediary variable doesn't bring much.

Indeed! Thanks, Julian!

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8974638 [details]
Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243024/diff/1-2/

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8974639 [details]
Bug 1457067 - Part 2: Add test whether options panel toggled by key event and Settings on the meatball menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243026/diff/1-2/

Comment 11

[Daisuke Akatsuka (:daisuke)]

The try of updated patch was green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f607f97aa451b8983c09fd967e5a0e24ef398418
Let me make the patches to land!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fddb6d7c89b9
Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c43f79a262bf
Part 2: Add test whether options panel toggled by key event and Settings on the meatball menu. r=jdescottes

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/fddb6d7c89b9
https://hg.mozilla.org/mozilla-central/rev/c43f79a262bf

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this worth backport consideration or can it ride the 62 train?

Comment 15

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8974638 [details]
Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1444301
[User impact if declined]: Can't toggle settings with menu
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Part 2 contains automated test
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Baked on Nightly, has automated test, small front end patch
[String changes made/needed]: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8974638 [details]
Bug 1457067 - Part 1: Implement so as to toggle the options panel if click 'Settings' on the meatball menu.

Thanks for adding a new test for this regression. Approved for 61.0b6.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3f260301d0e
https://hg.mozilla.org/releases/mozilla-beta/rev/d7cc3e2fa65b

Comment 18

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  61.0a1 (2018.04.26) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b9 and 62.0a1 on Win 8.1 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.