873058 - Download button should do something reasonable when in the menu panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

It should either:

1) Open a subview listing some downloads
2) Open up the Downloads view in the Library

(1) is a bit tricky and likely to require some design work to make it look right.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

My vote is for subview, as always. But we could open the Library for now, and implement the subview in a subsequent release.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Or,

3) Not allow the download button to be moved to the menu panel. It could still be placed in a toolbar or put in the customization palette if the user doesn't want it.

Comment 3

[Johan C]

(In reply to Jared Wein [:jaws] from comment #2)
> Or,
> 
> 3) Not allow the download button to be moved to the menu panel. It could
> still be placed in a toolbar or put in the customization palette if the user
> doesn't want it.

Why would you possibly have this arbitrary limitation?

Not only would this make the download button inconsistent from the history and bookmarks buttons, arguably similar buttons (take the library as an example where you can browse history, downloads and bookmarks).

More importantly it would be confusing for users if some buttons can't be added to the panel.

My vote, if it counts, is for alternative 1, a subview listing the (10-15?) most recent downloads and a button which opens the downloads view in the library.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So here we are opening the Library if the downloads button has been placed within a menu panel area.

fang / shorlander: is this an acceptable solution for the v1 version of Australis (like - we'd be OK shipping this in 25)?

Comment 5

[Zhenshuo Fang (:fang) - Firefox UX Team]

Yes. I think it's good enough for V1.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 760559 [details] [diff] [review]
Patch v1

Review of attachment 760559 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/content/indicator.js
@@ +519,5 @@
>        DownloadsCommon.getIndicatorData(window).attention = false;
>        BrowserDownloadsUI();
>      } else {
> +      // If the downloads button is in the menu panel, open the Library
> +      let widgetGroup = CustomizableUI.getWidget("downloads-button");

off-topic: this getWidget things doesn't look very well named considered it doesn't return a widget, but a group of them...

Comment 7

[Marco Bonardo [:mak]]

A side question, do we disable animations properly when the widget is in the panel? Do we animate regardless when the panel is open?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #7)
> A side question, do we disable animations properly when the widget is in the
> panel? Do we animate regardless when the panel is open?

Good question - I'll test that now.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

So here's how it behaves when in the panel

1) If the panel is closed and a download finishes, we see no animation (unsurprisingly).
2) If the panel is open and a download is in progress, we do see progress in the panel: http://i.imgur.com/XpM6U7w.png . I actually quite like that.
3) If the panel is open and a download completes, it *does* perform the animation.

I think the above is reasonable behaviour.

(In reply to Marco Bonardo [:mak] from comment #6)
> Comment on attachment 760559 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 760559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/content/indicator.js
> @@ +519,5 @@
> >        DownloadsCommon.getIndicatorData(window).attention = false;
> >        BrowserDownloadsUI();
> >      } else {
> > +      // If the downloads button is in the menu panel, open the Library
> > +      let widgetGroup = CustomizableUI.getWidget("downloads-button");
> 
> off-topic: this getWidget things doesn't look very well named considered it
> doesn't return a widget, but a group of them...

I guess our notion of "widget" needs a better explanation. For us, a "widget" is the generalized notion of a piece of UI across every window. This gives us a way of manipulating this piece of UI for every window without having to iterate all of the windows it exists in (for example, disabling the widget).

I think it's kind of a half-realized design, though. There are some things, like the current area types, that we have to get the node for anyhow. :/ Yeah, might need a bit of a re-think there. I'll file a bug about re-thinking the name.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Thanks for the review! Landed in UX as https://hg.mozilla.org/projects/ux/rev/48fa00afe683

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Had to land a follow-up because I used AREATYPE_MENU_PANEL instead of TYPE_MENU_PANEL. Now before anybody says I didn't test my patch before I posted it, I did - but I had applied it on top of 873066 in mq, which introduces AREATYPE_MENU_PANEL.

Anywho, my bad. Follow-up landed as: https://hg.mozilla.org/projects/ux/rev/e1bfdf58328c

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/e1bfdf58328c

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Follow-up landed on mozilla-central as https://hg.mozilla.org/mozilla-central/rev/48fa00afe683