697688 - [Downloads Panel] New appearance and behavior for the list items

Status: RESOLVED DUPLICATE of bug 726444

(Firefox :: General, defect)

Description

[:Paolo Amadini]

| This bug describes changes to the version of the downloads panel and
| indicator that is currently present in the UX branch, more specifically:
| * Bug 564934, attachment 558500 [details] [diff] [review]
| * Bug 663772, attachment 558504 [details] [diff] [review]
| 
| Patches for this bug should apply on top of the two mentioned patches.

* Ongoing downloads should display the "X" icon, whether they can be paused
   or not, as they currently do. Clicking the icon should either stop or
   pause the download, based on server capabilities, without removing the
   item. At this point, the "circular arrow" icon should appear to allow the
   download to be resumed or restarted. Note that "play" and "pause" icons
   aren't used because it's unclear if they represent the current state or
   an action on the download.
* Add a "Clear list" entry to the item's context menu, which should remove
   completed items from the panel (same effect that closing the last browser
   should have).
* The background highlight on hover should only cover the area with the file,
   not the area with the icon (which should have its own hover). Make sure to
   center the icon padding-wise left/right.
* Investigate why the the icon near the download entry sometimes doesn't
   appear for in-progress downloads, in the latest builds.

Comment 1

[:Paolo Amadini]

