Closed Bug 1397447 Opened 7 years ago Closed 7 years ago

Automatically hide/show the downloads button by default, providing an option to have it permanently visible

Categories

(Firefox :: Downloads Panel, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 3 open bugs, Blocks 2 open bugs)

Details

(Whiteboard: [reserve-photon-structure][photon-l10n-risk])

Attachments

(3 files)

Per some looong discussions, we'll: - automatically show/hide the downloads button by default, based on whether that session has downloads or not - have a hidden pref to turn that behaviour on/off - have some TBD UI to toggle said pref (maybe in customize mode footer, maybe context menu, maybe in the downloads panel itself, maybe in a separate dropdown panel that appears when moving the item in customize mode... or maybe something else) - show the item in customize mode even if auto-hidden "normally" (but maybe with a "ghost"-ish dotted icon, if it's in the toolbar), with users able to put it where they want - the default position of the button will be next to the urlbar(/searchbar)(/spacer(s)), as before. We will probably add a migration step to put the button there, given that it now auto-hides. Users can obviously move the button if they don't want it there (similar to the sidebar/library/bookmarks menu migrations we've already done). edgecases: - it could be confusing there's an extra icon in customize mode that isn't there when leaving. Hopefully the added UI toggle for the hidden pref and/or the ghost icon and/or an updated tooltip (TBD, depends on UI) will help make this clearer to users - the overflow panel is a bit awkward - toggling the overflow button based on whether there are downloads as well, if auto-hide is turned on, would be a bit nightmarish. Instead, we will practically speaking not autohide when the button is in there, and any UI (see above) would be greyed out as long as that's where the button was. I'm going to start work on the implementation side of things, and hopefully UI will firm up in the next day or two and we can still land this in the next week or so. This will likely need 1 or 2 strings for UI, but not too much change given that the previous attempt to do this was already backed out so generally speaking what we need will be in the tree already. Still, CC'ing flod so he's aware. I'll do my best to move as quickly as possible here to give l10n maximum space (though, again, it shouldn't be a lot of strings).
Flags: qe-verify+
Status: NEW → ASSIGNED
QA Contact: gwimberly
Whiteboard: [photon-structure][photon-l10n-risk] → [reserve-photon-structure][photon-l10n-risk]
Will the "Show all downloads" button from the downloads panel be mirrored in the library panel in order to reach it while the download button is hidden?
(In reply to Albert Scheiner [:alberts] from comment #1) > Will the "Show all downloads" button from the downloads panel be mirrored in > the library panel in order to reach it while the download button is hidden? Yes, this is bug 1354532 and not part of this bug, and should be in nightly sometime today (given it was landed on autoland yesterday).
This is obviously still missing the UI, but the fundamental code changes are pretty solid, I think/hope, and I added fairly extensive automated test coverage (I think), so I'm confident this is ready for reviews (and maybe more iterations) while we wait for a UI/UX spec.
Comment on attachment 8905926 [details] Bug 1397447 - make downloads button autohide by default, https://reviewboard.mozilla.org/r/177722/#review182800 It looks good, I'll have a second look though, since it's a larger than expected patch (fwiw I still think magic toolbarbuttons increase our technical debt, but I won't block on my opinion). ::: browser/base/content/browser.css:699 (Diff revision 1) > + */ > +#urlbar-container { > + min-width: calc(49ch + 24px + 2 * var(--toolbarbutton-inner-padding)); > +} > + > +#nav-bar[hasdynamicdownloads] #urlbar-container { I was a bit confused by the naming, since dynamicdownloads could mean a lot of things. Maybe "downloadsbuttonshown" What happens to this solution if/when we'll have a second "magically disappearing" button? I'm tempted to think the magic buttons should just be inside #urlbar-container by default and when moved off the default position, it should become always visible. Though this may break other things expecting it to be a direct child of the toolbar :( I guess for now there's no better solution, it doesn't look great though. ::: browser/base/content/browser.css:700 (Diff revision 1) > +#urlbar-container { > + min-width: calc(49ch + 24px + 2 * var(--toolbarbutton-inner-padding)); > +} > + > +#nav-bar[hasdynamicdownloads] #urlbar-container { > + min-width: 49ch; why the change to all of the other max-widths in this same patch? Is it to cope with Photon mockups when the button is visible? I'd prefer a second opinion on these by someone else actually working on Photon visuals, since I would just blindly approve without full understanding. ::: browser/base/content/browser.js:1325 (Diff revision 1) > gBrowser.addProgressListener(window.XULBrowserWindow); > gBrowser.addTabsProgressListener(window.TabsProgressListener); > > SidebarUI.init(); > > + DownloadsButton.init(); I suppose the reason this is not delayed is to avoid the button flickering on window open? May you add a comment about that here so people won't move it to save startup work and inadvertently cause a regression? ::: browser/components/downloads/content/indicator.js:171 (Diff revision 1) > + }, > + > + hide() { > + let button = this._placeholder; > + if (this.autoHideDownloadsButton && button && button.closest("toolbar")) { > + DownloadsPanel.hidePanel(); This makes me think there may be code around that invokes DownloadsPanel.showPanel(), and we should ensure that unhides the indicator if it's hidden. ::: browser/components/downloads/content/indicator.js:208 (Diff revision 1) > + init() { > + XPCOMUtils.defineLazyPreferenceGetter( > + this, "autoHideDownloadsButton", "browser.download.autohideButton", > + true, this.checkForAutoHide.bind(this)); > + > + CustomizableUI.addListener(this); Does this mean we can remove the manual DownloadsButton.customizeStart/Done(); calls from browser-customization.js? ::: browser/components/downloads/content/indicator.js:552 (Diff revision 1) > - // If the downloads button is in the menu panel, open the Library > - let widgetGroup = CustomizableUI.getWidget("downloads-button"); > - if (widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) { > - DownloadsPanel.showDownloadsHistory(); > - } else { > - DownloadsPanel.showPanel(); > + DownloadsPanel.showPanel(); I hope you tested this deeply, since I have no idea if all of the panel works as expected there, it's very likely untested. ::: browser/components/downloads/test/browser/browser_downloads_autohide.js:9 (Diff revision 1) > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +add_task(async function checkStateDuringPrefFlips() { > + ok(Services.prefs.getBoolPref("browser.download.autohideButton"), "Should be autohiding the button by default"); the whole test needs some indentation for the soft-80 chars wrap... At least moving the test msgs to a new line? ::: browser/components/downloads/test/browser/browser_downloads_autohide.js:13 (Diff revision 1) > +add_task(async function checkStateDuringPrefFlips() { > + ok(Services.prefs.getBoolPref("browser.download.autohideButton"), "Should be autohiding the button by default"); > + ok(!DownloadsIndicatorView.hasDownloads, "Should be no downloads when starting the test"); > + let downloadsButton = document.getElementById("downloads-button"); > + ok(downloadsButton.hasAttribute("hidden"), "Button should be hidden in the toolbar"); > + gCustomizeMode.addToPanel(downloadsButton); What about a registerCleanupFunction that in any case restores the customization state, if the test should ever timeout?
Attachment #8905926 - Flags: review?(mak77)
Comment on attachment 8905927 [details] Bug 1397447 - ensure the button is in the navbar by default, https://reviewboard.mozilla.org/r/177724/#review182822 LGTM
Attachment #8905927 - Flags: review?(mak77) → review+
(In reply to :Gijs from comment #5) > This is obviously still missing the UI, but the fundamental code changes are > pretty solid, I think/hope, and I added fairly extensive automated test > coverage (I think), so I'm confident this is ready for reviews (and maybe > more iterations) while we wait for a UI/UX spec. Gijs, here's the spec for the tooltip behavior we discussed: https://mozilla.invisionapp.com/share/54DEJX5TP#/screens/252504394_Customize_
Comment on attachment 8905926 [details] Bug 1397447 - make downloads button autohide by default, https://reviewboard.mozilla.org/r/177722/#review183438 ::: browser/base/content/browser.css:699 (Diff revision 1) > + */ > +#urlbar-container { > + min-width: calc(49ch + 24px + 2 * var(--toolbarbutton-inner-padding)); > +} > + > +#nav-bar[hasdynamicdownloads] #urlbar-container { I think I originally intended to do something else if the button was dynamically shown vs. permanently in the navbar, but on reflection I don't think that's worth the complexity. Renaming as suggested. As for putting the magic buttons in the urlbar container... that wouldn't currently work with the design (because the spacer and/or searchbar would also have to go in there) and is very complex, so as suggested I'm not doing that right now. Finally, note that this is effectively the same stuff that was already r+'d in bug 1371765 by Paolo and/or Jared. :-) ::: browser/base/content/browser.css:700 (Diff revision 1) > +#urlbar-container { > + min-width: calc(49ch + 24px + 2 * var(--toolbarbutton-inner-padding)); > +} > + > +#nav-bar[hasdynamicdownloads] #urlbar-container { > + min-width: 49ch; The adjustments for the button are here to make sure that other items in the navbar don't overflow when we add the downloads button to the navbar, which would be confusing and potentially cause a lot of jumbling about in the toolbar. See https://bugzilla.mozilla.org/show_bug.cgi?id=1371765#c28 for more details on what's going on with these styles, which hopefully helps. ::: browser/base/content/browser.js:1325 (Diff revision 1) > gBrowser.addProgressListener(window.XULBrowserWindow); > gBrowser.addTabsProgressListener(window.TabsProgressListener); > > SidebarUI.init(); > > + DownloadsButton.init(); Yes, good point. ::: browser/components/downloads/content/indicator.js:171 (Diff revision 1) > + }, > + > + hide() { > + let button = this._placeholder; > + if (this.autoHideDownloadsButton && button && button.closest("toolbar")) { > + DownloadsPanel.hidePanel(); So, https://dxr.mozilla.org/mozilla-central/search?q=downloadspanel.showpanel&=mozilla-central shows 3 non-test callsites. One of which is for when subview hides (so the panel must already be open anyway), one of which is for the onCommand handler (so the button must be shown already anyway) and finally there's https://dxr.mozilla.org/mozilla-central/rev/fd87bb184e299fec695f69bd2977276c25719b98/browser/components/downloads/DownloadsCommon.jsm#850 (which I *think* shouldn't run until we've already added a download and therefore shown the button). So I'm not sure it's really required, but as a belt-and-suspenders fix I'll call unhide() from the showPanel method and add a comment there. It makes sense that that method should "do what I mean". :-) ::: browser/components/downloads/content/indicator.js:208 (Diff revision 1) > + init() { > + XPCOMUtils.defineLazyPreferenceGetter( > + this, "autoHideDownloadsButton", "browser.download.autohideButton", > + true, this.checkForAutoHide.bind(this)); > + > + CustomizableUI.addListener(this); Not outright, but we can move to using the listener, yes. I went ahead and did that. ::: browser/components/downloads/content/indicator.js:552 (Diff revision 1) > - // If the downloads button is in the menu panel, open the Library > - let widgetGroup = CustomizableUI.getWidget("downloads-button"); > - if (widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) { > - DownloadsPanel.showDownloadsHistory(); > - } else { > - DownloadsPanel.showPanel(); > + DownloadsPanel.showPanel(); So, just to be clear, this will open the panel anchored on the overflow button. We already do this (before this patch) when the button is in the 'dynamic' part of the overflow panel. We never open a subview (though we could try to do that in the future, it's out of scope for this bug). So I don't think this needs significantly more testing than before. When the button is in the navbar, it won't go into the dynamic part of the overflow panel anymore (after this patch). It can still be pinned to the permanent part of the overflow panel. The reason I avoided it going into the dynamic overflow is that it would be a nightmare for both users and our code to reason about if/when/how the button enters/leaves the overflow panel if/when it is shown dynamically. Rather than having different behaviour for autohide on/off, I just kept 1 behaviour. In *theory* it should be possible to change this at runtime, but that comes with its own edgecases like multiple windows (ie I toggle autohide in one window in customize mode, while the button is in the overflow part of the navbar in the other window, etc.). This 'ensemble' of changes broke the customizableui overflow test for the download panel, which I adapted (was already in this cset) to move the button to the 'permanent' part of the overflow panel instead, and test there that it opens successfully, anchored on the overflow button. So the automated test coverage gives me additional confidence that this isn't a risky change. ::: browser/components/downloads/test/browser/browser_downloads_autohide.js:9 (Diff revision 1) > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +add_task(async function checkStateDuringPrefFlips() { > + ok(Services.prefs.getBoolPref("browser.download.autohideButton"), "Should be autohiding the button by default"); Done. I wish we had some official policy about this and enforced it with eslint. Right now it seems different people prefer different things. ::: browser/components/downloads/test/browser/browser_downloads_autohide.js:13 (Diff revision 1) > +add_task(async function checkStateDuringPrefFlips() { > + ok(Services.prefs.getBoolPref("browser.download.autohideButton"), "Should be autohiding the button by default"); > + ok(!DownloadsIndicatorView.hasDownloads, "Should be no downloads when starting the test"); > + let downloadsButton = document.getElementById("downloads-button"); > + ok(downloadsButton.hasAttribute("hidden"), "Button should be hidden in the toolbar"); > + gCustomizeMode.addToPanel(downloadsButton); Added.
UI patch incoming. The basic behaviour is specced here: https://mozilla.invisionapp.com/share/54DEJX5TP#/screens/252504394_Customize_-_Downloads_Menu with the following modifications per discussions with Aaron: 1) don't show the tooltip when the button is in the overflow panel (incl. when the user clicks the button) 2) when moving the button via dnd (in customize mode), or the context menu (in or outside customize mode), untick autohide 3) if ticking autohide if the item is in the palette, when exiting customize mode, move item to the toolbar The combination of these two things ((2) and (3)) is a bit annoying to implement in that we can't just do this from within the listener, because it has no idea if the user moved it or if we were moving it because of (3) or a reset or something like that. As a result, I instead added code to the specific codepaths in CustomizeMode.jsm to flip the pref. 4) keep the tooltip open on mouseover 5) on mouseout, hide after 2s We also agreed to use the "default" panel styling as far as box shadow / border / color on the panel is concerned, to avoid having something that then diverges from any updates to the 'default' box shadow etc. again in the future. I hope this makes the patch more or less clear. Feel free to ping me here or on IRC (probably tomorrow...) if you have other questions.
Comment on attachment 8905926 [details] Bug 1397447 - make downloads button autohide by default, https://reviewboard.mozilla.org/r/177722/#review183746 ::: browser/components/downloads/content/indicator.js:162 (Diff revision 2) > + } > + }, > + > + checkForAutoHide() { > + let button = this._placeholder; > + if (!this._customizing && this.autoHideDownloadsButton && button && button.closest("toolbar")) { nit: 80 chars limit, reindent button && ... ::: browser/components/downloads/test/browser/head.js:208 (Diff revision 2) > + * Waits for a given button to become visible. > + */ > +function promiseButtonShown(id) { > + let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > + return BrowserTestUtils.waitForCondition(() => { > + info(`Waiting for button ${id} to have non-0 size`); nit: I'm not sure this is more useful than just passing it as message (second argument) to waitForCondition. The only difference is that this prints it at every polling, vs printing it only on timeout.
Attachment #8905926 - Flags: review?(mak77) → review+
Comment on attachment 8906761 [details] Bug 1397447 - add UI and automatic toggles to make the auto-hide functionality more seamless, https://reviewboard.mozilla.org/r/178482/#review183760 I don't see any blocking issues. I have some fear about the panel timers (sometimes I've seen missing mouseover/out eventsin panels), but it's edge cases. In any case I think it's better to test this in the wild and polish based on such feedback, than delay it further. ::: browser/components/customizableui/CustomizeMode.jsm:680 (Diff revision 1) > removeFromArea(aNode) { > aNode = this._getCustomizableChildForNode(aNode); > if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) { > aNode = aNode.firstChild; > } > + // If the user explicitly moves this item, turn off autohide. s/moves/removes/ I also assume this is to allow removing the button completely, otherwise it would auto-unhide? Maybe the comment could be better. ::: browser/components/customizableui/CustomizeMode.jsm:2307 (Diff revision 1) > + _maybeMoveDownloadsButtonToNavBar() { > + // If the user toggled the autohide checkbox while the item was in the > + // palette, and hasn't moved it since, move the item to the default > + // location in the navbar for them. > + if (!CustomizableUI.getPlacementOfWidget("downloads-button") && > + this._moveDownloadsButtonToNavBar && this.window.DownloadsButton.autoHideDownloadsButton) { nit: indent third condition ::: browser/locales/en-US/chrome/browser/browser.dtd:871 (Diff revision 1) > <!ENTITY customizeMode.uidensity.menuTouch.label "Touch"> > <!ENTITY customizeMode.uidensity.menuTouch.tooltip "Touch"> > <!ENTITY customizeMode.uidensity.menuTouch.accessKey "T"> > <!ENTITY customizeMode.uidensity.autoTouchMode.checkbox.label "Use Touch for Tablet Mode"> > > +<!ENTITY customizeMode.autoHideDownloadsButton.label "Auto-hide"> the only other auto hide entry I found in localization is http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/devtools/client/locales/en-US/toolbox.properties#171 that is "auto hide" (no dash), I wonder if we should go for consistency. Maybe ask l10n? Please ensure the panel will accomodate and not crop other locales, for example in italian it could be a much longer "Nascondi Automaticamente"
Attachment #8906761 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #15) > Comment on attachment 8906761 [details] > Bug 1397447 - add UI and automatic toggles to make the auto-hide > functionality more seamless, > > https://reviewboard.mozilla.org/r/178482/#review183760 > > I don't see any blocking issues. > I have some fear about the panel timers (sometimes I've seen missing > mouseover/out eventsin panels), but it's edge cases. > In any case I think it's better to test this in the wild and polish based on > such feedback, than delay it further. > > ::: browser/components/customizableui/CustomizeMode.jsm:680 > (Diff revision 1) > > removeFromArea(aNode) { > > aNode = this._getCustomizableChildForNode(aNode); > > if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) { > > aNode = aNode.firstChild; > > } > > + // If the user explicitly moves this item, turn off autohide. > > s/moves/removes/ > I also assume this is to allow removing the button completely, otherwise it > would auto-unhide? Maybe the comment could be better. To be honest, it's just to be consistent. We always turn off autohide if the user drags and drops the button or uses the context menu to move it. We wouldn't automatically auto-unhide it unless the user actually ticked the pref in the panel. > ::: browser/locales/en-US/chrome/browser/browser.dtd:871 > (Diff revision 1) > > <!ENTITY customizeMode.uidensity.menuTouch.label "Touch"> > > <!ENTITY customizeMode.uidensity.menuTouch.tooltip "Touch"> > > <!ENTITY customizeMode.uidensity.menuTouch.accessKey "T"> > > <!ENTITY customizeMode.uidensity.autoTouchMode.checkbox.label "Use Touch for Tablet Mode"> > > > > +<!ENTITY customizeMode.autoHideDownloadsButton.label "Auto-hide"> > > the only other auto hide entry I found in localization is > http://searchfox.org/mozilla-central/rev/ > 51eae084534f522a502e4b808484cde8591cb502/devtools/client/locales/en-US/ > toolbox.properties#171 that is "auto hide" (no dash), I wonder if we should > go for consistency. Maybe ask l10n? Per discussion on IRC, gonna keep the dash because that's what was in the spec, but if we need to remove it we can do that without rev'ing string ids. > Please ensure the panel will accomodate and not crop other locales, for > example in italian it could be a much longer "Nascondi Automaticamente" I checked, and this works. This is part of why I didn't set the width of the panel anywhere - layout deals with that based on the length of the string.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s ad1e8c354593 -d e19a2e8394e8: rebasing 419411:ad1e8c354593 "Bug 1397447 - make downloads button autohide by default, r=mak" merging browser/app/profile/firefox.js merging browser/base/content/browser.css merging browser/base/content/browser.js merging browser/base/content/browser.xul merging browser/components/customizableui/CustomizableUI.jsm merging browser/components/downloads/content/indicator.js merging browser/components/downloads/test/browser/browser_overflow_anchor.js warning: conflicts while merging browser/base/content/browser.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/downloads/content/indicator.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/components/downloads/test/browser/browser_overflow_anchor.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6e36419338c3 make downloads button autohide by default, r=mak https://hg.mozilla.org/integration/autoland/rev/364d4279ef9f ensure the button is in the navbar by default, r=mak https://hg.mozilla.org/integration/autoland/rev/b9e11056a978 add UI and automatic toggles to make the auto-hide functionality more seamless, r=mak
I am using the latest hourly build with this patch and download button that I had on the navbar is gone. I find NO UI in 'Options' on Win10 x64 to 'restore' the button as indicated that the patch provides. There is NO button in the 'Customize' panel - and I've tried all three themes, still no button. Where is the UI for this feature supposed to appear -
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #26) > I am using the latest hourly build with this patch and download button that > I had on the navbar is gone. > I find NO UI in 'Options' on Win10 x64 to 'restore' the button as indicated > that the patch provides. > There is NO button in the 'Customize' panel - and I've tried all three > themes, still no button. Does the button appear in customize mode? If you click the downloads button in customize mode, no panel appears? If you move it, no panel appears? Do you see the pref (browser.download.autohideButton) in about:config ? > Where is the UI for this feature supposed to appear - See: (In reply to Aaron Benson from comment #8) > Gijs, here's the spec for the tooltip behavior we discussed: > https://mozilla.invisionapp.com/share/54DEJX5TP#/screens/252504394_Customize_
(In reply to :Gijs from comment #27) > (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #26) > > I am using the latest hourly build with this patch and download button that > > I had on the navbar is gone. > > I find NO UI in 'Options' on Win10 x64 to 'restore' the button as indicated > > that the patch provides. > > There is NO button in the 'Customize' panel - and I've tried all three > > themes, still no button. > > Does the button appear in customize mode? If you click the downloads button > in customize mode, no panel appears? If you move it, no panel appears? Do > you see the pref (browser.download.autohideButton) in about:config ? > > > Where is the UI for this feature supposed to appear - > > See: > > (In reply to Aaron Benson from comment #8) > > Gijs, here's the spec for the tooltip behavior we discussed: > > https://mozilla.invisionapp.com/share/54DEJX5TP#/screens/252504394_Customize_ The download button does NOT appear in Customize mode as I stated above I do not see browser.download.autohideButton in about:config - I added the pref, restarted browser and still does not appear in the Customize panel or in the Options Panel under any of the tabs. Time to do a quick test in a clean profile. Will report back.
Pref is there in a new profile and toggling will show/hide the downloads button. Now on to figuring out why it does not workin my existing profile.
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #28) > (In reply to :Gijs from comment #27) > > (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #26) > > > I am using the latest hourly build with this patch and download button that > > > I had on the navbar is gone. > > > I find NO UI in 'Options' on Win10 x64 to 'restore' the button as indicated > > > that the patch provides. > > > There is NO button in the 'Customize' panel - and I've tried all three > > > themes, still no button. > > > > Does the button appear in customize mode? If you click the downloads button > > in customize mode, no panel appears? If you move it, no panel appears? Do > > you see the pref (browser.download.autohideButton) in about:config ? > > > > > Where is the UI for this feature supposed to appear - > > > > See: > > > > (In reply to Aaron Benson from comment #8) > > > Gijs, here's the spec for the tooltip behavior we discussed: > > > https://mozilla.invisionapp.com/share/54DEJX5TP#/screens/252504394_Customize_ > > The download button does NOT appear in Customize mode as I stated above I assumed you meant you didn't see UI / a button to toggle the downloads button... The downloads button would appear in the default location (the nav bar) in customize mode, where it is hidden outside customize mode (if autohide is on and you have no downloads). Dragging it or clicking it should pop up a panel that lets you toggle autohide. It's also supposed to toggle off autohide as soon as you move the button. It looks like the checkbox state is updated too early so I'll file a bug on that, but the underlying behaviour seems to work in a quick test I ran against current m-c... > I do not see browser.download.autohideButton in about:config - I added the > pref, restarted browser and still does not appear The pref is in the codebase, so you shouldn't need to add it at all. So if it doesn't show up in about:config either you're running an older build or something has removed it from the default pref branch. I don't think we have any code that would do that, though I obviously can't tell you what (non-webext) add-ons would (not) do.
I had previously set browser.download.panel.shown to 'true' , had to do a reset to 'false' (default) and toggling the downloads autohide pref will show/hide the downloads button as it should. I was expecting it also to appear on the right-hand side of the navbar with other buttons but it was something placed on un-hide between the url & search bars. Wasn't seeing it Duh!, moved it over to where other buttons are and all seems well. Thanks again, sorry for the noise. Thanks for the follow-up bug on the possible 'race condition'.
Depends on: 1399486
I guess you won't believe it but browser.download.autohideButton is missing in my regular Nightly profile and in two new Nightly profiles that I've just created using Profile Manager (launch Nightly with -p, Create profile - Enter profile name - Confirm - Launch - go to about:config - pref is missing). Nightly 13-09-17. As the result, download button is not shown/hidden automatically and the UI option to automatically show/hide download button is missing no matter what I do (tried to drag button back to the pallet in Customize mode and drag it, left-click/right-click on it, drag it to the nav bar again, right click on it there, etc). There are two errors in the browser console: https://i.imgur.com/KtiOHmV.png
Depends on: 1399490
(In reply to ajfhajf from comment #32) > I guess you won't believe it but browser.download.autohideButton is missing > in my regular Nightly profile and in two new Nightly profiles that I've just > created using Profile Manager (launch Nightly with -p, Create profile - > Enter profile name - Confirm - Launch - go to about:config - pref is > missing). Nightly 13-09-17. > > As the result, download button is not shown/hidden automatically and the UI > option to automatically show/hide download button is missing no matter what > I do (tried to drag button back to the pallet in Customize mode and drag it, > left-click/right-click on it, drag it to the nav bar again, right click on > it there, etc). > > There are two errors in the browser console: https://i.imgur.com/KtiOHmV.png This bug won't have made today's morning nightly (see comment #25, which was only 3 hours ago), so all of this is expected. Jim was testing an hourly, ie a non-nightly build directly off treeherder for mozilla-central.
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #31) > I was expecting it also to appear on the right-hand side of the navbar with > other buttons but it was something placed on un-hide between the url & > search bars. Wasn't seeing it Duh!, moved it over to where other buttons > are and all seems well. Out of curiosity, can you tell me the output of running this in the browser console: CustomizableUI.getWidgetIdsInArea('nav-bar') ? Because the button *should* be moved to be after the search bar by this build - unless you have (or had!) other things between the url bar and search bar.
Flags: needinfo?(jmjeffery)
Not real familiar but when I input that line I get: ReferenceError: CustomizableUI is not defined
Flags: needinfo?(jmjeffery)
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #35) > Not real familiar but when I input that line I get: > > ReferenceError: CustomizableUI is not defined This sounds like you ran it in the web console. This is the one I mean: https://developer.mozilla.org/en-US/docs/Tools/Browser_Console .
sorry did not have the command-bar enabled in devtools now I get: POST https://shavar.services.mozilla.com/downloads [HTTP/1.1 200 OK 574ms] CustomizableUI.getWidgetIdsInArea('nav-bar') Array [ "back-button", "forward-button", "stop-reload-button", "home-button", "customizableui-special-spring6", "urlbar-container", "flashblockToggle-button", "search-container", "customizableui-special-spring7", "downloads-button", 3 more… ]
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #37) > sorry did not have the command-bar enabled in devtools now I get: > > POST > https://shavar.services.mozilla.com/downloads > [HTTP/1.1 200 OK 574ms] > CustomizableUI.getWidgetIdsInArea('nav-bar') > Array [ "back-button", "forward-button", "stop-reload-button", > "home-button", "customizableui-special-spring6", "urlbar-container", > "flashblockToggle-button", "search-container", > "customizableui-special-spring7", "downloads-button", 3 more… ] Ah, right, so the reason the button didn't appear where expected was that there is / used to be a "flashblockToggle-button" between your url bar and search bar. The code that sets the initial position unfortunately can't know whether that button still exists or not. Anyway, sounds like things were (more or less) working for you, in the end. :-)
Thanks for your time - Yes I used to use Flashblock but its long-since Legacy/unsupported and I removed the addon. Unfortunately - as we all know well - removing an addon does not do a good job, if any in 'cleaning up' left-overs. Thanks again
Quick question - If I delete all the entries in pref: browser.uiCustomization.state which shows a now dead Flasblock and a few other obsolete addons that I no longer use will the browser 'Recreate and populate' the field again on restart ? or... should I just manually 'remove' them from the string ?
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #40) > Quick question - If I delete all the entries in pref: > browser.uiCustomization.state which shows a now dead Flasblock and a few > other obsolete addons that I no longer use will the browser 'Recreate and > populate' the field again on restart ? It would potentially use the currentset information that's stored in the legacy localstore database, so it's not a complete guarantee. > or... should I just manually 'remove' them from the string ? I would just use 'restore defaults' in customize mode instead, and then make any further customizations you think you need.
Thanks again - I will proceed with caution, I will let you get back to real-work, this seems to be OK now for me.
Depends on: 1399873
Depends on: 1399935
This brought some noticeable improvements: == Change summary for alert #9435 (as of September 12 2017 17:52 UTC) == Improvements: 3% Images summary linux64 opt 5,953,026.00 -> 5,758,102.77 2% Images summary osx-10-10 opt 6,191,625.33 -> 6,068,344.51 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9435 They can be inspected better from here: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=25116b9d05415547e2424c0ce2c75cca7e8ff945&newProject=autoland&newRevision=f2e2ecd961cf5e242f301327308234d562af6ed3&originalSignature=1652cbcf255142dfdb93f74b92fe72486f8988cc&newSignature=1652cbcf255142dfdb93f74b92fe72486f8988cc&framework=4
Oh, I forgot. I had moved ⇩ in my preferred page layout using 三 customise (I'm thankful I still can; I'd like to vary other icons which are for now fixed.) When I reinstated the arrow using toggle config, it replaced the arrow instaed in the default position. No big deal, I moved it again. Subsequent toggle of the config did however replace it properly, so not so bad (just an initial implementation problem).
Depends on: 1407596
I have verified that the downloads button automatically hides/shows and has an option to have it permanently visible using Firefox 57.0b7 (BuildId:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
@ Emil Ghitta, QA [:emilghitta] re Comment 48 Perhaps clarify that autohide is not an 'Option' (I went to Options & searched for "downloads": no autohide). Not that I suggest this as solution. Instead the election to auto-hide is cunningly written into the customise facility. However, with the default auto-hide, the download arrow appears in place (even if it were missing) when using customise. Which could be puzzling. Mouseover brings up caption [Downloads] (new user is still puzzled). Only when I actually click on the download-arrow does it offer the autohide. (Same if it is moved to the body of hidden features.) This rather carries the implication that I should already know it was there, or find it accidentally through experimentation, which I don't believe is good design policy; I don't think firefox should mystify users who are not thoroughly familiar with these things in the way developers are. Of course documentation should provide some help to the mystified, if it is clear & easily acessible. * Perhaps a compromise would be for the mouseover to read "click for download-arrow autohide option", or some-such. Note: The 'hidden' config option is browser.download.autohideButton;false which is not all that easy to find. Filter 'download' returns too much; 'arrow' returns nothing.
(In reply to Bill his name from comment #49) > @ Emil Ghitta, QA [:emilghitta] re Comment 48 > > Perhaps clarify that autohide is not an 'Option' (I went to Options & > searched for "downloads": no autohide). QA verifying bugs is not a helpful time to bring up suggestions like this. Emil isn't responsible for the current behaviour or changes to it, they're just checking that the current design works and doesn't have functional bugs vs. what was specified (not whether the design is 'ideal' in whatever way you would define that). In other words, this not behaving how you would like isn't their "fault", nor are they in a position to change it. I believe what you're after is bug 1399873.
(In reply to Comment 50) Thanks, :Gijs My main point (as a former software QA), was to say that autohide is not an "Option", even though it is a rather cunningly designed feature of Customise. A customisation. I would check (and probably criticise) the design spec if I knew where to find it, and if there was an opportunity to do so! (?) I did raise similar points earlier (comment 45). Someone found my comments offensive, hence redacted. I've yet to find out exactly why, since moderation has not replied to me. I am indeed after bug 1399873, thank you. I'll get better at this, eventually.
Depends on: 1410445
Depends on: 1452970
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: