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)
DevTools
General
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?
Assignee | ||
Comment 1•6 years ago
|
||
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!
Assignee | ||
Comment 2•6 years ago
|
||
> 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.
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf6ce8e19652fbb22cab38f00572bd4d71946a98
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
(I forgot to include the animation fixes in the second try, but I will include them when I put this up for review.)
Assignee | ||
Comment 7•6 years ago
|
||
Latest attempt including test simplifications: https://treeherder.mozilla.org/#/jobs?repo=try&revision=568cbf1d50c9a67ebc5a7ca46228d7d505e1e90a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-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 12•6 years ago
|
||
mozreview-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 13•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/00c968ee4e0c https://hg.mozilla.org/mozilla-central/rev/2ccaad035035 https://hg.mozilla.org/mozilla-central/rev/e5d9b47a10e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 21•6 years ago
|
||
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)
Comment 22•6 years ago
|
||
(illustrates previous comment)
Assignee | ||
Comment 23•6 years ago
|
||
(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)
Comment 24•6 years ago
|
||
Thanks for the feedback! Works for me, logged Bug 1456069 to implement option 3 and added some WIP patches.
Comment 25•6 years ago
|
||
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]
Comment 26•6 years ago
|
||
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.
Comment 27•6 years ago
|
||
Brian, could you look into the performance regression of this patch?
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 28•6 years ago
|
||
Yes, it won't be until next week though I'm afraid.
Assignee | ||
Comment 29•6 years ago
|
||
Filed bug 1463026 for the DAMP regression.
Flags: needinfo?(bbirtles)
Comment 30•6 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•