Julian Descottes [:jdescottes] Assignee
	Description • 6 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 3 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 4 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 7 • 6 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 6 years ago
mozreview-review

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

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 9 • 6 years ago
mozreview-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?

	Julian Descottes [:jdescottes] Assignee
	Comment 10 • 6 years ago

Thanks for the reviews!

> Should Victoria check the UX as well?

Good point, I'll attach a screen recording.

	Julian Descottes [:jdescottes] Assignee
	Comment 11 • 6 years ago

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?

	Victoria Wang [:victoria]
	Comment 12 • 6 years ago

This looks great. Thanks for the screencast!

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 13 • 6 years ago
mozreview-review

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 14 • 6 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 16 • 6 years ago

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

	Pulsebot
	Comment 17 • 6 years ago

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

	Cristina Coroiu [:ccoroiu]
	Comment 18 • 6 years ago
bugherder

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 19 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 20 • 6 years ago

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

	Victoria Wang [:victoria]
	Comment 21 • 6 years ago

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

Forgot to set the review flag

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 24 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 26 • 6 years ago

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?

	Francesco Lodolo [:flod]
	Comment 27 • 6 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 28 • 6 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 6 years ago

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

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

	Julian Descottes [:jdescottes] Assignee
	Comment 30 • 6 years ago

Just rebased on latest beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 6 years ago
bugherder uplift

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

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 32 • 6 years ago

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.

	Irene Smith
	Comment 33 • 6 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 34 • 6 years ago

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.