Closed Bug 1457067 Opened 2 years ago Closed Last year

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

Categories

(DevTools :: General, enhancement, P3)

All
Unspecified
enhancement

Tracking

(firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: rares.doghi, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

[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.
Blocks: 1444299
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.
Severity: normal → enhancement
Priority: -- → P3
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
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!
Attachment #8974639 - Flags: review?(jdescottes) → review+
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 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.
Attachment #8974638 - Flags: review?(jdescottes) → review+
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!
The try of updated patch was green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f607f97aa451b8983c09fd967e5a0e24ef398418
Let me make the patches to land!
Pushed by dakatsuka@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/fddb6d7c89b9
https://hg.mozilla.org/mozilla-central/rev/c43f79a262bf
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Is this worth backport consideration or can it ride the 62 train?
Flags: needinfo?(dakatsuka)
Flags: in-testsuite+
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
Flags: needinfo?(dakatsuka)
Attachment #8974638 - Flags: approval-mozilla-beta?
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.
Attachment #8974638 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.