Unify the Downloads theme files across platforms

VERIFIED FIXED in Firefox 47

Status

()

Firefox
Downloads Panel
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox47 verified)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
104.24 KB, image/png
Details
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
58 bytes, text/x-review-board-request
jaws
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
Flags: qe-verify+
(Assignee)

Comment 1

2 years ago
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.
Flags: needinfo?(jaws)
(Assignee)

Comment 2

2 years ago
Created attachment 8714022 [details]
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/
Yes, I'll be available to review it :)
Flags: needinfo?(jaws)
(Assignee)

Comment 4

2 years ago
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.
Depends on: 819428
(Assignee)

Comment 5

2 years ago
Created attachment 8714318 [details]
Hover state in keyboard focus mode
(Assignee)

Comment 6

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8714022 - Attachment description: MozReview Request: Bug 1244473 - Part 1 - Add shared theme files for Downloads. → MozReview Request: Bug 1244473 - Part 1 - Add shared theme files for Downloads. r=jaws
Attachment #8714022 - Flags: review?(jaws)
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8715836 [details]
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/
Attachment #8715836 - Flags: review?(jaws)
(Assignee)

Comment 9

2 years ago
Created attachment 8715837 [details]
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/
Attachment #8715837 - Flags: review?(jaws)
(Assignee)

Comment 10

2 years ago
Created attachment 8715838 [details]
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/
Attachment #8715838 - Flags: review?(jaws)
(Assignee)

Comment 11

2 years ago
Created attachment 8715839 [details]
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/
Attachment #8715839 - Flags: review?(jaws)
(Assignee)

Comment 12

2 years ago
Created attachment 8715840 [details]
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/
Attachment #8715840 - Flags: review?(jaws)
(Assignee)

Comment 13

2 years ago
Created attachment 8715841 [details]
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/
Attachment #8715841 - Flags: review?(jaws)
(Assignee)

Comment 14

2 years ago
Created attachment 8715842 [details]
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/
Attachment #8715842 - Flags: review?(jaws)
(Assignee)

Comment 15

2 years ago
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 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
Attachment #8714022 - Flags: review?(jaws) → review+
Attachment #8715836 - Flags: review?(jaws) → review+
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 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.
Attachment #8715837 - Flags: review?(jaws) → review+
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/download-summary@2x.png");
> +  }
> +}
> +%endif

Can you please file a follow-up bug to get a 2x download-summary icon for Windows and Linux?
Attachment #8715838 - Flags: review?(jaws) → review+
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
Attachment #8715839 - Flags: review?(jaws) → review+
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.
Attachment #8715840 - Flags: review?(jaws) → review+
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 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
Attachment #8715841 - Flags: review?(jaws) → review+
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
Attachment #8715842 - Flags: review?(jaws) → review+
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.
(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.
(Assignee)

Comment 27

2 years ago
(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.)
(Assignee)

Updated

2 years ago
Depends on: 1246126
(Assignee)

Comment 28

2 years ago
(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.
(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.
(Assignee)

Comment 30

2 years ago
(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

2 years ago
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
(Assignee)

Comment 32

2 years ago
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
QA Contact: paul.silaghi
I'm not sure I see many differences looking at builds before/after the fix.
What should I be looking for exactly?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 35

2 years ago
(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.
Flags: needinfo?(paolo.mozmail)
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).
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
You need to log in before you can comment on or make changes to this bug.