Closed Bug 1451592 Opened 6 years ago Closed 6 years ago

Hide iframes button from toolbar when there are no iframes in the doc.

Categories

(DevTools :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Spinning this off from bug 1444301.

As per:

  https://mozilla.invisionapp.com/share/M5G8OO1ZVE4#/screens/283871189

We should hide the iframes button when there are no iframes in the doc.

The hardest bit is probably making sure we respond to dynamic changes correctly (e.g. when the first iframe / last iframe is dropped from the parent doc) but it looks like we already get "frame-update" messages in the toolbox so hopefully there's not too much extra plumbing involved.

We should also drop the separator before the RDM button when it is the only button.

Might it be worth giving a label to the button too like Safari does ("Main frame") to bring attention to the button when it is there?
I think this mostly just involves making _updateFrames in devtools/client/framework/toolbox.js call a method on this.component. It could be something like `setFrames` but perhaps for this bug it could just be `hasChildFrames` (`hasSubframes`?) that takes a bool indicating if the number of frames is > 1.

I'm not sure if ToolboxController should then filter the set of toolbox buttons it passes to ToolboxToolbar, or if we should propagate the 'hasSubframes' flag to ToolboxToolbar and let it do the filtering. I suspect it should be the former.

This would be nice to fix next week if possible. I'll try to put up a patch on Monday. CC'ing Hiro though in case I need his help!
> Might it be worth giving a label to the button too like Safari does ("Main frame") to bring attention to the button when it is there?

I'm thinking we should leave this to a separate bug.
Slightly updated version:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f62a44619f2c565db2842bc701e263a036426f2

Completely different approach:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42848cdde4e963443e574a1c5eb404ce28d167f5

The former is more React-like, the latter is more consistent with what we already do and should hopefully pass all the tests.
(I forgot to include the animation fixes in the second try, but I will include them when I put this up for review.)
Comment on attachment 8968260 [details]
Bug 1451592 - Hide the iframe button when there are no child iframes;

https://reviewboard.mozilla.org/r/236940/#review242736

Thanks, this looks reasonable overall! :)

::: devtools/client/framework/components/toolbox-toolbar.js:182
(Diff revision 1)
>    // Add the appropriate separator, if needed.
>    let children = renderedButtons;
>    if (renderedButtons.length) {
>      // For the end group we add a separator *before* the RDM button if it
> -    // exists.
> -    if (rdmIndex !== -1) {
> +    // exists, but only if it is not the only button.
> +    if (!isStart) {

Having a negated condition followed by an else block reads a bit strangely to me...  Consider flipping the blocks and removing negation.

::: devtools/client/framework/toolbox.js:2317
(Diff revision 1)
>      if (!topFrameSelected && this.selectedFrameId) {
>        this._framesButtonChecked = false;
>      }
> +
> +    // We may need to hide/show the frames button now.
> +    this.updateToolboxButtonsVisibility();

This seems like it will do some extra work by looping over all the buttons.  Could we instead do:

```
this.frameButton.isVisible = this._commandIsVisible(this.frameButton);
this.component.setToolboxButtons(this.toolbarButtons);
```
Attachment #8968260 - Flags: review?(jryans) → review+
Comment on attachment 8968261 [details]
Bug 1451592 - Only transition the background-color of buttons;

https://reviewboard.mozilla.org/r/236942/#review242740

Good catch!
Attachment #8968261 - Flags: review?(jryans) → review+
Comment on attachment 8968262 [details]
Bug 1451592 - Simplify browser_toolbox_options_disable_buttons.js;

https://reviewboard.mozilla.org/r/236944/#review242742
Attachment #8968262 - Flags: review?(jryans) → review+
Thanks for the reviews!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Having a negated condition followed by an else block reads a bit strangely
> to me...  Consider flipping the blocks and removing negation.

Sounds good!

> This seems like it will do some extra work by looping over all the buttons. 
> Could we instead do:
> 
> ```
> this.frameButton.isVisible = this._commandIsVisible(this.frameButton);
> this.component.setToolboxButtons(this.toolbarButtons);
> ```

I'm a little bit hesitant about this. We only run this code when the set of iframes is changed (i.e. probably quite infrequently) so it doesn't seem important to optimize out half a dozen iterations of a fairly simple loop. Instead I'd slightly prefer to keep this logic in one place since it's simpler and if updateToolboxButtonsVisibility ends up requiring any further steps we'll possibly forget to add them here.

Anyway, I've made the suggested change, but I thought it was worth mentioning the trade off as I understand it since perhaps there's something I'm overlooking (e.g. the pref lookups being quite costly and introducing jank in the past).
(In reply to Brian Birtles (:birtles, travelling 7~18 April) from comment #14)
> I'm a little bit hesitant about this. We only run this code when the set of
> iframes is changed (i.e. probably quite infrequently) so it doesn't seem
> important to optimize out half a dozen iterations of a fairly simple loop.
> Instead I'd slightly prefer to keep this logic in one place since it's
> simpler and if updateToolboxButtonsVisibility ends up requiring any further
> steps we'll possibly forget to add them here.
> 
> Anyway, I've made the suggested change, but I thought it was worth
> mentioning the trade off as I understand it since perhaps there's something
> I'm overlooking (e.g. the pref lookups being quite costly and introducing
> jank in the past).

The prefs are the main cost I was thinking of.  We've been a bit lax at thinking about performance with DevTools historically; these days we're fighting hard to have great performance, so it's probably worth avoiding the pref lookups here in one way or another.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00c968ee4e0c
Hide the iframe button when there are no child iframes; r=jryans
https://hg.mozilla.org/integration/autoland/rev/2ccaad035035
Only transition the background-color of buttons; r=jryans
https://hg.mozilla.org/integration/autoland/rev/e5d9b47a10e0
Simplify browser_toolbox_options_disable_buttons.js; r=jryans
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?
Flags: needinfo?(bbirtles)
(illustrates previous comment)
(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.
Flags: needinfo?(bbirtles)
Thanks for the feedback! Works for me, logged Bug 1456069 to implement option 3 and added some WIP patches.
I have reproduced this bug with Nightly 61.0a1 (2018-04-04) on ManjaroLinux 17.1.8, 64 bit!

The bug's fix is now verified on Latest Nightly.

Build ID 	20180506100126
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [bugday-20180509]
This patches regressed performance of many tests on DAMP:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=703c14f284211de080c95cccfa317b8e2c8ffb76&newProject=try&newRevision=af1e9bad6918a2b04844f0ddf489c0e14fff1438&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

I think it is related to the call made to setToolboxButtons from `_updateFrames` as :pbro reported in bug 1455645.
The concerning regressions are the one made to *.reload tests. The tools are slower to update after a page reload.
Brian, could you look into the performance regression of this patch?
Flags: needinfo?(bbirtles)
Yes, it won't be until next week though I'm afraid.
Filed bug 1463026 for the DAMP regression.
Flags: needinfo?(bbirtles)
I have reproduced this issue using Firefox  61.0a1 (2018.04.04) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b9 on Win 10 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.