1384692 - Download door hanger navigation should be updated to Photon style

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, enhancement, P2)

Description

[Amy Lee [:amylee] UX]

Hi, 

The current download door-hanger sub menu navigation should be updated to the Photon style sub menu for 57.

I've attached a spec for reference. Please let me know if you have any questions.

Spec: https://mozilla.invisionapp.com/d/main#/console/11636806/245689012/preview

There is currently work being done on updating sub menu transitions in Bug 1374749

Comment 1

[Amy Lee [:amylee] UX]

Attachment: Current Download Doorhanger.png

Comment 2

[:Paolo Amadini]

Amy, can you provide a public link to the spec?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

I can probably take this one, and have access to the spec on InVision…

Comment 4

[Amy Lee [:amylee] UX]

(In reply to :Paolo Amadini from comment #2)
> Amy, can you provide a public link to the spec?

Here you go https://mozilla.invisionapp.com/share/VXCUG7QU6#/245689012_Downloads_Door_Hanger_Spec

Comment 5

[:Paolo Amadini]

Thank you both! I can review the conversion to the <photonpanelmultiview> element when a patch is ready.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 8

[:Paolo Amadini]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

https://reviewboard.mozilla.org/r/163956/#review169778

This still needs localization, so I guess this is an initial feedback request.

I suggest you require ui-review on a screenshot of the result rather than the patch itself.

::: browser/components/downloads/content/downloadsOverlay.xul:110
(Diff revision 2)
>                    label="&cmd.clearList2.label;"
>                    accesskey="&cmd.clearList2.accesskey;"/>
>        </menupopup>
>  
> -      <panelmultiview id="downloadsPanel-multiView"
> +      <photonpanelmultiview id="downloadsPanel-multiView"
>                        mainViewId="downloadsPanel-mainView">

Keep the other line aligned, thanks.

::: browser/components/downloads/content/downloadsOverlay.xul:162
(Diff revision 2)
> +          <hbox id="downloadsPanel-blockedSubview-header"
> +                class="panel-header">
> +            <toolbarbutton class="subviewbutton subviewbutton-iconic subviewbutton-back"
> +                           tabindex="0" tooltip="Back"
> +                           onclick="document.getBindingParent(this).goBack()"/>
> +            <label id="downloadsPanel-blockedSubview-headerText"
> +            >Download Details</label>
> +          </hbox>

I don't see these elements defined at:

https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/content/panelUI.inc.xul#779-793

Maybe the "title" attribute is enough to have them shown?

::: browser/themes/shared/downloads/downloads.inc.css:350
(Diff revision 2)
> +#downloadsPanel-blockedSubview-header {
> +  border-bottom: 1px solid var(--panel-separator-color);
> +  display: flex;
> +  height: 40px;
> +  align-items: center;
> +}
> +
> +#downloadsPanel-blockedSubview-backButton {
> +  min-width: 32px;
> +  min-height: 32px;
> +  margin: 5px 0;
> +  padding: 0;
> +}
> +
> +#downloadsPanel-blockedSubview-headerText {
> +  flex: 1;
> +  text-align: center;
> +}
> +

And hopefully we won't need to duplicate the styles.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

https://reviewboard.mozilla.org/r/163956/#review169778

Amy and I are in the same office, so I'm getting ui-reviews by walking over to her desk with the live build.  She gave it the thumbs up in person, but didn't update the bug yet… :)

> Keep the other line aligned, thanks.

Fixed!

> I don't see these elements defined at:
> 
> https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/content/panelUI.inc.xul#779-793
> 
> Maybe the "title" attribute is enough to have them shown?

Oh!  Nice!  That's way better…

> And hopefully we won't need to duplicate the styles.

And removed!

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 11

[:Paolo Amadini]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

https://reviewboard.mozilla.org/r/163956/#review170322

I tried the patch and it has some bugs that may be related to the interaction with the photonpanelmultiview element.

One bug is that the height jumps if you open subviews of different heights. You can try this by downloading an uncommon and a malware download and opening the two subviews. This should probably be addressed in a separate bug because it's likely an issue with the panelmultiview code.

Another bug is that the keyboard actions are broken after you go back to the main view at least once. This may be due to the special keyboard handling that was added for the menus, that we don't need here except for the left arrow going back to the main view. This may need some investigation.

On Mac, the subview background is also completely transparent.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:137
(Diff revision 3)
>       the panel at all.
>       -->
>  <!ENTITY downloadsHistory.label           "Show All Downloads">
>  <!ENTITY downloadsHistory.accesskey       "S">
>  
> +<!ENTITY downloadDetails.label           "Download Details">

nit: one more space to align

Please add a localization note to explain where this string is displayed.

::: browser/themes/shared/downloads/downloads.inc.css:338
(Diff revision 3)
>  }
>  
>  #downloadsPanel-blockedSubview {
>    background-image: url("chrome://browser/skin/warning.svg");
>    background-size: 32px 32px;
> -  background-position: 16px 16px;
> +  background-position: 16px 56px;

