Closed Bug 1371765 Opened 7 years ago Closed 7 years ago

Only show the downloads button when necessary

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jaws, Assigned: Gijs)

References

(Depends on 2 open bugs)

Details

Attachments

(4 files)

Bug 1371765 - make downloads button non-customizable,
59 bytes, text/x-review-board-request
jaws
: review+
Paolo
: review+
Details
Bug 1371765 - remove unused downloads icons, tidy up CSS,
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Is this also part of this?
6.05 KB, image/jpeg
Details
example case of disabled context menu
295 bytes, text/html
Details 
As part of the downloads animation discussion with UX, we have learned that UX would like to change how the download button appears/disappears.

At startup of Firefox, the downloads button should be hidden.

When a download is started, the downloads button should appear.
- If a flexible spacer is in the toolbar and there is room available, the downloads button should appear and take space from the flexible spacer.
- If a flexible spacer is not in the toolbar, or if there is one but there is no room available, the downloads button should appear and take space from the location bar.
-- The location bar will need to have a min-width. Through minor testing I did not see a min-width already applied to the location bar.
-- The min-width should vary based on the window size, though exact specifics on what the min-width should be have not been specified.
-- We should have a min-width without the downloads button, and another min-width with the downloads button, for the various window sizes.
-- We may decide to support a few window sizes, hopefully this can all be implemented through media queries.
-- The appearance and disappearance of the downloads button will not have any animation associated with it, besides from the "download started" animation.
- The downloads button should appear in the first position to the right of the location bar, unless a flexible spacer or the search box is immediately to the right of the location bar. If a flexible spacer or the search box is immediately to the right of the location bar, then the downloads button should appear to the right of the flexible spacer or the combination of the search box and flexible spacer.

The downloads button should remain visible for the entirety of the session unless the user clears the preview panel, at which point the button should be removed when the panel closes.

Previous customizations as to the placement of the download button should be forgotten. The location of this button will no longer be customizable and it should not overflow.

(Flagging Gijs for needinfo to make sure this gets on the structure team's radar)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> As part of the downloads animation discussion with UX, we have learned that
> UX would like to change how the download button appears/disappears.
> 
> At startup of Firefox, the downloads button should be hidden.
> 
> When a download is started, the downloads button should appear.
> - If a flexible spacer is in the toolbar and there is room available, the
> downloads button should appear and take space from the flexible spacer.

I don't believe CSS has a way of expressing "this item should have flex X, but then subtract Y pixels from its width", so I don't understand what this would mean in practice. Do you know?

On the whole, I thought UX was rethinking these animations to avoid this scenario, and I am confused/sad that this has changed and now there is suddenly extra work to be done here.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jaws)
Depends on: 1369896
I probably could have left off the part about the flexible spacers. I included that because it helped me think about how it would work but I don't think any extra code will need to be written to account for the sizing of them.

As long as we have one media for when the downloads button is hidden and another for when it is visible, I think the flexible spaces and overflow code should handle this just fine.
Flags: needinfo?(jaws)
s/media/media query/
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> I probably could have left off the part about the flexible spacers. I
> included that because it helped me think about how it would work but I don't
> think any extra code will need to be written to account for the sizing of
> them.
> 
> As long as we have one media for when the downloads button is hidden and
> another for when it is visible, I think the flexible spaces and overflow
> code should handle this just fine.

I don't really understand - both the flexible spaces and the url bar / search box will have flex. In fact, the url bar and search box might have fixed widths because of the resizer, too (which is a separate problem). If an extra item appears, the decrease in space will reduce the size of the flexed items proportionally. That's not what comment #0 says should happen, but that's how I think flex works... what am I missing?
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> As part of the downloads animation discussion with UX, we have learned that
> UX would like to change how the download button appears/disappears.
> 
> At startup of Firefox, the downloads button should be hidden.

