1244473 - Unify the Downloads theme files across platforms

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[:Paolo Amadini]

We need to unify the Downloads Panel and Downloads View theme files across platforms to make them manageable when we change the item and panel structure for the new design of the interface for unblocking downloads.

This is a CSS code refactoring bug, but marked for QA verification since all theme changes requires manual testing for regressions.

Comment 1

[:Paolo Amadini]

Jared, this might be a significant change to the Downloads theme files, and it's in the list of priorities for our team because we're implementing the new UI for blocked downloads now.

I might have something ready in the second half of next week. Would you be available to set aside some time to review it?

What I'll do, if it works for you, is to have multiple changesets associated with this bug, each moving a few related rules to new downloads.inc.css and allDownloadsViewOverlay.inc.css shared files. This should be easier to review since the current theme files of each platform have become different in structure.

Comment 2

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 1 - Add shared theme files for Downloads. r=jaws

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

Comment 3

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

Yes, I'll be available to review it :)

Comment 4

[:Paolo Amadini]

I might clean up some of the states added for keyboard navigation in bug 819428. Indeed Marco noted in bug 819428 comment 79 that we might have too many of them.

In particular, when the panel is in keyboard focus mode we still have a hover state depending on where the mouse pointer is. We can probably get rid of this state.

Comment 5

[:Paolo Amadini]

Attachment: Hover state in keyboard focus mode

Comment 6

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #4)
> In particular, when the panel is in keyboard focus mode we still have a
> hover state depending on where the mouse pointer is. We can probably get rid
> of this state.

In the end I didn't touch the button rules here, a simplification based on "@" substitutions was enough to make them readable and manageable during the restructuring of the panel items.

We have a proposed panel style redesign where we can get rid of the unneeded states.

Comment 7

[:Paolo Amadini]

Comment on attachment 8714022 [details]
MozReview Request: Bug 1244473 - Part 1 - Add shared theme files for Downloads. r=jaws

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

Comment 8

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 2 - Share simple rules for the panel and the outer controls. r=jaws

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

Comment 9

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 3 - Restructure some rules for the panel and the outer controls. r=jaws

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

Comment 10

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 4 - Share rules that apply only to the summary. r=jaws

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

Comment 11

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 5 - Share rules for download items in the panel. r=jaws

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

Comment 12

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 6 - Shorten complex rules for download items in the panel. r=jaws

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

Comment 13

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 7 - Share rules for download items in the Library window. r=jaws

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

Comment 14

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1244473 - Part 8 - Shorten complex rules for download items the Library window. r=jaws

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

Comment 15

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/33641/#review30317

Note that I've adjusted a few rules, often using width instead of min-width on all platforms, and there was a color that was apparently unnecessary since it was overridden a few rules later on OS X.

::: browser/themes/shared/downloads/downloads.inc.css:86
(Diff revision 1)
> +%ifdef XP_WIN
> +@media (-moz-windows-default-theme) {
> +%endif
> +richlistitem[type="download"]:last-child {
> +  border-bottom: 1px solid transparent;
> +}
> +%ifdef XP_WIN
> +}
> +%endif

While testing, I've noticed that on Windows 7 with the Windows Classic theme there is an unwanted extra border.

Is there any reason at all why the removal of the extra border on the last item is conditional on Windows? I cannot test on Windows 10, but maybe it's an issue there?

Comment 16

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

Comment on attachment 8714022 [details]
MozReview Request: Bug 1244473 - Part 1 - Add shared theme files for Downloads. r=jaws

https://reviewboard.mozilla.org/r/32937/#review30341

Comment 17

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

Comment on attachment 8715836 [details]
MozReview Request: Bug 1244473 - Part 2 - Share simple rules for the panel and the outer controls. r=jaws

https://reviewboard.mozilla.org/r/33635/#review30345

Comment 18

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

Comment on attachment 8715837 [details]
MozReview Request: Bug 1244473 - Part 3 - Restructure some rules for the panel and the outer controls. r=jaws

https://reviewboard.mozilla.org/r/33637/#review30347

::: browser/themes/linux/downloads/downloads.css:18
(Diff revision 1)
> -#downloadsPanel[keyfocus] > #downloadsFooter > #downloadsHistory:focus > .button-box {
> +@keyfocus@ #downloadsSummary:focus,
> +@keyfocus@ #downloadsHistory:focus > .button-box {