We should move the background to the element right under the header, so we don't need to change this position if the height of the header changes.

Comment 12

[:Paolo Amadini]

Ah, there is probably additional CSS to invert the direction of the arrow on blocked downloads that can now be removed.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 14

[:Paolo Amadini]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

https://reviewboard.mozilla.org/r/163956/#review173530

Most of the issues from comment 11 and 12 are still unaddressed, feel free to ask questions on the bug if anything is unclear.

- The height jump is likely a panel issue, please file a bug that blocks this one, although it is not a strict blocker.
- The keyboard issue is a blocker, and I forgot to mention that mouse clicks on blocked items are also broken. Going back to the main view should keep the same behavior as before the subview is opened.
- You should remove the unneeded styles, and also the icons but only if they're now unused.
- You should add the localization note.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

(In reply to :Paolo Amadini from comment #14)
> Most of the issues from comment 11 and 12 are still unaddressed, feel free
> to ask questions on the bug if anything is unclear.

Yeah, my apologies.  I forget why I posted that previous patch.

> - The height jump is likely a panel issue, please file a bug that blocks
> this one, although it is not a strict blocker.

Filed as bug 1392340.

> - The keyboard issue is a blocker, and I forgot to mention that mouse clicks
> on blocked items are also broken. Going back to the main view should keep
> the same behavior as before the subview is opened.

Mouse clicks work, keyboard nav is filed as bug 1392332. (With a patch, even! ;)

> - You should remove the unneeded styles, and also the icons but only if
> they're now unused.

Fixed.

> - You should add the localization note.

Added.

New MozReview request incoming…

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 17

[:Paolo Amadini]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

https://reviewboard.mozilla.org/r/163956/#review177372

With the dependent bug fixed, navigation now works just as well as the current version.

Thanks a lot for converting one more panel to Photon!

::: browser/themes/shared/downloads/downloads.inc.css:273
(Diff revision 5)
>  @item@[showingsubview] {
>    background-color: Highlight;
>    color: HighlightText;
>    transition: background-color var(--panelui-subview-transition-duration),
>                color var(--panelui-subview-transition-duration);
>  }

This can be removed, there is no need to invert the background color of potentially unwanted and uncommon downloads anymore.

::: browser/themes/shared/downloads/downloads.inc.css:315
(Diff revision 5)
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype=main] > .panel-subviews {
>    /* When the main view is showing, the shadow on the left edge of the subview is
>       barely visible on the right edge of the main view, so set it to none. */
>    box-shadow: none;
>  }
>  
>  /* When the subview is showing, turn the download button into an arrow pointing
>     back to the main view. */
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state[showingsubview] .downloadButton {
>    color: HighlightText;
>  }
>  
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state[showingsubview] .downloadButton > .button-box > .button-icon {
>    list-style-image: url("chrome://browser/skin/panel-icon-arrow-left.svg");
>  }
>  
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state[showingsubview] .downloadButton > .button-box > .button-icon:-moz-locale-dir(rtl) {
>    list-style-image: url("chrome://browser/skin/panel-icon-arrow-right.svg");
>  }

These can be removed as well.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

(In reply to :Paolo Amadini from comment #17)
> ::: browser/themes/shared/downloads/downloads.inc.css:273
> (Diff revision 5)
> >  @item@[showingsubview] {
> >    background-color: Highlight;
> >    color: HighlightText;
> >    transition: background-color var(--panelui-subview-transition-duration),
> >                color var(--panelui-subview-transition-duration);
> >  }
> 
> This can be removed, there is no need to invert the background color of
> potentially unwanted and uncommon downloads anymore.

Good call.  I also removed the `@item@[verdict="Malware"][showingsubview]` below that:
 @item@[verdict="Malware"]:hover,
-@item@[verdict="Malware"]:hover:active,
-@item@[verdict="Malware"][showingsubview] {
+@item@[verdict="Malware"]:hover:active {
   background-color: #aa1b08;
   color: white;
 }

(I'll wait for your thumb's up before I push that)

> ::: browser/themes/shared/downloads/downloads.inc.css:315
> These can be removed as well.

Removed.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 20

[:Paolo Amadini]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #18)
> Good call.  I also removed the `@item@[verdict="Malware"][showingsubview]`
> below that:

I was concerned that not showing the highlight between the click and the time the subview slides in wouldn't look that good, but it doesn't look bad after all, so the removal sounds good.

Can we get rid of the code that sets the "showingsubview" attribute on the item now? If so, worth updating the patch with this removal as well.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

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

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8892948 [details]
Bug 1384692 - Update the download doorhanger style. ui-r=amylee, .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163956/diff/7-8/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f832bd881f8
Update the download doorhanger style. ui-r=amylee, r=Paolo.

Comment 24

[Blake Winton (:bwinton) (:☕️)]

(Discussed over irc, so clearing the needinfo…)

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1f832bd881f8

Comment 26

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the download Panel has been updated. I verified using Fx 57.0b9, on Windows 10 x64, Ubuntu 14.04 LTS and mac OS X 10.12.6.