Note that this is what we were doing initially with the downloads button, and after a couple versions we (along with UX) decided to go back and remove all those magic code paths from the toolbar management, both for having a far more maintainable code and so that downloads were immediately available to the user (there has been some discussion around having a more direct link to the user's download folder, that recently was disabled because incomplete work). I'm not sure why we keep going in cycles through the same ideas we already discarded in the past. Maybe the presence of the Library button invalidates the need for a way to easily reach downloads?
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]
(In reply to :Gijs from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > I probably could have left off the part about the flexible spacers. I
> > included that because it helped me think about how it would work but I don't
> > think any extra code will need to be written to account for the sizing of
> > them.
> > 
> > As long as we have one media for when the downloads button is hidden and
> > another for when it is visible, I think the flexible spaces and overflow
> > code should handle this just fine.
> 
> I don't really understand - both the flexible spaces and the url bar /
> search box will have flex. In fact, the url bar and search box might have
> fixed widths because of the resizer, too (which is a separate problem). If
> an extra item appears, the decrease in space will reduce the size of the
> flexed items proportionally. That's not what comment #0 says should happen,
> but that's how I think flex works... what am I missing?

Using the Browser Toolbox, I was able to set a min-width on #urlbar-container, and it appears to work fine while also having flex="400".

Indeed the appearance of an item will make the flexed items smaller. Comment 0 is talking about adding a minimum width to #urlbar-container, and reducing that minimum width when the downloads button should be shown so as to make more room for the button in this edge case (to guarantee that this specific button won't overflow). 

(In reply to Marco Bonardo [::mak] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > As part of the downloads animation discussion with UX, we have learned that
> > UX would like to change how the download button appears/disappears.
> > 
> > At startup of Firefox, the downloads button should be hidden.
> 
> Note that this is what we were doing initially with the downloads button,
> and after a couple versions we (along with UX) decided to go back and remove
> all those magic code paths from the toolbar management, both for having a
> far more maintainable code and so that downloads were immediately available
> to the user (there has been some discussion around having a more direct link
> to the user's download folder, that recently was disabled because incomplete
> work). I'm not sure why we keep going in cycles through the same ideas we
> already discarded in the past. Maybe the presence of the Library button
> invalidates the need for a way to easily reach downloads?

I'll forward your question to Bryan Bell, who is leading the UX request to restart these conversations.
Flags: needinfo?(jaws) → needinfo?(bbell)
Showing the Recent Downloads icon only when needed is an important aspect of the Photon design. We're doing this for a few reasons. One, it clears up unnecessary clutter in the toolbar, to help the user stay focused on what's relevant at the moment.  Two, it removes the potential of a user accidentally hiding an important feature via the customize menu.  Three, we're solving the access to Previous Downloads with the Library, so there's no need to leave a vestigial tool around because it has a backdoor to that information.
Flags: needinfo?(bbell)
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Blocks: 1387512
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 57.2 - Aug 29
Blocks: 952422
This causes the downloads button animation to be off to the side. I don't know why. I assume Jared can help clarify what's going on there.
I've also deliberately not tried to create different min-widths per window size. For one, I don't really see the point of doing that, and for another, there was no real specification to speak of, so I've left it as just the 1 (well, now 2) min-width.

It seems these changes break a number of other tests (not in the downloads dir), so I'll likely do another commit fixing various tests, with a trypush, probably followed by more test-fixing. :-\
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review174524

::: browser/base/content/browser.css:10
(Diff revision 1)
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  @namespace html url("http://www.w3.org/1999/xhtml");
>  @namespace svg url("http://www.w3.org/2000/svg");
>  
>  :root {
> +  --toolbarbutton-inner-padding: 6px;

Please leave this variable in toolbarbuttons.inc.css where it belongs. This file should still be able to use the variable regardless.

::: browser/base/content/browser.css:510
(Diff revision 1)
> + * appears. This is an approximation that errs on the side of caution to
> + * avoid items ending up in overflow on narrow windows once the
> + * downloads button appears (e.g. due to rounding on hidpi machines).
> + */
>  #urlbar-container {
> +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));

How did you arrive at 24px? My first guess would have been 20px (16px icon size plus 2*2px of extra horizontal padding directly on .toolbarbutton-1).

::: browser/base/content/browser.css:513
(Diff revision 1)
> + */
>  #urlbar-container {
> +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));
> +}
> +
> +#nav-bar[hasdownloads] #urlbar-container {

nit: please use the child selector
(In reply to Dão Gottwald [::dao] from comment #11)
> Comment on attachment 8897888 [details]
> Bug 1371765 - make downloads button non-customizable,
> 
> https://reviewboard.mozilla.org/r/169170/#review174524
> 
> ::: browser/base/content/browser.css:10
> (Diff revision 1)
> >  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> >  @namespace svg url("http://www.w3.org/2000/svg");
> >  
> >  :root {
> > +  --toolbarbutton-inner-padding: 6px;
> 
> Please leave this variable in toolbarbuttons.inc.css where it belongs. This
> file should still be able to use the variable regardless.

Isn't it still verboten to rely on themes/ stuff from content/ ? :-\

> ::: browser/base/content/browser.css:510
> (Diff revision 1)
> > + * appears. This is an approximation that errs on the side of caution to
> > + * avoid items ending up in overflow on narrow windows once the
> > + * downloads button appears (e.g. due to rounding on hidpi machines).
> > + */
> >  #urlbar-container {
> > +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));
> 
> How did you arrive at 24px? My first guess would have been 20px (16px icon
> size plus 2*2px of extra horizontal padding directly on .toolbarbutton-1).

As the comment says, it's deliberately slightly higher. But yes, I think the minimal size is usually 20px.

> ::: browser/base/content/browser.css:513
> (Diff revision 1)
> > + */
> >  #urlbar-container {
> > +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));
> > +}
> > +
> > +#nav-bar[hasdownloads] #urlbar-container {
> 
> nit: please use the child selector

But it's not a child. It's a child of the customization target, and if we customize it'll stop being a child of that too. So you end up with something like:

#nav-bar[hasdownloads] > #nav-bar-customization-target > #urlbar-container,
#nav-bar[hasdownloads] > #nav-bar-customization-target > toolbarpaletteitem > #urlbar-container {
 ...
}

which seems much too complicated to justify a perf penalty that I've never seen any real evidence of. Thoughts?
Flags: needinfo?(dao+bmo)
I haven't looked at the patch yet, but I've tested it and there are a few things missing. The button:
 - Should be placed to the right of the flexible spacer, close to the other icons
 - Should appear to use the area inside the spacer if it's wide enough (i.e. the location bar shouldn't move)
 - Should disappear when the Downloads Panel becomes empty again
