1252509 - Use a sliding panel overlay from the downloads panel to display the available actions

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

We want to add a sliding panel for each blocked download, just like we do in the Control Center for subsections. The UX spec here contains the details:

https://docs.google.com/document/d/1P0vP0UJhkXF-jDjTc_lskprhV_uMuqIx_DcdebG8JCw/edit?usp=sharing

Comment 1

[Drew Willcoxon :adw]

Attachment: MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Really messy WIP.  This focuses on the XUL and CSS -- getting the subview looking right and not messing up the panel as a whole by adding it.  It was trickier than it seems like it should be.  I don't know if I'm doing it wrong but specifying flex and related attributes in the XUL doesn't seem to work with elements in PanelUI.  CSS flexbox works though.

Right now I've only changed the JS enough to let me hackily open the subview when you click a blocked item.

Review commit: https://reviewboard.mozilla.org/r/47897/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47897/

Comment 2

[Drew Willcoxon :adw]

Attachment: downloads-panel-slide.mov

Here's a small screen recording showing it in action.

Comment 3

[Drew Willcoxon :adw]

A status update: I'm still working on this, and it was pretty easy to integrate panelUI to get the sliding panel basically working, but it's been hard to make it ready for prime time.  It's been a pain to make sure the panel's height grows and shrinks properly when you interact with it.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47897/diff/1-2/

Comment 5

[Drew Willcoxon :adw]

Panos, Paolo isn't taking reviews right now.  Maybe you'd like to review or redirect?  Not sure who would be good.

* * *

For the buttons in the subview, especially the "open" button, I tried matching the behavior and color of the history button.  The spec doesn't specify colors and interactions around those buttons.

I haven't tested this on Windows or Linux yet.

This needs a test.  I wanted to start review on the main patch, so I'll post another patch for the test when I'm done with it.

We need images for the right arrow that replaces the small button near the right edge of each richlistitem, including the hover image, which according to the spec is inverted and has a circle around it.

This patch makes it so that clicking the entire item shows the subview.  It's not clear from the spec whether that's the right behavior.

This doesn't change the highlight color to red for malware items.  Maybe we should leave that for a follow-up since it's not directly related to the sliding subview.

The blue highlight color in the spec is much lighter than HighlighColor.  Not sure what color that is other than a hardcoded color.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

I didn't have time to look at the code today, but I played with the patch on Mac and Linux and it looks really great! We discussed these f2f, but my notes for posterity are:
- using a red color for malware downloads for the entire row can be a followup and I think we already have one on file
- having the entire row consume the click is the wanted behavior, but that should include the action icon (currently an X or a magnifying glass, in the new design a right arrow), which at the moment displays a modal prompt (the subpanel performs the same role as that prompt)
- the back arrow button in the subpanel should be displayed on a semi-transparent element that shows the main panel beneath as in the spec
- the spec I think incorrectly displays the left part of the main panel beneath the semi-transparent layer, while it should be showing the right part as it happens in the CC and the menu panel
- idea: it might make sense to make the blue "back" button on the subpanel larger and have it be placed on top of the row item that was clicked, just like we do in the panel menu
- the subpanel buttons have an extra border on Linux
- you need to get the assets from sevaan, unless paolo has them already

Comment 7

[Drew Willcoxon :adw]

I asked Sevaan about the questions we brought up and he said to follow what Firefox does in other cases like the control center instead of following the spec.  So I'll work on that.  He'll also get back to us with images for the arrow glyph action icon.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Clearing review until I update the patch for those visual changes.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47897/diff/2-3/

Comment 10

[Drew Willcoxon :adw]

This commit has these fixes:

* Makes the main view slide to the left as the subview slides in from the right
* When the subview is showing, the right edge of the main view is still visible, and the item that you clicked remains highlighted and its button is replaced with the back arrow
* The button in each richlistitem now has the right arrow image
* Malware downloads are now highlighted in red

I still haven't tested on Windows and Linux, so things could look pretty bad there although I tried to match what I'm doing on OS X.  And this still needs a test.

I just noticed a couple of problems with the new icons that Sevaan gave me.  In some files, the images aren't positioned correctly within the canvas, so images on the edge of the canvas are slightly cut off.  And we need an RTL version of the right arrow icon.

Comment 11

[Drew Willcoxon :adw]

Try builds for anyone who wants to try it:

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-3991d23bb7d6b62b93b23b9c04e3e8e6dbd95479/

Comment 12

[:Paolo Amadini]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

I won't be able to review this soon, maybe Jared can help?

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47897/diff/3-4/

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

This fixes Windows and Linux problems.  I'll leave the r? cleared until I add a test.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47897/diff/4-5/

Comment 16

[Drew Willcoxon :adw]

That's the complete patch with test and fixed icons and everything.

Comment 17

[Drew Willcoxon :adw]

Try builds here:

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-29917350acbca535d90d7a35c98f201f3af5b66b/

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47897/diff/5-6/

Comment 19

[Drew Willcoxon :adw]

Small change to head.js to fix the failing test on try.

Comment 20

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

Comment on attachment 8743593 [details]
MozReview Request: Bug 1252509 - Use a sliding panel overlay from the downloads panel to display the available actions. r?jaws

https://reviewboard.mozilla.org/r/47897/#review54594

r=me with the below questions answered. I'm most curious about the two styling changes at the end of my review notes.

::: browser/components/downloads/DownloadsViewUI.jsm:273
(Diff revision 6)
> +    // Assume Downloads.Error.BLOCK_VERDICT_MALWARE
> +    return [s.blockedMalware, [s.unblockTypeMalware, s.unblockTip2]];

Instead of assuming, can we move this up to the switch statement and then either throw or log an error if we are missing a case?

::: browser/components/downloads/content/downloads.css:181
(Diff revision 6)
> +/* Hide all the usual buttons. */
> +#downloadsPanel-mainView .download-state[state="8"] .downloadCancel,
> +#downloadsPanel-mainView .download-state[state="8"] .downloadConfirmBlock,
> +#downloadsPanel-mainView .download-state[state="8"] .downloadChooseUnblock,
> +#downloadsPanel-mainView .download-state[state="8"] .downloadChooseOpen,
> +#downloadsPanel-mainView .download-state[state="8"] .downloadRetry,
> +#downloadsPanel-mainView .download-state[state="8"] .downloadShow {
> +  visibility: hidden;
> +}

Should these be display:none instead? visibility:hidden will still consume space in the layout, thought it may not be as much of a problem if they are in a stack, it will still cause extra layout work.

::: browser/components/downloads/content/downloads.js:1583
(Diff revision 6)
> +  handleEvent(event) {
> +    this.subview.removeEventListener(event.type, this);
> +
> +    this.element.removeAttribute("showingsubview");
> +    delete this.element;

This is all pretty tightly tied to the ViewHiding event, but this code doesn't make sure that the event is type="ViewHiding". Can you add a check to this code so it only runs if type="ViewHiding" and will throw or log if the type is unexpected? (Just thinking of potential errors down the road)

::: browser/themes/shared/downloads/download-blocked.svg:10
(Diff revision 6)
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">
> +  <style>
> +    circle {
> +      fill: #D92215;
> +    }
> +    

nit, trailing whitespace

::: browser/themes/shared/downloads/downloads.inc.css
(Diff revision 6)
> -#downloadsPanel:not([hasdownloads]) > #downloadsListBox {
> -  display: none;
> -}
> -
> -#downloadsPanel[hasdownloads] > #emptyDownloads {
> -  display: none;
> -}
> -

Why are these rules being removed?

::: browser/themes/shared/downloads/downloads.inc.css:29
(Diff revision 6)
> -  display: none;
> -}
> -
>  #emptyDownloads {
>    padding: 10px 20px;
> -  max-width: 40ch;
> +  text-align: center;

Why is the text being centered now?

Comment 21

[Drew Willcoxon :adw]

Thanks Jared.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> ::: browser/components/downloads/DownloadsViewUI.jsm:273
> (Diff revision 6)
> > +    // Assume Downloads.Error.BLOCK_VERDICT_MALWARE
> > +    return [s.blockedMalware, [s.unblockTypeMalware, s.unblockTip2]];
> 
> Instead of assuming, can we move this up to the switch statement and then
> either throw or log an error if we are missing a case?

Sure.

> ::: browser/components/downloads/content/downloads.css:181
> (Diff revision 6)
> > +/* Hide all the usual buttons. */
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadCancel,
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadConfirmBlock,
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadChooseUnblock,
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadChooseOpen,
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadRetry,
> > +#downloadsPanel-mainView .download-state[state="8"] .downloadShow {
> > +  visibility: hidden;
> > +}
> 
> Should these be display:none instead?

Done.

> 
> ::: browser/components/downloads/content/downloads.js:1583
> (Diff revision 6)
> > +  handleEvent(event) {
> > +    this.subview.removeEventListener(event.type, this);
> > +
> > +    this.element.removeAttribute("showingsubview");
> > +    delete this.element;
> 
> This is all pretty tightly tied to the ViewHiding event, but this code
> doesn't make sure that the event is type="ViewHiding". Can you add a check
> to this code so it only runs if type="ViewHiding" and will throw or log if
> the type is unexpected? (Just thinking of potential errors down the road)

Sure.

> ::: browser/themes/shared/downloads/downloads.inc.css
> (Diff revision 6)
> > -#downloadsPanel:not([hasdownloads]) > #downloadsListBox {
> > -  display: none;
> > -}
> > -
> > -#downloadsPanel[hasdownloads] > #emptyDownloads {
> > -  display: none;
> > -}
> > -
> 
> Why are these rules being removed?

I actually moved them to browser/components/downloads/content/downloads.css.  I think they make more sense being in the content css, not the theme css, but I can move them back if you disagree.  The new rules are:

/*** Downloads panel ***/

#downloadsPanel[hasdownloads] #emptyDownloads,
#downloadsPanel:not([hasdownloads]) #downloadsListBox {
  display: none;
}

I had to make them general descendant selectors since #emptyDownloads and #downloadsListBox are no longer children of the panel due to the new panelUI elements.

> ::: browser/themes/shared/downloads/downloads.inc.css:29
> (Diff revision 6)
> > -  display: none;
> > -}
> > -
> >  #emptyDownloads {
> >    padding: 10px 20px;
> > -  max-width: 40ch;
> > +  text-align: center;
> 
> Why is the text being centered now?

I had to give the panel's mainview a min-width via this new rule in browser/components/downloads/content/downloads.css:

.panel-mainview[panelid=downloadsPanel] {
  /* Make the main view fill the entire width of the panel. */
  min-width: 30em;
}

The reason is that otherwise, when there's a blocked download in the panel and you click it to show its info and then you go back and remove that download (via the context menu for example) so that the panel becomes empty, the panel remains wide as if it's still showing the info subview, but its right portion is blank -- the "no downloads for this session" is scrunched up into the left half of the panel.

Giving the panel mainview a min-width fixes that, but then the text like the empty-downloads text isn't centered within the panel.  So that's the answer to your question. :-)

I tried fixing the root problem of the panel remaining too wide but I just couldn't.  Maybe it's possible though.

Comment 22

[Drew Willcoxon :adw]

I'm still trying to figure out a timeout on Linux ASAN before I land this.

Comment 23

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

(In reply to Drew Willcoxon :adw from comment #21)
> I actually moved them to browser/components/downloads/content/downloads.css.
> I think they make more sense being in the content css, not the theme css,
> but I can move them back if you disagree.

Moving to content CSS is good, I just didn't notice it in the review.

> I tried fixing the root problem of the panel remaining too wide but I just
> couldn't.  Maybe it's possible though.

I've been down that road before and I don't blame you for your choice here. This is fine by me :)

Comment 24

[:Paolo Amadini]

