1267047 - Reuse more classes/styles/variables from info-pages.css stylesheet for about:privatebrowsing

Status: RESOLVED WORKSFORME

(Firefox :: Private Browsing, defect)

Description

[Tim Nguyen :ntim]

Benefits:
- less code duplication
- info-pages.css includes responsive styles that about:privatebrowsing can benefit from
- In case the spec for in-content pages is updated (font-size, borders, ...), all in-content pages are updated at once

Comment 1

[:Gijs (he/him)]

I still don't understand why you didn't listen to Ricky or at least read the patch (or one of the many comments in the bug) before commenting and filing this bug, but in case there was any doubt:

https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l10.9

+@import url("chrome://global/skin/in-content/info-pages.css");

--> INVALID.

Comment 2

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #1)
> I still don't understand why you didn't listen to Ricky or at least read the
> patch (or one of the many comments in the bug) before commenting and filing
> this bug, but in case there was any doubt:
> https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l10.9
> 
> +@import url("chrome://global/skin/in-content/info-pages.css");
I did read the patch, and it seemed to me that the patch wasn't reusing the stylesheet, because almost nothing is being reused from the stylesheet. I only see .title being reused (and being completely overriden by aboutPrivateBrowsing.css). I would at least expect classes like .title-text or .container being reused. As info-pages.css is including common.css, I would expect most color/background to be done by overriding variables.
In fact, including url("chrome://global/skin/in-content/info-pages.css") is almost useless as aboutPrivateBrowsing.css is specifying almost all the CSS.

> https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l10.92
> +.title {
> +  background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
> +  background-size: 64px;
> +  background-position: left, center;
> +  font-weight: lighter;
> +  line-height: 1.5em;
> +  color: var(--color-darkTheme-purple-text);
> +  min-height: 64px;
> +  -moz-margin-start: 0;
> +  -moz-padding-start: calc(var(--icon-margin) + 24px);
> +}
Why is this block even needed with info-pages.css included ?
Also, all these overrides make the responsive styles from info-pages.css useless (I've tested the patch, and I don't see the page icon being hidden when the window width is small): https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/info-pages.inc.css#58


> https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l3.48
> +        <span id="title">&privateBrowsing.title;</span>
> +        <span id="titleTracking">&privateBrowsing.title.tracking;</span>
Why aren't these using the .title-text class ?

> https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l10.40
> +a:link {
> +  color: var(--color-blue);
> +  text-decoration: none;
> +}
> +
> +a:hover {
> +  color: var(--color-blue-dark);
> +  text-decoration: underline;
> +}
> +
> +a:hover:active {
> +  color: var(--color-blue-darker);
> +}
> +
> +a:visited {
> +  color: var(--color-blue-darker);
> +}
Why can't we simply override var(--in-content-link-color), var(--in-content-link-color-hover), var(--in-content-link-color-active); ?
Also, the text-decoration styles are not needed, the same are included in common.css (which is included inside info-pages.css).

https://hg.mozilla.org/integration/fx-team/rev/463fc6d36ff5#l10.29
> +body {
> +  padding: 40px;
> +  background-color: var(--color-background-light);
> +  font-size: 1.5em;
> +}
> +
> +body.private {
> +  color: var(--color-darkTheme-purple-text);
> +  background-color: var(--color-background-dark-purple);
> +}
> +
We have these variables:
--in-content-page-color
--in-content-page-background
--in-content-text-color

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch

Here's a patch that simplifies the stylesheet. Also makes responsive styles actually apply. Cherry on the cake: RTL support.

Comment 4

[Tim Nguyen :ntim]

Attachment: Patch

Comment 5

[:Gijs (he/him)]

Comment on attachment 8744696 [details] [diff] [review]
Patch

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

Please submit through mozreview instead.

Please update the title of the bug to explain what you're doing.

I don't understand why the .private/.normal classes need to live on <html> instead of <body>; that doesn't seem like a useful change. I haven't looked at anything besides (content) aboutPrivateBrowsing.css/js

