Use the new In-Content background color in contentAreaDownloadsView

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

5 years ago
The contentAreaDownloadsView, the downloads tab in private mode, uses the old styling of the add-on manager. With the incontent prefs and the new add-on manager styling the background with it's different gradients looks a little bit like an alien. I think it's better to use the Chameleon background color (#f1f1f1) instead.
Assignee

Comment 1

5 years ago
Posted patch AreaDownloadsViewColor.patch (obsolete) — Splinter Review
This patch uses directly the styles in contentAreaDownloadsView.css instead of using a @import. I think this rule is simple enough to not need a @import.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8519524 - Flags: review?(gavin.sharp)
Attachment #8519524 - Flags: review?(jaws)
Attachment #8519524 - Flags: review?(gijskruitbosch+bugs)
Attachment #8519524 - Flags: review?(gavin.sharp)

Comment 2

5 years ago
Comment on attachment 8519524 [details] [diff] [review]
AreaDownloadsViewColor.patch

(In reply to Richard Marti (:Paenglab) from comment #1)
> Created attachment 8519524 [details] [diff] [review]
> AreaDownloadsViewColor.patch
> 
> This patch uses directly the styles in contentAreaDownloadsView.css instead
> of using a @import. I think this rule is simple enough to not need a @import.

Why the padding change? Why remove the existing @import if it didn't break/fix this anyway? Why is all this in the individual theme files instead of in a shared file and/or the content file?
Attachment #8519524 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 3

5 years ago
(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8519524 [details] [diff] [review]
> AreaDownloadsViewColor.patch
> 
> (In reply to Richard Marti (:Paenglab) from comment #1)
> > Created attachment 8519524 [details] [diff] [review]
> > AreaDownloadsViewColor.patch
> > 
> > This patch uses directly the styles in contentAreaDownloadsView.css instead
> > of using a @import. I think this rule is simple enough to not need a @import.
> 
> Why the padding change? Why remove the existing @import if it didn't
> break/fix this anyway? Why is all this in the individual theme files instead
> of in a shared file and/or the content file?

Where is a padding change? The 18px padding was already in inContentUI.css.
Remove the @import to not use something like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/inContentUI.css#37
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 4

5 years ago
Posted patch AreaDownloadsViewColor.patch (obsolete) — Splinter Review
Ah, I think I know now what Gijs thought about the padding change. OS X doesn't use contentAreaDownloadsView.css and without it has no padding. I removed now the not packaged file on OS X and the other two platforms are using now a shared file.
Attachment #8519524 - Attachment is obsolete: true
Attachment #8519524 - Flags: review?(jaws)
Attachment #8520146 - Flags: review?(jaws)

Comment 5

5 years ago
(In reply to Richard Marti (:Paenglab) from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Comment on attachment 8519524 [details] [diff] [review]
> > AreaDownloadsViewColor.patch
> > 
> > (In reply to Richard Marti (:Paenglab) from comment #1)
> > > Created attachment 8519524 [details] [diff] [review]
> > > AreaDownloadsViewColor.patch
> > > 
> > > This patch uses directly the styles in contentAreaDownloadsView.css instead
> > > of using a @import. I think this rule is simple enough to not need a @import.
> > 
> > Why the padding change? Why remove the existing @import if it didn't
> > break/fix this anyway? Why is all this in the individual theme files instead
> > of in a shared file and/or the content file?
> 
> Where is a padding change? The 18px padding was already in inContentUI.css.
> Remove the @import to not use something like this:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> inContentUI.css#37

Well, you added padding and removed inContentUI.css. I don't really understand why we shouldn't just keep inContentUI.css and do without the explicit padding in this file, and if necessary update inContentUI.css to have the right background here... Anyway, I'm just going to let Jared deal with this review, as he knows the in content CSS a lot better than me anyway.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 6

5 years ago
Without the padding it would look like on OS X, the richlist box fills the complete window. Then it makes also no sense to @import inContentUI.css.

When we want to update inContentUI.css with the new background, we have to wait until bug 989469 lands to not break the actual Add-on manager styling.
Assignee

Comment 7

5 years ago
Posted patch AreaDownloadsViewColor.patch (obsolete) — Splinter Review
Jared asked over IRC to package contentAreaDownloadsView.css also on OS X.
Attachment #8520146 - Attachment is obsolete: true
Attachment #8520146 - Flags: review?(jaws)
Attachment #8520871 - Flags: review?(jaws)

Comment 8

5 years ago
Why don't you move all styles in shared/ so we get a consistent styling across platforms ?
Assignee

Comment 9

5 years ago
The remaining styles are to override platform specific button rules. When bug 989469 lands we could style the richlistitems like the add-on manager and use a shared allDownloadsViewOverlay.css.
Comment on attachment 8520871 [details] [diff] [review]
AreaDownloadsViewColor.patch

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

Visually this looks good.

I'd rather that we keep the @import for inContentUI.css, and then in that file add a rule for *|*#contentAreaDownloadsView that overrides the one for *|*:root. This way we don't have to duplicate the extra styles that were added to the Windows version of the contentAreaDownloadsView.css, and any other styles that may have accidentally been forgotten.

We should then file a follow-up bug (or make it par of 989469) to remove the override for *|*#contentAreaDownloadsView.
Attachment #8520871 - Flags: review?(jaws) → feedback+
Assignee

Comment 11

5 years ago
Is this what you thought?
Attachment #8520871 - Attachment is obsolete: true
Attachment #8521719 - Flags: review?(jaws)
Comment on attachment 8521719 [details] [diff] [review]
AreaDownloadsViewColor.patch

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

Yeah, did you find any issues with this approach?
Attachment #8521719 - Flags: review?(jaws) → review+
Assignee

Comment 13

5 years ago
I started a try build with tests on OS X to see if the new padding on OS X will make tests fail. The failing test on social_flyout.js don't look caused by this patch as this styles aren't used there.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7c8c1c43d7b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2cdf3d16397f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36

Comment 16

5 years ago
Comment on attachment 8521719 [details] [diff] [review]
AreaDownloadsViewColor.patch

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

::: toolkit/themes/osx/global/inContentUI.css
@@ +23,5 @@
>  
> +/* Use the new in-content colors for #contentAreaDownloadsView. After landing
> +   of bug 989469 the colors can be moved to *|*:root */
> +*|*#contentAreaDownloadsView {
> +  background: #f1f1f1;

Note that the new in-content page background color is #fbfbfb. This new color is already in common.css, so it shouldn't be too dramatic to keep #f1f1f1 for a while (since we're gonna switch to the common.css stylesheet later).

Updated

5 years ago
Blocks: 989469
You need to log in before you can comment on or make changes to this bug.