1456069 - Always display the Frame picker button if the user is on the Options panel

Status: VERIFIED FIXED

(DevTools :: General, defect, P3)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1451592:

(In reply to Brian Birtles (:birtles) from comment #23)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #21)
> > It can be surprising that checking the option "Select an iframe [...]"
> > doesn't result in any visible feedback for the user if the current page
> > doesn't have any iframe.
> > 
> > Any thoughts on how we could mitigate that? Maybe we should remove the
> > option and make the button always available? Or are there plans to modify
> > this further?
> 
> Yes I deliberately made the settings here work even when the button is not
> displayed, but I can see what you mean about potential confusion.
> 
> There are no current plans to modify this further (other than the general
> plan to overhaul the DevTools settings panel).
> 
> Some possibilities I can think of are:
> 
> 1. Remove the option
> 2. Only show the option when the button would be displayed (probably pretty
> confusing too)
> 3. Show the button while the settings panel is open -- perhaps if there are
> no iframes make it translucent and unclickable?
> 
> (3) is probably nicest and most consistent with Firefox's "Customize" mode.
> It's also what we'll probably eventually want if we later do a similar sort
> of customize mode for DevTools or support re-ordering these buttons. However
> it's unlikely we could do (3) in 61 timeframe due to other bugs we need to
> fix before Golden Week. I'd lean towards doing (1) if we need to do
> something here.

The goal of this bug is to always show the Frame picker button if the user is on the options page, but in a disabled state and with a custom title.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1456069 - Remove inaccurate comment in toolbox controller;

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1456069 - Always show frame button if user is on options panel;

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

Comment 3

[Brian Birtles (:birtles)]

Julian, these patches look great. Thank you!

I've also checked that these patches largely fix bug 1456788 too.

Is there anything blocking you from putting these up for review?

I've reviewed them myself and they look good to me, but you probably want a DevTools person to do the proper review.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for having a look at the patches. I was unsure about pushing this forward in 61 cycle. I felt like this could use some baking time on Nightly before shipping. But I think we can move to review now, and uplift if needed. I will just try to add a test before.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8970106 [details]
Bug 1456069 - Remove inaccurate comment in toolbox controller;

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8970107 [details]
Bug 1456069 - Always show frame button if user is on options panel;

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

Comment 7

[Julian Descottes [:jdescottes]]

Added a small test, here's a try push: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fb5418ddf359ec21d5bbd258669be33fcda0c34

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8970106 [details]
Bug 1456069 - Remove inaccurate comment in toolbox controller;

https://reviewboard.mozilla.org/r/238888/#review247984

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8970107 [details]
Bug 1456069 - Always show frame button if user is on options panel;

https://reviewboard.mozilla.org/r/238890/#review247990

Great, looks reasonable to me!  Should Victoria check the UX as well?

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the reviews!

> Should Victoria check the UX as well?

Good point, I'll attach a screen recording.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: frames-button-always-visible-options-panel.gif

Victoria: to add some user feedback when clicking on the checkbox for the "frame picker", this patch forces the display of the button when the user is on the options panel, if the checkbox is checked. 

If the page doesn't contain several frames to select, the button looks disabled, with a title explaining why it is disabled ("This button is only available on pages with several iframes"). Then if the user leaves the options panel, the button becomes hidden.

Does that sound ok to you?

Comment 12

[Victoria Wang [:victoria]]

This looks great. Thanks for the screencast!

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8970107 [details]
Bug 1456069 - Always show frame button if user is on options panel;

https://reviewboard.mozilla.org/r/238890/#review248084

This is excellent, particularly the test.

We might want to check DAMP results, however. According to bug 1451592 comment 26 the extra call to setToolboxButtons I added in that bug regressed reload tests and this likewise adds a call to setToolboxButtons.

Comment 14

[Julian Descottes [:jdescottes]]

Thanks for the reviews! New try push, added a missing await in the test, which lead to leaks: 

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

Pushed to DAMP:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0c5e6721a1d11aede292a67e21db21996c1fe8a4&newProject=try&newRevision=3a13e0e1b17a3e1b0c019c09c1a6d50d0e91bf81&framework=1

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8970107 [details]
Bug 1456069 - Always show frame button if user is on options panel;

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

Comment 16

[Julian Descottes [:jdescottes]]

This new patch doesn't seem to regress performances, landing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/416f094c8570
Remove inaccurate comment in toolbox controller;r=jryans
https://hg.mozilla.org/integration/autoland/rev/37074feaa478
Always show frame button if user is on options panel;r=birtles,jryans

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/416f094c8570
https://hg.mozilla.org/mozilla-central/rev/37074feaa478

Comment 19

[Brian Birtles (:birtles)]

Julian, do you think we can apply to uplift this to 61? It's still early in the cycle and it mitigates bug 1456788 which QA identified as something they'd like fixed in 61.

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: bug1456069-beta.patch

Sure, we can try to uplift this.

Rebased on top of beta, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc005f24f697bd51c00cd395d46646458ff6b1b0

Approval Request Comment
[Feature/Bug causing the regression]:1451592
[User impact if declined]:flickering effect with one of the devtools icons
[Is this code covered by automated tests?]:yes, included in this patch
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: simple javascript fix covered by test, only impacting the devtools options panel
[String changes made/needed]: toolbox.frames.disabled.tooltip in devtools/client/locales/en-US/toolbox.properties

Comment 21

[Victoria Wang [:victoria]]

Comment on attachment 8973842 [details]
frames-button-always-visible-options-panel.gif

Forgot to set the review flag

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Fixes a pretty distracting flickering icon as reported by QA in bug 1456788 and includes an automated test. Approved for 61.0b6.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Actually, no, this can't land as-is because it adds new strings. Please attach a patch without string changes and re-request approval.

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: bug1456069-beta.patch

Here is a new version without any new string. Rather than hardcoding the english string, I removed the title. 

I was under the impression we could still uplift localized strings? We did so in https://bugzilla.mozilla.org/show_bug.cgi?id=1444327

Comment 25

[Ryan VanderMeulen [:RyanVM]]

If you want to land the string change, you're welcome to run it past flod and get his blessing :). It just needs approval from someone on the l10n team to appease our commit hooks.

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Ah thanks, was not sure about the process.
Francesco, do you think the string change in this patch can be uplifted to beta?

Comment 27

[Francesco Lodolo [:flod]]

Yes, it's OK to uplift (1 string, relatively low visibility, early enough in the cycle).

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Thanks Francesco! 

Ryan: I can't reflag for approval-mozilla-beta, can you do it or do I need to upload a new patch?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Approved for 61.0b6 now that it has flod's blessing!

Comment 30

[Julian Descottes [:jdescottes]]

Attachment: bug1456069-beta.rebased.patch

Just rebased on latest beta.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0315d341f73f

Comment 32

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

I have reproduced this issue using Firefox 62.0a1 (2018.04.23) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b11 and 62.0a1 (latest nightly) on Win 10 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.

Comment 33

[Irene Smith]

Added the following paragraph to the settings page:

As of Firefox 62, if the option to "Select an iframe as the currently targeted document" is checked, the icon will appear in the toolbar while the Settings tab is displayed, even if the current page doesn't include any iframes.

I also added this to the release notes:

If the option to "Select an iframe as the currently targeted document" is checked, the icon will appear in the toolbar while the Settings tab is displayed, even if the current page doesn't include any iframes.

Settings page: https://developer.mozilla.org/en-US/docs/Tools/Settings
Release notes: https://developer.mozilla.org/en-US/Firefox/Releases/62

Comment 34

[Julian Descottes [:jdescottes]]

Looks good to me! Thanks for updating the screenshot as well. Regarding the wording, it seems like https://developer.mozilla.org/en-US/docs/Tools/Settings prefers to use "Settings pane" rather than "Settings tab" though I'm not sure which one is best here.