Comment 6

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1267047 - Simplify aboutPrivateBrowsing.css by reusing at most info-pages.css. r=Gijs

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

Comment 7

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8744696 [details] [diff] [review]
> Patch
> 
> Review of attachment 8744696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please submit through mozreview instead.
> 
> Please update the title of the bug to explain what you're doing.
Modified slightly the title.

> I don't understand why the .private/.normal classes need to live on <html>
> instead of <body>; that doesn't seem like a useful change. I haven't looked
> at anything besides (content) aboutPrivateBrowsing.css/js
In order to override --in-content-page-color, --in-content-page-background and --in-content-text-color, the private class needs to be on the <html> element, because those variables are used on the html elements, therefor overriding them on <body> does nothing.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8744740 [details]
MozReview Request: Bug 1267047 - Simplify aboutPrivateBrowsing.css by reusing at most info-pages.css. r=Gijs

https://reviewboard.mozilla.org/r/48667/#review45473

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:37
(Diff revision 1)
> -        <span id="title">&privateBrowsing.title;</span>
> -        <span id="titleTracking">&privateBrowsing.title.tracking;</span>
> +        <span id="title" class="title-text">&privateBrowsing.title;</span>
> +        <span id="titleTracking" class="title-text">&privateBrowsing.title.tracking;</span>

There seems to be no benefit to this change, but the downside is now we have to explicitly override styles from info-pages.css to not get the bottom border. Let's just leave them out?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:14
(Diff revision 1)
> -  --color-blue: #0996f8;
> -  --color-blue-dark: #0670cc;
> -  --color-blue-darker: #005bab;
> +  --in-content-link-color: #0996f8;
> +  --in-content-link-color-hover: #0670cc;
> +  --in-content-link-color-active: #005bab;