(In reply to :Paolo Amadini from comment #13)
> I haven't looked at the patch yet, but I've tested it and there are a few
> things missing. The button:
>  - Should be placed to the right of the flexible spacer, close to the other
> icons

WFM. Clean profile? Steps? I can't fix this if I can't reproduce it.

>  - Should appear to use the area inside the spacer if it's wide enough (i.e.
> the location bar shouldn't move)

That's not what it means, see the other comments in this bug.

>  - Should disappear when the Downloads Panel becomes empty again

That wasn't in comment #0, where does this requirement come from?
Flags: needinfo?(paolo.mozmail)
(In reply to :Gijs from comment #14)
> (In reply to :Paolo Amadini from comment #13)
> >  - Should disappear when the Downloads Panel becomes empty again
> 
> That wasn't in comment #0, where does this requirement come from?

From comment 0,

The downloads button should remain visible for the entirety of the session unless the user clears the preview panel, at which point the button should be removed when the panel closes.
Flags: needinfo?(paolo.mozmail)
(putting needinfo back for the other questions from comment 14)
Flags: needinfo?(paolo.mozmail)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> (In reply to :Gijs from comment #14)
> > (In reply to :Paolo Amadini from comment #13)
> > >  - Should disappear when the Downloads Panel becomes empty again
> > 
> > That wasn't in comment #0, where does this requirement come from?
> 
> From comment 0,
> 
> The downloads button should remain visible for the entirety of the session
> unless the user clears the preview panel, at which point the button should
> be removed when the panel closes.

Is the user clearing the only way the panel becomes empty again? What should happen in private browsing mode? Is there a reasonable place to hook in to this happening that you can recommend?
(In reply to :Gijs from comment #12)
> > Please leave this variable in toolbarbuttons.inc.css where it belongs. This
> > file should still be able to use the variable regardless.
> 
> Isn't it still verboten to rely on themes/ stuff from content/ ? :-\

Not anymore, as we've dropped support for third-party themes.

> > > + * appears. This is an approximation that errs on the side of caution to
> > > + * avoid items ending up in overflow on narrow windows once the
> > > + * downloads button appears (e.g. due to rounding on hidpi machines).
> > > + */
> > >  #urlbar-container {
> > > +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));
> > 
> > How did you arrive at 24px? My first guess would have been 20px (16px icon
> > size plus 2*2px of extra horizontal padding directly on .toolbarbutton-1).
> 
> As the comment says, it's deliberately slightly higher. But yes, I think the
> minimal size is usually 20px.

This should be documented more accurately, e.g. explicitly state that you're adding 4px. "This is an approximation that errs on the side of caution" isn't very helpful when it comes to maintaining this code.

> > > +#nav-bar[hasdownloads] #urlbar-container {
> > 
> > nit: please use the child selector
> 
> But it's not a child.

I knew that, but...

> It's a child of the customization target, and if we
> customize it'll stop being a child of that too. So you end up with something
> like:
> 
> #nav-bar[hasdownloads] > #nav-bar-customization-target > #urlbar-container,
> #nav-bar[hasdownloads] > #nav-bar-customization-target > toolbarpaletteitem
> > #urlbar-container {
>  ...
> }
> 
> which seems much too complicated to justify a perf penalty that I've never
> seen any real evidence of. Thoughts?

Sigh, okay. I forgot again that we wrap the location bar when customizing.
Flags: needinfo?(dao+bmo)
(In reply to :Gijs from comment #14)
> WFM. Clean profile? Steps? I can't fix this if I can't reproduce it.

Weird. New profile on macOS, use "Save Page As" on the start page, the button is on the left of the spacer. Same changeset pulled from MozReview. Maybe I'll have to retry after a clobber build.

> >  - Should appear to use the area inside the spacer if it's wide enough (i.e.
> > the location bar shouldn't move)
> 
> That's not what it means, see the other comments in this bug.

From my discussion with Bryan, I think the intention is to avoid glitches in the position of the location bar in the most common case (window maximized) when the button appears. It's expected that the bar will be slightly off-center once this happens. There may be different technical solutions for achieving this.

The min-width is a separate discussion related to the case where the window is very small, and I agree it's less important.
Flags: needinfo?(paolo.mozmail) → needinfo?(bbell)
(In reply to :Paolo Amadini from comment #19)
> (In reply to :Gijs from comment #14)
> > >  - Should appear to use the area inside the spacer if it's wide enough (i.e.
> > > the location bar shouldn't move)
> > 
> > That's not what it means, see the other comments in this bug.
> 
> From my discussion with Bryan, I think the intention is to avoid glitches in
> the position of the location bar in the most common case (window maximized)
> when the button appears. It's expected that the bar will be slightly
> off-center once this happens. There may be different technical solutions for
> achieving this.
> 
> The min-width is a separate discussion related to the case where the window
> is very small, and I agree it's less important.

Gijs and I had a discussion with Bryan and explained to him how the location bar would be forced to move due to how flexible spacers work and he said he expected this and was OK with it.
(In reply to :Gijs from comment #17)
> Is the user clearing the only way the panel becomes empty again? What should
> happen in private browsing mode? Is there a reasonable place to hook in to
> this happening that you can recommend?

I think the triggering condition is the panel being empty, hence my rephrasing of comment 0. It is currently checked here:

https://dxr.mozilla.org/mozilla-central/rev/7ff4c2f1fe11f6b98686f783294692893b1e1e8b/browser/components/downloads/content/downloads.js#696-717

Private Browsing mode handles the Downloads Panel in the same way, just using a different global list of downloads for the back-end.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Gijs and I had a discussion with Bryan and explained to him how the location
> bar would be forced to move due to how flexible spacers work and he said he
> expected this and was OK with it.

Okay, this is different from what we discussed when we talked through the scenarios, but that discussion happened close to the All Hands so I'm fine with the simpler solution if the design has changed in the meantime.
Flags: needinfo?(bbell)
(In reply to :Paolo Amadini from comment #19)
> (In reply to :Gijs from comment #14)
> > WFM. Clean profile? Steps? I can't fix this if I can't reproduce it.
> 
> Weird. New profile on macOS, use "Save Page As" on the start page, the
> button is on the left of the spacer. Same changeset pulled from MozReview.
> Maybe I'll have to retry after a clobber build.

It seems like the fact that I was using a regular testing profile made a difference, so the downloads button was already positioned after the spacers. I'm not really sure what determined this - the downloads button is now ignored by CUI, so why it puts spacers before it for me and after it in a clean profile, I don't know off-hand, and it doesn't really matter very much. In finding an insertion point, I had to update the code to also ignore the downloads button, and I moved to have it only run all this code once - it seems _ensureOperational can be called repeatedly.

Maybe there's a better place to call DownloadsButton.unhideAndPlace() ? I have to confess that the fact that all the code is split across 5 files didn't make it easier for me to find how things worked together, e.g. I first tried checkIfVisible, which then turned out to be completely unused (so I removed it in my patch).

I considered doing it in the same place you pointed to where I'm now hiding things, but I was worried that that might happen after the code that actually loads the indicator overlay etc. Let me know what you think makes sense.

(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to :Gijs from comment #12)
> > > Please leave this variable in toolbarbuttons.inc.css where it belongs. This
> > > file should still be able to use the variable regardless.
> > 
> > Isn't it still verboten to rely on themes/ stuff from content/ ? :-\
> 
> Not anymore, as we've dropped support for third-party themes.

OK, I moved it back... I was also worried about browser_parsable_css complaining, but it seems to be OK, at least on my local machine. IME the order of files there can't really be relied upon and so whether variables are defined is anyone's guess unless they're in the same file. It does also seem neater to have everything in one file, but maybe we can refactor that post-photon.

> > > > + * appears. This is an approximation that errs on the side of caution to
> > > > + * avoid items ending up in overflow on narrow windows once the
> > > > + * downloads button appears (e.g. due to rounding on hidpi machines).
> > > > + */
> > > >  #urlbar-container {
> > > > +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));
> > > 
> > > How did you arrive at 24px? My first guess would have been 20px (16px icon
> > > size plus 2*2px of extra horizontal padding directly on .toolbarbutton-1).
> > 
> > As the comment says, it's deliberately slightly higher. But yes, I think the
> > minimal size is usually 20px.
> 
> This should be documented more accurately, e.g. explicitly state that you're
> adding 4px. "This is an approximation that errs on the side of caution"
> isn't very helpful when it comes to maintaining this code.

I updated the comment.

(In reply to :Paolo Amadini from comment #21)
> (In reply to :Gijs from comment #17)
> > Is the user clearing the only way the panel becomes empty again? What should
> > happen in private browsing mode? Is there a reasonable place to hook in to
> > this happening that you can recommend?
> 
> I think the triggering condition is the panel being empty, hence my
> rephrasing of comment 0. It is currently checked here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 7ff4c2f1fe11f6b98686f783294692893b1e1e8b/browser/components/downloads/
> content/downloads.js#696-717
> 
> Private Browsing mode handles the Downloads Panel in the same way, just
> using a different global list of downloads for the back-end.

I added code to hide the item again.
(In reply to :Gijs from comment #24)
> (In reply to :Paolo Amadini from comment #19)
> > (In reply to :Gijs from comment #14)
> > > WFM. Clean profile? Steps? I can't fix this if I can't reproduce it.
> > 
> > Weird. New profile on macOS, use "Save Page As" on the start page, the
> > button is on the left of the spacer. Same changeset pulled from MozReview.
> > Maybe I'll have to retry after a clobber build.
> 
> It seems like the fact that I was using a regular testing profile made a
> difference, so the downloads button was already positioned after the
> spacers. I'm not really sure what determined this - the downloads button is
> now ignored by CUI, so why it puts spacers before it for me and after it in
> a clean profile, I don't know off-hand, and it doesn't really matter very
> much. In finding an insertion point, I had to update the code to also ignore
> the downloads button, and I moved to have it only run all this code once -
> it seems _ensureOperational can be called repeatedly.


To be clear, I tried to implement comment #0:

- The downloads button should appear in the first position to the right of the location bar, unless a flexible spacer or the search box is immediately to the right of the location bar. If a flexible spacer or the search box is immediately to the right of the location bar, then the downloads button should appear to the right of the flexible spacer or the combination of the search box and flexible spacer.

So the code iterates through the elements in the location bar, forward from the urlbar, and stops as soon as it finds a non-searchbar/flexspace/searchbar-splitter element. But I had to make it also ignore itself (and reorder when I was hiding it, because it failed the next.hidden check, too).
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review174840

::: browser/base/content/browser.css:510
(Diff revision 2)
> + * plus the toolbarbutton-inner-padding. We're adding 4px to ensure things
> + * like rounding on hidpi don't accidentally result in the button going
> + * into overflow.
> + */
>  #urlbar-container {
> +  min-width: calc(50ch + 24px + 2 * var(--toolbarbutton-inner-padding));

I just realized there are, in fact, other minimum sizes further down in this file. I'll update this patch to move these rules together with the media query'd one, and update all those rules to increase the minimum width per this set of rules -- as soon as my Windows development environment is working again. In the meantime, I think the rest of this patch is still ready for review.
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review174848

::: browser/base/content/browser.css:685
(Diff revisions 2 - 3)
>    pointer-events: none;
>    -moz-user-focus: ignore;
>  }
>  
> +
> +/* We leave 50ch plus whatever space the download button will need when it

self-nit: 49ch now.


So fwiw, I updated all of these to be 1ch smaller than before, but *added* the prospective size of the downloads button (if the downloads button is not displayed). This means the minimum size will now be ~20px (36px on normal density - 1ch) higher without the downloads button, and ~10px smaller with the downloads button. I've also reduced the identity icon label sizes by 1em / 10px to match the potential reduction in space. This should ensure URLs have approximately the same space as before when the button is displayed, and slightly more without it being there, while also ensuring we keep space for the button. If people want we can tweak these some more, though I strongly believe (for phishing/security reasons, basically) that we shouldn't reduce the minimum space for the URL any further.
I've noticed that the panel flickers to a different position before disappearing after clearing downloads. I'll take a closer look tomorrow and see if I have any suggestions.
(In reply to :Paolo Amadini from comment #30)
> I've noticed that the panel flickers to a different position before
> disappearing after clearing downloads. I'll take a closer look tomorrow and
> see if I have any suggestions.

We could probably call hidePopup() on the panel from within the DownloadsButton.hide() method, but maybe there's something better.
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review175296

::: browser/base/content/browser.css:503
(Diff revision 5)
> -#urlbar-container {
> -  min-width: 50ch;
> -}
> +/* url bar min-width is defined further down, together with the maximum size
> + * of the identity icon block, for different window sizes.
> + */

I'll leave the min-widths to Jared for review.

::: browser/base/content/test/general/browser_documentnavigation.js:181
(Diff revision 5)
> +  let download = {
> +    source: {
> +      url: "http://www.example.com/test-download.txt",
> +    },
> +    target: {
> +      path: testTargetFile.path,
> +    },
> +    succeeded: true,
> +    canceled: false,
> +    error: null,
> +    hasPartialData: false,
> +    hasBlockedData: false,
> +    startTime: new Date(),
> +  };

nit: you can use strings directly for "source" and "target", and there is no need to specify default properties.

::: browser/base/content/test/general/browser_documentnavigation.js:195
(Diff revision 5)
> +    error: null,
> +    hasPartialData: false,
> +    hasBlockedData: false,
> +    startTime: new Date(),
> +  };
> +  await downloadsList.add(await Downloads.createDownload(download));

The "add" method only guarantees that the download is in the list, not that all the views have shown it. This means that the downloads-button might still be hidden if we have to load the overlay asynchronously.

::: browser/components/customizableui/CustomizableUI.jsm:425
(Diff revision 5)
> +
> +    if (currentVersion < 11 && gSavedState && gSavedState.placements) {
> +      for (let placements of Object.values(gSavedState.placements)) {
> +        if (placements.includes("downloads-button")) {
> +          placements.splice(placements.indexOf("downloads-button"), 1);
> +          break;
> +        }
> +      }
> +    }

One thing I noticed is that the button then disappears permanently when I use the same profile on a previous version. Not an issue, just wondering if it could be worked around.

::: browser/components/downloads/content/downloads.js:705
(Diff revision 5)
>      if (count > 0) {
>        DownloadsCommon.log("Setting the panel's hasdownloads attribute to true.");
>        DownloadsPanel.panel.setAttribute("hasdownloads", "true");
>      } else {
>        DownloadsCommon.log("Removing the panel's hasdownloads attribute.");
>        DownloadsPanel.panel.removeAttribute("hasdownloads");
> +      DownloadsButton.hide();
>      }

We don't need the "hasdownloads" attribute and the label in the panel anymore.

And yes, we can likely just close the popup in the "hide" method.

::: browser/components/downloads/content/indicator.js:165
(Diff revision 5)
> +          break;
> +        }
> +      }
> +      if (insertionPoint && insertionPoint.nextElementSibling != this._placeholder &&
> +          insertionPoint.nextElementSibling != this._placeholder.nextElementSibling) {
> +        insertionPoint.parentNode.insertBefore(this._placeholder, insertionPoint.nextElementSibling);

nit: insertAdjacentElement might be handy, unless there is some special case that it does not handle here.

::: browser/components/downloads/content/indicator.js:320
(Diff revision 5)
>      let anchor = DownloadsButton._placeholder;
> -    let widgetGroup = CustomizableUI.getWidget("downloads-button");
> +    if (!window.toolbar.visible) {

nit: move the let statement below if we're not using it immediately.

Does this code work for popup windows with a minimal address bar? It seems to work for me.

::: browser/components/downloads/content/indicator.js:381
(Diff revision 5)
>     */
>    set hasDownloads(aValue) {
>      if (this._hasDownloads != aValue || (!this._operational && aValue)) {
>        this._hasDownloads = aValue;
>  
>        // If there is at least one download, ensure that the view elements are

nit: the comment was truncated.

::: browser/components/downloads/content/indicator.js:437
(Diff revision 5)
>      // For arrow-Styled indicator, suppress success attention if we have
>      // progress in toolbar
> -    let suppressAttention = !inMenu &&
> +    let suppressAttention = this._attention == DownloadsCommon.ATTENTION_SUCCESS &&
> -      this._attention == DownloadsCommon.ATTENTION_SUCCESS &&
> -      this._percentComplete >= 0;
> +                            this._percentComplete >= 0;

Suppressing attention when there is progress is actually not required anymore - it can be a separate bug, but it's also quite simple and we can fold it into this patch.

Just remove the _refreshAttention function and set the attribute in the "attention" setter.

::: browser/components/downloads/content/indicator.js:445
(Diff revision 5)
> -      this._percentComplete >= 0;
> +                            this._percentComplete >= 0;
>  
>      if (suppressAttention || this._attention == DownloadsCommon.ATTENTION_NONE) {
>        this.indicator.removeAttribute("attention");
>      } else {
>        this.indicator.setAttribute("attention", this._attention);

There is also CSS we can remove related to badging when the button was moved to the menu panel, look for "cui-areatype".

The "download-glow-menuPanel" image variants are also probably a leftover from previous bugs.
Attachment #8897888 - Flags: review?(paolo.mozmail)
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review175336

::: browser/components/downloads/content/indicator.js:381
(Diff revision 5)
>     */
>    set hasDownloads(aValue) {
>      if (this._hasDownloads != aValue || (!this._operational && aValue)) {
>        this._hasDownloads = aValue;
>  
>        // If there is at least one download, ensure that the view elements are

I didn't touch this comment, but I've completed it for whoever wrote it...
FWIW, the icons are a right rigmarole. There's something strange going on because there's actually a rule to set the icon for the downloads button to the menu panel image (???) irrespective of its location, which doesn't seem to work right for some reason. Removing that, though, it stops overriding the rule in toolbarbuttons.inc.css, which blocks you from seeing the 'attention' state or the progress bar on the icon. So I ended up removing that too. :-\

I'm sure I'm missing something about how this is all supposed to work, but it wasn't really obvious to me *what* I was missing...
(In reply to :Gijs from comment #39)
> FWIW, the icons are a right rigmarole. There's something strange going on
> because there's actually a rule to set the icon for the downloads button to
> the menu panel image (???) irrespective of its location, which doesn't seem
> to work right for some reason. Removing that, though, it stops overriding
> the rule in toolbarbuttons.inc.css, which blocks you from seeing the
> 'attention' state or the progress bar on the icon. So I ended up removing
> that too. :-\
> 
> I'm sure I'm missing something about how this is all supposed to work, but
> it wasn't really obvious to me *what* I was missing...

Ah, never mind, I figured it out - I was removing too much of the cui-areatype-related styling. Fixed now (though, tbh, I don't think anyone will now ever see that icon, so maybe it really should be removed out of toolbarbutton-icons.inc.css )
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review175594

I'm leaving the exact logic in unhideAndPlace and the min-width logic to Jared for detailed review.

::: browser/components/downloads/content/indicator.js:440
(Diff revision 8)
> -      this.indicator.setAttribute("attention", this._attention);
> +        this.indicator.setAttribute("attention", this._attention);
> -    }
> +      }
> +    }
> +    return this._attention;
>    },
> +

nit: no blank line between an internal property and its getter and setter
Attachment #8897888 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8898980 [details]
Bug 1371765 - remove unused downloads icons, tidy up CSS,

https://reviewboard.mozilla.org/r/170318/#review175792

(In reply to :Gijs from comment #42)
> Ah, never mind, I figured it out - I was removing too much of the
> cui-areatype-related styling. Fixed now (though, tbh, I don't think anyone
> will now ever see that icon, so maybe it really should be removed out of
> toolbarbutton-icons.inc.css )

We can file a separate bug to do this and clean up the code some more, if I remember correctly we have more workarounds in place to handle the transition between the neutral state and the state provided by the overlay, which we don't need anymore. We may be able to replace the custom XBL binding with a standard toolbaritem now.
Attachment #8898980 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8897888 [details]
Bug 1371765 - make downloads button non-customizable,

https://reviewboard.mozilla.org/r/169170/#review176014

Looks good, I tried it out locally and it tested good too.
Attachment #8897888 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/97f7f5175b2f
make downloads button non-customizable, r=jaws,Paolo
https://hg.mozilla.org/integration/autoland/rev/f8193cf47975
remove unused downloads icons, tidy up CSS, r=Paolo
https://hg.mozilla.org/mozilla-central/rev/97f7f5175b2f
https://hg.mozilla.org/mozilla-central/rev/f8193cf47975
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1393080
(In reply to bbell from comment #7)
> Showing the Recent Downloads icon only when needed is an important aspect of
> the Photon design. We're doing this for a few reasons. One, it clears up
> unnecessary clutter in the toolbar, to help the user stay focused on what's
> relevant at the moment.  Two, it removes the potential of a user
> accidentally hiding an important feature via the customize menu.  Three,
> we're solving the access to Previous Downloads with the Library, so there's
> no need to leave a vestigial tool around because it has a backdoor to that
> information.

Personally that plain stupid reason and let the user decide, normal users don't even mess with them and you all are basically limiting and shoving personal preference down users throats.
personally like to have my download button present all time which cannot be done any more & if i play around with the UI buttons it appears randomly in different places.

At-least provide an option to always show the button at users discretion .
Depends on: 1393096
Depends on: 1393103
Depends on: 1393136
> Only show the downloads button when necessary

it's always necessary, since the button accepts drag-and-drop.
Depends on: 1393158
See Also: → 1393160
See Also: 1393160 →
See Also: → 1393160
See Also: 1393160 → 
(In reply to Tooru Fujisawa [:arai] from comment #52)
> > Only show the downloads button when necessary
> 
> it's always necessary, since the button accepts drag-and-drop.

This the first use-case someone has brought up, that is certainly not covered in the new design.

Do you use this often? What platform are you on in general? Is it true that even though saving is available in the content menu you prefer this method more?
(In reply to Tooru Fujisawa [:arai] from comment #52)
> > Only show the downloads button when necessary
> 
> it's always necessary, since the button accepts drag-and-drop.

It seems we could restore this functionality by having the menu appear when a drag event is detected. This change may have the added benefit of clarifying to the user that this icon is a drag-to target, which is something I'm confident most users had no idea that was possible.
>Do you use this often? What platform are you on in general? Is it true that even though saving is available in the content menu you prefer this method more?

context menu's can be disabled.
(In reply to Danial Horton from comment #55)
> >Do you use this often? What platform are you on in general? Is it true that even though saving is available in the content menu you prefer this method more?
> 
> context menu's can be disabled.

I didn't realize that. How is that done?
(In reply to bbell from comment #53)
> Do you use this often?

yes.

> What platform are you on in general?

I'm on macOS.

> Is it true that
> even though saving is available in the content menu you prefer this method
> more?

yes.

There are several reasons download button is better than context menu:
  1. the set of context menu items can differ for each case (link, image, etc),
     and it takes me some time to find "Save Link As" or "Save Image As" from those items,
     when I tried to use "Save Link As", I often clicked wrong one that looks similar (like "Copy Link Location"),
     but download button is always there in the same place that I placed (before this bug)
  2. the download button can accept not only link or image, but also selected text with multiple URLs (see bug 92737)

and also the case that context menu gets disabled, as mentioned above.

(In reply to bbell from comment #54)
> It seems we could restore this functionality by having the menu appear when
> a drag event is detected. This change may have the added benefit of
> clarifying to the user that this icon is a drag-to target, which is
> something I'm confident most users had no idea that was possible.

yes, it would be nice to somehow highlight the button while dragging downloadable items,
but even in that case I don't think hiding the button by default makes it better.
Dragging and dropping on to the Downloads button (Bug 1393103) is the only way to easily download some file types (eg SVG images (Bug 1235179)). Although obscure, it is incredibly versatile.
(In reply to Tooru Fujisawa [:arai] from comment #57)

Thank you for your feedback I really appreciate it.
(In reply to Kestrel from comment #58)
> Dragging and dropping on to the Downloads button (Bug 1393103) is the only
> way to easily download some file types (eg SVG images (Bug 1235179)).
> Although obscure, it is incredibly versatile.

I get what you're saying. The current version is certainly esoteric, but the utility is clear. A new solution should maintain the utility but help make it way less obscure.
(In reply to bbell from comment #56)
> (In reply to Danial Horton from comment #55)
> > >Do you use this often? What platform are you on in general? Is it true that even though saving is available in the content menu you prefer this method more?
> > 
> > context menu's can be disabled.
> 
> I didn't realize that. How is that done?

here's the testcase that only download button works.
context menu cannot be opened for the link in the file, but dragging and dropping to download button works.
> here's the testcase that only download button works.

This is helpful. Having a download drag target is a key feature. Just how to achieve that while avoiding the need to clutter the UI with empty menu items in a general use case is something that we need to think about.
having the drop target before starting drag-and-drop action is important.

if the download button is hidden by default and gets visible only while dragging,
user cannot certainly start the action "drag-and-drop this item to the download button there",
since the target is missing, and cannot be sure the action can be performed.
(In reply to Tooru Fujisawa [:arai] from comment #63)
> having the drop target before starting drag-and-drop action is important.
> 
> if the download button is hidden by default and gets visible only while
> dragging,
> user cannot certainly start the action "drag-and-drop this item to the
> download button there",
> since the target is missing, and cannot be sure the action can be performed.

This is a good point. I agree that you need to at the least know the initial drag direction.
I don't find my toolbar cluttered with seven items in it. IMO, having dedicated buttons for downloads, history, and bookmarks is way preferable to a single, multi-tiered Library menu.

At least preserve the individual items for those who want them. This includes the static Downloads button. Feel free to create a separate Awesome Downloads Button to please the UX team.
Depends on: 1393418
Fx did used to have a way to override web page behaviors, but i think that was removed at some point.
A number of use cases mentioned in this bug are about downloading elements of the page that are not obviously downloadable from the main Firefox user interface. Although only a few cases have been mentioned, this can happen in a number of other instances:



* CSS background images:

In this case, drag and drop is simply not possible.

* Embedded SVG images:

I opened the test case in bug 1235179 and I dragged the image to the Downloads button. This did *not* save the image to disk, but it saved the entire page.

* Image elements or links with the context menu disabled:

This is, of course, the individual case for which the drag and drop to the Downloads button is a workaround. Even without the Downloads button, the same workaround is still possible by dragging the element to the tab bar, and then saving the new tab either as a complete page or as an individual file. It's much more complicated, but see below.

* Image elements with the context menu and drag and drop disabled:

Of course, the page can also control drag and drop in the same way it can control the context menu, which would make the workaround above ineffective.



There is, however, another obscure way to save some of the resources mentioned above in Firefox, which is the "Media" tab in the "Page Info" dialog. If we want to support the use cases above in Firefox, we should probably improve or surface this feature rather than continue to support these workarounds.

I would also argue that instead of improving the "Media" tab, we should encourage all these use cases to be handled by browser extensions, which can potentially provide a much better experience to users who need this functionality. This would include the ability to save multiple referenced links or images at the same time.
(In reply to :Paolo Amadini from comment #69)
> I would also argue that instead of improving the "Media" tab, we should
> encourage all these use cases to be handled by browser extensions, which can
> potentially provide a much better experience to users who need this
> functionality. This would include the ability to save multiple referenced
> links or images at the same time.

"Download Star" WebExtension does exactly that: https://addons.mozilla.org/en-US/firefox/addon/download-star/
Depends on: 1393484
I can see this feature implemented on latest nightly 57.0a1 (2017-08-24) in Ubuntu 16.04, 64 bit 

Build ID 	20170824100243
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170823]
I can see this feature implemented on latest nightly latest nightly 57.0a1 (2017-08-24) on windows 10(32bit)

Build ID: 20170824100243
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170823]
(In reply to :Paolo Amadini from comment #69)
> * Image elements or links with the context menu disabled:

I would just add that Firefox has a built-in feature to override a website trying to disable the context menu - just use shift+right click. So e.g. if you go to http://www.pagetutor.com/no_right_click/, and try and right click on the puppy picture, it wouldn't work. But now try shift+right clicking :)
I've backed the main cset out of inbound so this will (assuming it merges well etc.) go back to its previous state while I'm away. I will file a separate bug to implement the mesh between comment #0 and having an option to customize the button as per bug 1393213 comment 29. Separate bug because both this bug and bug 1393213 are now full of confusing/unrelated comments and it will be easier to get this done that way, and also because something else landed in this bug and not all of it was backed out, so reusing it will be hard because mozreview and tracking flags will be confused.
Flags: qe-verify+
QA Contact: gwimberly
Resolution: FIXED → WONTFIX
Backout by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9136548f3de8
Back out changeset 97f7f5175b2f because we're changing the plan for the downloads button, rs=backout,firebot,paolo
Iteration: 57.2 - Aug 29 → ---
Whiteboard: [photon-structure]
Might this bug have anything to do with my download button disappearing  after the last update?
I just downloaded something and had no indicator in the toolbar, the library panel or the main menu. I had to open "Customize" and add the download button back in manually.
My build no: 20170826213134
(In reply to Albert Scheiner [:alberts] from comment #77)
> Might this bug have anything to do with my download button disappearing 
> after the last update?
> I just downloaded something and had no indicator in the toolbar, the library
> panel or the main menu. I had to open "Customize" and add the download
> button back in manually.
> My build no: 20170826213134

Yes, the backout did not restore the Downloads button to the NavBar, I too had to manually retrieve the button from the customize panel.
Looks like the backout brought in some regressions. It kind of makes sense, because when the feature landed on August 22, it landed visible improvements; but these improvements just weren't caught automatically by Perfherder. Unless there's still something strange about this, I consider it a WONTFIX from perf side of view.

== Change summary for alert #9028 (as of August 25 2017 14:55 UTC) ==

Regressions:

  4%  tart summary linux64 opt e10s     5.65 -> 5.87
  2%  tart summary linux64 pgo e10s     4.95 -> 5.07

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9028
If we keep the Downloads button as a customizable element, with an optional auto-hide functionality, the functions that toggle items like the Bookmarks button should take its existence into account, and add the new item _after_ the Downloads button:

https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/browser/base/content/browser-places.js#2091-2115
Depends on: 1397447
No longer blocks: 952422
Depends on: 1413233
Depends on: 1527607
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: