1096010 - Use the new In-Content background color in contentAreaDownloadsView

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

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.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 2

[:Gijs (he/him)]

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?

Comment 3

[Richard Marti (:Paenglab)]

(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

Comment 4

[Richard Marti (:Paenglab)]

Attachment: AreaDownloadsViewColor.patch

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.

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[Richard Marti (:Paenglab)]

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.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: AreaDownloadsViewColor.patch

Jared asked over IRC to package contentAreaDownloadsView.css also on OS X.

Comment 8

[Tim Nguyen :ntim]

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

Comment 9

[Richard Marti (:Paenglab)]

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 10

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

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.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: AreaDownloadsViewColor.patch

Is this what you thought?

Comment 12

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

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?

Comment 13

[Richard Marti (:Paenglab)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2cdf3d16397f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2cdf3d16397f

Comment 16

[Tim Nguyen :ntim]

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).