Closed Bug 1456069 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: General, defect, P3)

61 Branch
defect

Tracking

(firefox61 verified, firefox62 verified)

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

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 2 obsolete files)

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.
Blocks: 1444299
Blocks: 1456788
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.
Flags: needinfo?(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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Comment on attachment 8970106 [details]
Bug 1456069 - Remove inaccurate comment in toolbox controller;

https://reviewboard.mozilla.org/r/238888/#review247984
Attachment #8970106 - Flags: review?(jryans) → review+
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?
Attachment #8970107 - Flags: review?(jryans) → review+
Thanks for the reviews!

> Should Victoria check the UX as well?

Good point, I'll attach a screen recording.
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?
Attachment #8973842 - Flags: ui-review?(victoria)
This looks great. Thanks for the screencast!
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.
Attachment #8970107 - Flags: review?(bbirtles) → review+
This new patch doesn't seem to regress performances, landing.
Pushed by jdescottes@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/416f094c8570
https://hg.mozilla.org/mozilla-central/rev/37074feaa478
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Flags: needinfo?(jdescottes)
Attached patch bug1456069-beta.patch (obsolete) — Splinter Review
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
Flags: needinfo?(jdescottes)
Attachment #8975723 - Flags: approval-mozilla-beta?
Comment on attachment 8973842 [details]
frames-button-always-visible-options-panel.gif

Forgot to set the review flag
Attachment #8973842 - Flags: ui-review?(victoria) → ui-review+
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.
Attachment #8975723 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8975723 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Attached patch bug1456069-beta.patch (obsolete) — Splinter Review
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
Attachment #8975723 - Attachment is obsolete: true
Attachment #8976396 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(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?
Attachment #8975723 - Attachment is obsolete: false
Flags: needinfo?(jdescottes) → needinfo?(francesco.lodolo)
Yes, it's OK to uplift (1 string, relatively low visibility, early enough in the cycle).
Flags: needinfo?(francesco.lodolo)
Attachment #8976396 - Attachment is obsolete: true
Attachment #8976396 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(ryanvm)
Comment on attachment 8975723 [details] [diff] [review]
bug1456069-beta.patch

Approved for 61.0b6 now that it has flod's blessing!
Flags: needinfo?(ryanvm)
Attachment #8975723 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Just rebased on latest beta.
Attachment #8975723 - Attachment is obsolete: true
Attachment #8976542 - Flags: approval-mozilla-beta?
Attachment #8976542 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
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
Flags: needinfo?(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.
Flags: needinfo?(jdescottes)
You need to log in before you can comment on or make changes to this bug.