I first wrote a comment about why we're not suing child selectors here anymore, but thinking more about it I've decided that they're not necessary between two ID selectors as long as the DOM structure between the elements doesn't change.

Comment 19

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

Comment on attachment 8715838 [details]
MozReview Request: Bug 1244473 - Part 4 - Share rules that apply only to the summary. r=jaws

https://reviewboard.mozilla.org/r/33639/#review30355

::: browser/themes/shared/downloads/downloads.inc.css:44
(Diff revision 1)
> +%ifdef XP_MACOSX
> +@media (min-resolution: 2dppx) {
> +  #downloadsSummary > .downloadTypeIcon {
> +    list-style-image: url("chrome://browser/skin/downloads/[e-mail]");
> +  }
> +}
> +%endif

Can you please file a follow-up bug to get a 2x download-summary icon for Windows and Linux?

Comment 20

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

Comment on attachment 8715839 [details]
MozReview Request: Bug 1244473 - Part 5 - Share rules for download items in the panel. r=jaws

https://reviewboard.mozilla.org/r/33641/#review30375

Comment 21

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

https://reviewboard.mozilla.org/r/33641/#review30317

> While testing, I've noticed that on Windows 7 with the Windows Classic theme there is an unwanted extra border.
> 
> Is there any reason at all why the removal of the extra border on the last item is conditional on Windows? I cannot test on Windows 10, but maybe it's an issue there?

Both Windows 8 and 10 don't offer the classic theme. As to why on Windows we only remove that border-bottom if it's the default theme, well, hg blame shows that you added this in http://hg.mozilla.org/mozilla-central/rev/913f7811c068 ;)

Looking at bug 726444, it's hard to see why this was done only for the default-theme. It's possible that the design changed between the time that bug 726444 landed and now, and that it made more sense for the Classic theme then and doesn't so much now. I'll take your word that if it looks ugly on Classic theme Windows 7, then feel free to remove that condition.

Comment 22

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

Comment on attachment 8715840 [details]
MozReview Request: Bug 1244473 - Part 6 - Shorten complex rules for download items in the panel. r=jaws

https://reviewboard.mozilla.org/r/33643/#review30387