(In reply to Drew Willcoxon :adw from comment #21)
> .panel-mainview[panelid=downloadsPanel] {
>   /* Make the main view fill the entire width of the panel. */
>   min-width: 30em;
> }

Drive-by comment, haven't looked at the patch.

The width of the panel is localized, does the above min-width work for any panel width?

Comment 25

[Drew Willcoxon :adw]

Think I finally fixed the timeout with a terrible hack in the test.  Hopefully final try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4379ecfc1d37

If it's green then I'll land this ASAP.

(In reply to :Paolo Amadini from comment #24)
> The width of the panel is localized, does the above min-width work for any
> panel width?

Ah thanks, I see it.  Your comment prompted me to look at how I was doing that, and I changed it so that this rule is applied in JS instead based on computed style, so it should be right in all localizations.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2719065dd5a6
Use a sliding panel overlay from the downloads panel to display the available actions. r=jaws

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2719065dd5a6

Comment 28

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Could you please specify if the UX spec document https://docs.google.com/document/d/1P0vP0UJhkXF-jDjTc_lskprhV_uMuqIx_DcdebG8JCw/edit?usp=sharing is up to date? It would be very helpful.

Comment 29

[:Paolo Amadini]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #28)
> Could you please specify if the UX spec document
> https://docs.google.com/document/d/1P0vP0UJhkXF-
> jDjTc_lskprhV_uMuqIx_DcdebG8JCw/edit?usp=sharing is up to date? It would be
> very helpful.

Eh, no, I guess it's difficult to keep a specification up to date with all the details that changed over time, and maybe it's not completely necessary. For example, some things like the style of warning badges changed in the meantime, and the final strings are different. So, you can still use this as a general guide but if you notice differences that you think are important, just ask about them.

Comment 30

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I investigated this issue on 50.0b2 build1 (20160926162149) across platforms (see [1], [2] and [3]) using  the UX spec document https://docs.google.com/document/d/1P0vP0UJhkXF-jDjTc_lskprhV_uMuqIx_DcdebG8JCw/edit?usp=sharing and http://testsafebrowsing.appspot.com/.

These are the results: 
I. There are items that don't correspond with the UX spec document:
  a) the green upward arrow is _not_ displayed over the Hamburger Menu button after a download is finished ([1], [2] and [3])
  b) for malware downloads, the extension _is_ appended even before the user confirms he wants to open the file ([1], [2] and [3])
  c) the status badge from the Downloads Panel for the uncommon files is a white exclamation mark in a blue circle (see the screenshot https://drive.google.com/file/d/0B0nYKG6PRiCcNnpqTzc4MHMwWmM/view?usp=sharing) ([1], [2] and [3])
  d) for the malware files the highlight colour is red only after the file is downloaded (and only in the Downloads Panel) ([1], [2] and [3])
  e) _no_ modal pop-up can be open from the Library ([1], [2] and [3])

II. There are some related issues: 
  f) the download progress animation from the Downloads button is different across platforms: always green ([1]), always orange ([2]) and always blue ([3])
  g) when the Downloads button is in the Menu Bar, the badges located over it are cut off (see the screenshot https://drive.google.com/file/d/0B0nYKG6PRiCceDA1ZzZsU21NQlU/view?usp=sharing) ([1])
  h) when the Downloads button is in the Menu Panel, the badges located over it are cut off (see the screenshot https://drive.google.com/file/d/0B0nYKG6PRiCcX05YVFVsODQwczQ/view?usp=sharing) ([1])
  i) when the Downloads button is in the Bookmarks Toolbar, the badges located over it are cut off ([1] and [3])
  j) when the Downloads button is in the Menu Bar or in the Bookmarks Toolbar, the Downloads button loses the proper alignment (see the screenshot https://drive.google.com/file/d/0B0nYKG6PRiCceDdyMnFwZ0lGWGc/view?usp=sharing) ([1])
  k) when the Downloads button is in the Menu Panel, there are displayed _no_ badges (over the Downloads button) when the Menu Panel is open ([2])
  l) height transitions are visible for the Downloads Panel ([1], [2] and [3]) 
 