You've removed the visited coloring, so now it uses the default from common.css, which is wrong.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
> -}
> -
> -body {
> -  padding: 40px;
> -  background-color: var(--color-background-light);
> -  font-size: 1.5em;

Why remove the font-size changes?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
> -  -moz-margin-start: var(--icon-margin);
> -  -moz-padding-start: 24px;

With these properties removed, the margin/padding here now depends on the font size of the document. It's a mistake that that font-size is currently fixed in common.inc and not OS-dependent. Please don't exacerbate it by relying on it here, and leave the margin/padding properties in this file alone insofar as it concerns the alignment of the sections, headers and icons.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:39
(Diff revision 1)
>    overflow: auto;
>  }
>  
>  .list-row > ul > li {
> -  float: left;
> -  width: 220px;
> +  float: inline-start;
> +  min-width: 30%;

This doesn't work properly:

http://imgur.com/S9ucdjc

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
>  }
>  
>  .title {
>    background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
> -  background-size: 64px;
> -  background-position: left, center;

The image is now misaligned.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
> -  font-weight: lighter;
> -  line-height: 1.5em;

Why remove the line-height?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
> -  -moz-margin-start: 0;
> -  -moz-padding-start: calc(var(--icon-margin) + 24px);

As noted earlier, please leave the margins/paddings.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
> +}
> +
>  .about-subheader {
>    display: flex;
>    align-items: center;
> -  font-size: 1.5em;

Why remove the font-size here?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:60
(Diff revision 1)
> +  .about-subheader::before {
> +    content: "";

There's no need to put this in a pseudoelement, just leave it in the element, the way it was before. The spacing of the icon is also wrong now.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:78
(Diff revision 1)
> +.about-info {
> +  font-size: .875em;
>  }

Don't move stuff around breaking blame needlessly.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:103
(Diff revision 1)
>    cursor: pointer;
>    min-width: 60px;
>    height: 24px;
>    border-radius: 12px;
>    background-color: var(--color-grey);
> -  border: 1px var(--color-grey) solid;
> +  border: 1px solid transparent;

Why remove the border here, when it was in the mockup?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:121
(Diff revision 1)
> -  left: 0;
>    box-shadow: 0 0 1px 1px hsla(0, 0%, 0%, .1),
>                0 1px 0 hsla(0, 0%, 0%, .2);
>    border-radius: 50%;
>    background: white;
> -  transition: left .2s ease;
> +  transition: offset-inline-start .2s ease;

This doesn't actually seem to work - the animation is broken when I build with your changes.

Comment 9

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8744740 [details]
> MozReview Request: Bug 1267047 - Simplify aboutPrivateBrowsing.css by
> reusing at most info-pages.css. r=Gijs
> 
> https://reviewboard.mozilla.org/r/48667/#review45473
> 
> ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:37
> (Diff revision 1)
> > -        <span id="title">&privateBrowsing.title;</span>
> > -        <span id="titleTracking">&privateBrowsing.title.tracking;</span>
> > +        <span id="title" class="title-text">&privateBrowsing.title;</span>
> > +        <span id="titleTracking" class="title-text">&privateBrowsing.title.tracking;</span>
> 
> There seems to be no benefit to this change, but the downside is now we have
> to explicitly override styles from info-pages.css to not get the bottom
> border. Let's just leave them out?
OK. I wonder if we can't just use the border to be consistent with other in-content pages.

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:14
> (Diff revision 1)
> > -  --color-blue: #0996f8;
> > -  --color-blue-dark: #0670cc;
> > -  --color-blue-darker: #005bab;
> > +  --in-content-link-color: #0996f8;
> > +  --in-content-link-color-hover: #0670cc;
> > +  --in-content-link-color-active: #005bab;
> 
> You've removed the visited coloring, so now it uses the default from
> common.css, which is wrong.
> 
> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> (Diff revision 1)
> > -}
> > -
> > -body {
> > -  padding: 40px;
> > -  background-color: var(--color-background-light);
> > -  font-size: 1.5em;
> 
> Why remove the font-size changes?
I can restore it, but I feel we should be consistent with other in-content pages here. If this needs to be updated across all in-content pages then let's update it in the info-pages sheet altogether, instead of introducing inconsistencies.
> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> (Diff revision 1)
> > -  -moz-margin-start: var(--icon-margin);
> > -  -moz-padding-start: 24px;
> 
> With these properties removed, the margin/padding here now depends on the
> font size of the document. It's a mistake that that font-size is currently
> fixed in common.inc and not OS-dependent. Please don't exacerbate it by
> relying on it here, and leave the margin/padding properties in this file
> alone insofar as it concerns the alignment of the sections, headers and
> icons.
That seems like a problem that should be fixed within info-pages.css. Overriding the margin/padding properties isn't a good idea, because the responsive styles will be overridden as well, which we don't want.

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:39
> (Diff revision 1)
> >    overflow: auto;
> >  }
> >  
> >  .list-row > ul > li {
> > -  float: left;
> > -  width: 220px;
> > +  float: inline-start;
> > +  min-width: 30%;
> 
> This doesn't work properly:
> 
> http://imgur.com/S9ucdjc
Not sure I'm seeing the problem here. Is everything supposed to fit in one line ?

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> (Diff revision 1)
> >  }
> >  
> >  .title {
> >    background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
> > -  background-size: 64px;
> > -  background-position: left, center;
> 
> The image is now misaligned.
> 
> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> (Diff revision 1)
> > -  font-weight: lighter;
> > -  line-height: 1.5em;
> 
> Why remove the line-height?
I didn't notice a difference when removing it (even manually using the devtools where I can see the changes live).

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> (Diff revision 1)
> > +}
> > +
> >  .about-subheader {
> >    display: flex;
> >    align-items: center;
> > -  font-size: 1.5em;
> 
> Why remove the font-size here?
Because it's already 1.5em by default in html.css (I doubt that stylesheet is changing).

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:60
> (Diff revision 1)
> > +  .about-subheader::before {
> > +    content: "";
> 
> There's no need to put this in a pseudoelement, just leave it in the
> element, the way it was before. The spacing of the icon is also wrong now.
I think this fits better as a pseudo-element, since we have more control over the sizing (in fact I plan on changing that for the title in info-pages.css (probably not in this bug though)). Anyway, I'll restore the old way in the scope of this bug.

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:78
> (Diff revision 1)
> > +.about-info {
> > +  font-size: .875em;
> >  }
> 
> Don't move stuff around breaking blame needlessly.
I moved it because it makes more sense to group the title styles together.

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:103
> (Diff revision 1)
> >    cursor: pointer;
> >    min-width: 60px;
> >    height: 24px;
> >    border-radius: 12px;
> >    background-color: var(--color-grey);
> > -  border: 1px var(--color-grey) solid;
> > +  border: 1px solid transparent;
> 
> Why remove the border here, when it was in the mockup?
border: 1px solid transparent == border: 1px solid var(--color-grey) (unless background-clip is content-box). I've changed it so we don't have to repeat the color, let me know if you want me to undo this change.

> ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:121
> (Diff revision 1)
> > -  left: 0;
> >    box-shadow: 0 0 1px 1px hsla(0, 0%, 0%, .1),
> >                0 1px 0 hsla(0, 0%, 0%, .2);
> >    border-radius: 50%;
> >    background: white;
> > -  transition: left .2s ease;
> > +  transition: offset-inline-start .2s ease;
> 
> This doesn't actually seem to work - the animation is broken when I build
> with your changes.
We can either get RTL support, either the transition. Should I remove RTL support or should I just remove the transition definition ?
Bug 1267063 was filed about getting the logical properties animated.

Comment 10

[:Gijs (he/him)]

(In reply to Tim Nguyen :ntim (busy, email me instead) from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Comment on attachment 8744740 [details]
> > MozReview Request: Bug 1267047 - Simplify aboutPrivateBrowsing.css by
> > reusing at most info-pages.css. r=Gijs
> > 
> > https://reviewboard.mozilla.org/r/48667/#review45473
> > 
> > ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:37
> > (Diff revision 1)
> > > -        <span id="title">&privateBrowsing.title;</span>
> > > -        <span id="titleTracking">&privateBrowsing.title.tracking;</span>
> > > +        <span id="title" class="title-text">&privateBrowsing.title;</span>
> > > +        <span id="titleTracking" class="title-text">&privateBrowsing.title.tracking;</span>
> > 
> > There seems to be no benefit to this change, but the downside is now we have
> > to explicitly override styles from info-pages.css to not get the bottom
> > border. Let's just leave them out?
> OK. I wonder if we can't just use the border to be consistent with other
> in-content pages.

This is a question for UX.

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > (Diff revision 1)
> > > -}
> > > -
> > > -body {
> > > -  padding: 40px;
> > > -  background-color: var(--color-background-light);
> > > -  font-size: 1.5em;
> > 
> > Why remove the font-size changes?
> I can restore it, but I feel we should be consistent with other in-content
> pages here. If this needs to be updated across all in-content pages then
> let's update it in the info-pages sheet altogether, instead of introducing
> inconsistencies.

As is this. Though, FWIW, considering the lighter background, I would not be surprised at wanting to have alternative font sizes and weights compared to the normal error pages.

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > (Diff revision 1)
> > > -  -moz-margin-start: var(--icon-margin);
> > > -  -moz-padding-start: 24px;
> > 
> > With these properties removed, the margin/padding here now depends on the
> > font size of the document. It's a mistake that that font-size is currently
> > fixed in common.inc and not OS-dependent. Please don't exacerbate it by
> > relying on it here, and leave the margin/padding properties in this file
> > alone insofar as it concerns the alignment of the sections, headers and
> > icons.
> That seems like a problem that should be fixed within info-pages.css.
> Overriding the margin/padding properties isn't a good idea, because the
> responsive styles will be overridden as well, which we don't want.

So let's fix it in info-pages.css . Also fine to leave this for another bug - but then don't change about:privatebrowsing's margin/padding.

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:39
> > (Diff revision 1)
> > >    overflow: auto;
> > >  }
> > >  
> > >  .list-row > ul > li {
> > > -  float: left;
> > > -  width: 220px;
> > > +  float: inline-start;
> > > +  min-width: 30%;
> > 
> > This doesn't work properly:
> > 
> > http://imgur.com/S9ucdjc
> Not sure I'm seeing the problem here. Is everything supposed to fit in one
> line ?

No, it's supposed to fit in two columns. See the mockup: http://people.mozilla.org/~bbell/about-page-improvements/about-privateBrowsing.html

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > (Diff revision 1)
> > >  }
> > >  
> > >  .title {
> > >    background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
> > > -  background-size: 64px;
> > > -  background-position: left, center;
> > 
> > The image is now misaligned.
> > 
> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > (Diff revision 1)
> > > -  font-weight: lighter;
> > > -  line-height: 1.5em;
> > 
> > Why remove the line-height?
> I didn't notice a difference when removing it (even manually using the
> devtools where I can see the changes live).

That's odd, because I see a difference when adding it back - the title shifts, and this seems related to the misalignment of the icon.

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > (Diff revision 1)
> > > +}
> > > +
> > >  .about-subheader {
> > >    display: flex;
> > >    align-items: center;
> > > -  font-size: 1.5em;
> > 
> > Why remove the font-size here?
> Because it's already 1.5em by default in html.css (I doubt that stylesheet
> is changing).

But you changed the font-size on body as well, and so now the result is smaller...

> > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:121
> > (Diff revision 1)
> > > -  left: 0;
> > >    box-shadow: 0 0 1px 1px hsla(0, 0%, 0%, .1),
> > >                0 1px 0 hsla(0, 0%, 0%, .2);
> > >    border-radius: 50%;
> > >    background: white;
> > > -  transition: left .2s ease;
> > > +  transition: offset-inline-start .2s ease;
> > 
> > This doesn't actually seem to work - the animation is broken when I build
> > with your changes.
> We can either get RTL support, either the transition. Should I remove RTL
> support or should I just remove the transition definition ?

I disagree with your assertion. We could add rules with the relevant selectors (body[dir=rtl] or :-moz-ui-locale-dir or whatever it is... probably best to use the former to reduce confusion when using force rtl and/or manually changing that attribute) that use position: right instead, and keep it animatable.

Comment 11

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Tim Nguyen :ntim from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > Comment on attachment 8744740 [details]
> > > MozReview Request: Bug 1267047 - Simplify aboutPrivateBrowsing.css by
> > > reusing at most info-pages.css. r=Gijs
> > > 
> > > https://reviewboard.mozilla.org/r/48667/#review45473
> > > 
> > > ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:37
> > > (Diff revision 1)
> > > > -        <span id="title">&privateBrowsing.title;</span>
> > > > -        <span id="titleTracking">&privateBrowsing.title.tracking;</span>
> > > > +        <span id="title" class="title-text">&privateBrowsing.title;</span>
> > > > +        <span id="titleTracking" class="title-text">&privateBrowsing.title.tracking;</span>
> > > 
> > > There seems to be no benefit to this change, but the downside is now we have
> > > to explicitly override styles from info-pages.css to not get the bottom
> > > border. Let's just leave them out?
> > OK. I wonder if we can't just use the border to be consistent with other
> > in-content pages.
> 
> This is a question for UX.
> 
> > > ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
> > > (Diff revision 1)
> > > > -}
> > > > -
> > > > -body {
> > > > -  padding: 40px;
> > > > -  background-color: var(--color-background-light);
> > > > -  font-size: 1.5em;
> > > 
> > > Why remove the font-size changes?
> > I can restore it, but I feel we should be consistent with other in-content
> > pages here. If this needs to be updated across all in-content pages then
> > let's update it in the info-pages sheet altogether, instead of introducing
> > inconsistencies.
> 
> As is this. Though, FWIW, considering the lighter background, I would not be
> surprised at wanting to have alternative font sizes and weights compared to
> the normal error pages.

needinfo for the 2 points above.

Comment 12

[Tim Nguyen :ntim]

I've filed bug 1267858 for the info-pages.css changes.

Comment 13

[Tim Nguyen :ntim]

The new activity stream like layout is now different enough for code sharing to become unnecessary.