::: browser/themes/osx/downloads/downloads.css:87
(Diff revision 1)
> -#downloadsPanel[keyfocus] > #downloadsListBox:focus > richlistitem[type="download"]:hover[selected] > stack > .downloadButton.downloadConfirmBlock,
> -#downloadsPanel[keyfocus] > #downloadsListBox:focus > richlistitem[type="download"]:hover[selected] > stack > .downloadButton.downloadCancel {
> +@keyfocus@ @itemFocused@:hover .downloadButton.downloadConfirmBlock,
> +@keyfocus@ @itemFocused@:hover .downloadButton.downloadCancel {

How does :hover work with @keyfocus@ here? I see that this is not a change from the previous code, but that similarily doesn't make sense.

Are these rules (and the rules below) dead?

Comment 23

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

Comment on attachment 8715841 [details]
MozReview Request: Bug 1244473 - Part 7 - Share rules for download items in the Library window. r=jaws

https://reviewboard.mozilla.org/r/33645/#review30397

Comment 24

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

Comment on attachment 8715842 [details]
MozReview Request: Bug 1244473 - Part 8 - Shorten complex rules for download items the Library window. r=jaws

https://reviewboard.mozilla.org/r/33647/#review30399

Comment 25

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

https://reviewboard.mozilla.org/r/33641/#review30317

Yeah I'm not sure why min-width was used only on Linux. The icon referenced in the Linux theme is 32x32, so setting min-width shouldn't have been necessary. Maybe at the time it was using GTK icons and was needed to scale them correctly? I don't see download-summary.png in the original changeset (http://hg.mozilla.org/mozilla-central/rev/913f7811c068) and we are now shipping our own icon here.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> How does :hover work with @keyfocus@ here? I see that this is not a change
> from the previous code, but that similarily doesn't make sense.
> 
> Are these rules (and the rules below) dead?

Note the panel has very special (and hack-ish) keyboard navigation handling, once you open it you can navigate through it with keyboard, but you can also use the mouse. So those rules are there to avoid making it ugly when you do both.

Comment 27

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #26)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> > How does :hover work with @keyfocus@ here? I see that this is not a change
> > from the previous code, but that similarily doesn't make sense.
> > 
> > Are these rules (and the rules below) dead?
> 
> Note the panel has very special (and hack-ish) keyboard navigation handling,
> once you open it you can navigate through it with keyboard, but you can also
> use the mouse. So those rules are there to avoid making it ugly when you do
> both.

Yeah, that's what comment 4 and attachment 8714318 [details] are about. I agree it doesn't make a lot of sense to keep the hover state while the panel is in keyfocus mode, but we can fix it later with the redesign.

(I also wonder how much value we get from handling keyboard navigation in the panel. We have a fully accessible view in the Library window anyways.)

Comment 28

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> Both Windows 8 and 10 don't offer the classic theme. As to why on Windows we
> only remove that border-bottom if it's the default theme, well, hg blame
> shows that you added this in
> http://hg.mozilla.org/mozilla-central/rev/913f7811c068 ;)
> 
> Looking at bug 726444, it's hard to see why this was done only for the
> default-theme. It's possible that the design changed between the time that
> bug 726444 landed and now, and that it made more sense for the Classic theme
> then and doesn't so much now. I'll take your word that if it looks ugly on
> Classic theme Windows 7, then feel free to remove that condition.

Thanks for checking, I'll remove the condition for Windows.

And thanks for the quick review turnaround! I'm glad this refactoring can already land today.

Comment 29

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #27)
> (I also wonder how much value we get from handling keyboard navigation in
> the panel. We have a fully accessible view in the Library window anyways.)

I honestly don't recall, the original bug may have more insight about the reasoning.
I suspect it was a way to reduce the impact of a new UI on users used to the old one.

Comment 30

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #28)
> Thanks for checking, I'll remove the condition for Windows.
> 
> And thanks for the quick review turnaround! I'm glad this refactoring can
> already land today.

And also thanks to the team who implemented artifact builds, "./mach build faster", and who pushes for really using the features of distributed version control.

This time, I was able to amend the changeset on Mac, rebase for landing in fx-team, then pull and test on Windows in less than 15 minutes overall!

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9c2c61d4182f
https://hg.mozilla.org/integration/fx-team/rev/dbeeaacdb761
https://hg.mozilla.org/integration/fx-team/rev/00cd4c6b3fba
https://hg.mozilla.org/integration/fx-team/rev/1ae77c0c84cf
https://hg.mozilla.org/integration/fx-team/rev/5c97719a060f
https://hg.mozilla.org/integration/fx-team/rev/59dc18d174c2
https://hg.mozilla.org/integration/fx-team/rev/807333b6fa21
https://hg.mozilla.org/integration/fx-team/rev/f0cf2f672254

Comment 32

[:Paolo Amadini]

Jared, let me know if you'd like to review bug 1117145 as well. Overall I think the changes in that bug will be easier to follow, so I could also ask someone less familiar with the Downloads Panel code. I've posted a work-in-progress patch though there are more steps before the binding can be unified.

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9c2c61d4182f
https://hg.mozilla.org/mozilla-central/rev/dbeeaacdb761
https://hg.mozilla.org/mozilla-central/rev/00cd4c6b3fba
https://hg.mozilla.org/mozilla-central/rev/1ae77c0c84cf
https://hg.mozilla.org/mozilla-central/rev/5c97719a060f
https://hg.mozilla.org/mozilla-central/rev/59dc18d174c2
https://hg.mozilla.org/mozilla-central/rev/807333b6fa21
https://hg.mozilla.org/mozilla-central/rev/f0cf2f672254

Comment 34

[Paul Silaghi, QA [:pauly]]

I'm not sure I see many differences looking at builds before/after the fix.
What should I be looking for exactly?

Comment 35

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #34)
> I'm not sure I see many differences looking at builds before/after the fix.
> What should I be looking for exactly?

Apart from a minor fix with an extra border on the last download item in the Windows 7 Classic theme, nothing should have changed and we should just make sure that there are no regressions in the styling of the Downloads Panel and the Downloads View in the Library window.

Comment 36

[Paul Silaghi, QA [:pauly]]

Everything's fine on Windows 7 Classic/Aero themes, Ubuntu 14.04, OS X 10.10.5.
Verified fixed Fx 47.0a1 (2016-02-10).