[1] Windows 10 x64
[2] Ubuntu 14.04 x86 
[3] Mac OS X 10.11

Please confirm which of these are expected and not.

Comment 31

[Drew Willcoxon :adw]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #30)
> I. There are items that don't correspond with the UX spec document:
>   a) the green upward arrow is _not_ displayed over the Hamburger Menu
> button after a download is finished ([1], [2] and [3])
>   b) for malware downloads, the extension _is_ appended even before the user
> confirms he wants to open the file ([1], [2] and [3])
>   c) the status badge from the Downloads Panel for the uncommon files is a
> white exclamation mark in a blue circle (see the screenshot
> https://drive.google.com/file/d/0B0nYKG6PRiCcNnpqTzc4MHMwWmM/
> view?usp=sharing) ([1], [2] and [3])

These aren't related to the download panel contents, so they shouldn't block this bug, but could you please file new bugs for them?  There may already be bugs on file, I don't know.

>   d) for the malware files the highlight colour is red only after the file
> is downloaded (and only in the Downloads Panel) ([1], [2] and [3])

I think the highlight color should only appear when a download list item is focused, like when you tab to it from the keyboard, and that should be the case for all downloads, malware or not.  In other words, highlights should behave the same on all download items, except for malware the color should be red.  And that's what I'm seeing.  If that's not what you're seeing, could you please file a new bug?

>   e) _no_ modal pop-up can be open from the Library ([1], [2] and [3])

How did you try this case?  If I right-click and choose Allow Download, I get a modal prompt.

> II. There are some related issues: 
>   f) the download progress animation from the Downloads button is different
> across platforms: always green ([1]), always orange ([2]) and always blue
> ([3])

I think that's how it's supposed to be, it's just theme differences.

>   g) when the Downloads button is in the Menu Bar, the badges located over
> it are cut off (see the screenshot
> https://drive.google.com/file/d/0B0nYKG6PRiCceDA1ZzZsU21NQlU/
> view?usp=sharing) ([1])
>   h) when the Downloads button is in the Menu Panel, the badges located over
> it are cut off (see the screenshot
> https://drive.google.com/file/d/0B0nYKG6PRiCcX05YVFVsODQwczQ/
> view?usp=sharing) ([1])
>   i) when the Downloads button is in the Bookmarks Toolbar, the badges
> located over it are cut off ([1] and [3])
>   j) when the Downloads button is in the Menu Bar or in the Bookmarks
> Toolbar, the Downloads button loses the proper alignment (see the screenshot
> https://drive.google.com/file/d/0B0nYKG6PRiCceDdyMnFwZ0lGWGc/
> view?usp=sharing) ([1])
>   k) when the Downloads button is in the Menu Panel, there are displayed
> _no_ badges (over the Downloads button) when the Menu Panel is open ([2])

These aren't related to this bug, but if there aren't already bugs for them, could you please file some?

>   l) height transitions are visible for the Downloads Panel ([1], [2] and
> [3])

What do you mean exactly?  The height should transition/animate as the blocked subview slides in and out.

Comment 32

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Drew Willcoxon :adw from comment #31)

> >   l) height transitions are visible for the Downloads Panel ([1], [2] and [3])
> 
> What do you mean exactly?  The height should transition/animate as the
> blocked subview slides in and out.

I mean that after removing a file from the sliding panel, it seems that the Downloads Panel wants to keep the old height and then, it quickly calibrates to the new height (see the screencast https://drive.google.com/file/d/0B0nYKG6PRiCcM2s4bXBCSFR4NFU/view?usp=sharing).

Comment 33

[Drew Willcoxon :adw]

I see, thanks.  That's a known bug with panel height changes that Paolo is addressing in another bug, bug 1009116.

Comment 34

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thanks you, Drew Willcoxon, for the explaining. I will document and file the above mentioned issues. Based on comment 33 and comment 31, this bug is verified fixed on Firefox 50. I will set the corresponding flags.