(In reply to Paolo Amadini from comment #0)
> * Ongoing downloads should display the "X" icon, whether they can be paused
>    or not, as they currently do. Clicking the icon should either stop or
>    pause the download, based on server capabilities, without removing the
>    item. At this point, the "circular arrow" icon should appear to allow the
>    download to be resumed or restarted. Note that "play" and "pause" icons
>    aren't used because it's unclear if they represent the current state or
>    an action on the download.

The note in bug 697683 comment 1 made me think about this interface proposal.

The solution described above makes the "X" icon behave differently based on a
"hidden" state, not visible in the interface, that depends on the capabilities
of the server that is serving the download.

Based on this hidden state, clicking the "X" can either cancel or pause the
download. The issue here is that, currently, the behavior of canceled and paused
downloads in the back-end is quite different. The former can be restarted,
but are removed from the downloads database when necessary (in our case, when
the last browser window is closed). The latter can be resumed, but remain in the
database indefinitely, even across sessions, unless they are manually removed.

I think we should assume that, by clicking the "X", the user always intends to
do the same action: stop the download.

As a follow-up optimization, we can make sure that the Download Manager back-end
preserves the temporary, partially downloaded files for some time (for example,
until the item is removed from the database). This way, if the server supports
the feature, the user can "undo" the fact that he just canceled the download
without losing the data downloaded so far. This however doesn't make the item
remain in the panel indefinitely, in case the user really just wanted to cancel
the download.

"Explicit" pause can still be available as an advanced (context menu) option.
In this case, pausing means that the user wants to resume the download later
(the next week maybe) and we should not remove the item when the last browser
window is closed or when "Clear list" is selected.

Comment 2

[:Paolo Amadini]

Attachment: Part 1 of 2 - Behavior

Updates behavior of the "X" icon and adds "Clear List" content menu entry.

Comment 3

[:Paolo Amadini]

Attachment: Part 2 of 2 - Appearance

Attempts to make entries fixed height, and to highlight only part of completed
items on hover, excluding the button. There are now two "inactive" pixels
between consecutive completed items, due to the outer border, but the overall
result should work well enough to let UX provide feedback on the UX branch.

Marco, can you take a look and see if my changes make sense for themes other
than Windows XP?

Comment 4

[:Paolo Amadini]

(In reply to Paolo Amadini from comment #0)
> * Investigate why the the icon near the download entry sometimes doesn't
>    appear for in-progress downloads, in the latest builds.

I observed this originally in local builds, but couldn't reproduce it anymore
with my latest local builds.

Comment 5

[:Paolo Amadini]

Tryserver build now running:

https://tbpl.mozilla.org/?tree=Try&rev=7c73d39a3730

Comment 6

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #1)
> I think we should assume that, by clicking the "X", the user always intends
> to
> do the same action: stop the download.

Maybe I'm special, but I more often pause downloads because I need bandwidth rather than stopping them indefinitely. Still, I'm not sure having a "pause" action associated to an X icon makes sense, I really doubt I've ever seen any app doing a similar association. On any native app, X is always associated to "close", "remove" or "delete" actions.
So, in the end I think I agree with you that X should just have just a single functionality, that is the most obvious: Stop. Regarding the fact Pause will just be in the context menu, I think we may have more requests than we expect to expose it in primary ui, but we can wait for that feedback, or even telemetry how many times it is used!

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 575850 [details] [diff] [review]
Part 1 of 2 - Behavior

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

::: browser/components/downloads/content/downloads.js
@@ +820,5 @@
> +    // Handle commands that are not selection-specific.
> +    switch (aCommand) {
> +      case "downloadsCmd_clearList":
> +        return Services.downloads.canCleanUp;
> +    }

don't switch for now, it's a single entry, so just use an if. when we'll have more we will use a switch.

@@ +859,5 @@
> +
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// Commands
> +
> +  commands: {

nit: you may rename this .commands to state in the name these are global commands and don't need a selection. This would self-document any code using them and one should not figure our why we handle two separate lists of .commands. Naming is hard though, globalCommands, genericCommands, anyItemCommands?

For sure you may improve the //// Commands header!

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +25,5 @@
>  <!ENTITY cmd.copyDownloadLink.accesskey   "L">
>  <!ENTITY cmd.removeFromList.label         "Remove From List">
>  <!ENTITY cmd.removeFromList.accesskey     "e">
> +<!ENTITY cmd.clearList.label              "Clear List">
> +<!ENTITY cmd.clearList.accesskey          "a">

is C taken already? this will differ from the DM window, while the other accesskeys are equal

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 575852 [details] [diff] [review]
Part 2 of 2 - Appearance

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

hm, honestly the new splitted hover selection doesn't look really nice, it is missing a border (that you commented out) so it doesn't look native, and it is clashing with the line that separates 2 sibling downloads. The selection of the download above the line touches it, while the one below the line has a 1 pixel space.
Btw, since we are going far from the mockups we had, I wouldn't know how to make it look as good as before, it's likely we need shorlander to directly get a build and fix the style as he likes it, or suggest a new mockup for the split selection.
As a general hint I'd keep the selection border though, as we do in split-menus.

::: browser/components/downloads/content/download.xml
@@ +54,5 @@
>      <resources>
>        <stylesheet src="chrome://browser/skin/downloads/downloads.css"/>
>      </resources>
> +    <content orient="horizontal">
> +      <xul:hbox class="downloadMainArea"

the MainArea name is too generic to even understand what it means.
May be just donwloadInfo while the other part could be downloadActions?

@@ +57,5 @@
> +    <content orient="horizontal">
> +      <xul:hbox class="downloadMainArea"
> +                align="center"
> +                flex="1"
> +                onclick="DownloadsView.onDownloadMainAreaClick(event);">

I'd probably keep the old good onDowloadClick name, since the other "area" doesn't need special handling, this is still the main click handler on a download. Btw if you rename MainArea you should rename this as well.

::: browser/components/downloads/content/downloads.js
@@ +475,2 @@
>    {
>      // Only do the action for primary clicks.

nit: s/do the action for/handle/

Comment 9

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #7)
> > +<!ENTITY cmd.clearList.label              "Clear List">
> > +<!ENTITY cmd.clearList.accesskey          "a">
> 
> is C taken already? this will differ from the DM window, while the other
> accesskeys are equal

By "Cancel", which is "C" in the Download Manager window too. Maybe we should
give priority to C for Cancel, since in the window it's a menu entry while the
other is a button accessed with a different overall key sequence in any case?

(In reply to Marco Bonardo [:mak] from comment #8)
> hm, honestly the new splitted hover selection doesn't look really nice, it
> is missing a border (that you commented out) so it doesn't look native, and
> it is clashing with the line that separates 2 sibling downloads.

Yeah, moreover on Windows XP inverting (rather than highlighting) just part of
the item instead of the whole entry doesn't look right to me for some reason.

> it's likely we need shorlander to directly get a build and fix the style as
> he likes it, or suggest a new mockup for the split selection.

What about landing this "raw" patch (which includes the necessary XUL changes)
so that shorlander can refine styles on the UX branch?

(Yes, this looks like a project branch workflow; should work fine for people
that are likely to have a UX branch checkout, I guess.)

> May be just donwloadInfo while the other part could be downloadActions?

downloadInfo seems fine (though we don't need a class for downloadActions for
now, so that won't appear in the code).

Comment 10

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #9)
> (In reply to Marco Bonardo [:mak] from comment #7)
> > is C taken already? this will differ from the DM window, while the other
> > accesskeys are equal
> 
> By "Cancel", which is "C" in the Download Manager window too. Maybe we should
> give priority to C for Cancel, since in the window it's a menu entry while
> the
> other is a button accessed with a different overall key sequence in any case?

yeah, if the window already uses C for cancel, it's being the same issue I was pointing out, no gain.

> (In reply to Marco Bonardo [:mak] from comment #8)
> > it's likely we need shorlander to directly get a build and fix the style as
> > he likes it, or suggest a new mockup for the split selection.
> 
> What about landing this "raw" patch (which includes the necessary XUL
> changes)
> so that shorlander can refine styles on the UX branch?

ok, but keep the selection borders for now, I honestly didn't try with them, but without borders it looks everything but native.

> > May be just donwloadInfo while the other part could be downloadActions?
> 
> downloadInfo seems fine (though we don't need a class for downloadActions for
> now, so that won't appear in the code).

Yes I know, I was mostly pointing out that the "special" area is the actions one, so I gave it a name :)

Comment 11

[:Paolo Amadini]

Attachment: Unified patch

For simplicity, this single patch includes both the code and the style changes.

Styles are untested on platforms other than Windows XP, I made the same changes
to the CSS files for other platforms, I hope they work! I also hope I got the
Windows Vista "@media" parts right :-)

Comment 12

[:Paolo Amadini]

The patch on this bug is now included as part of the patch in bug 726444. Any
pending issue here should have already been filed as one of the bug's follow-ups.