1117258 - Implement CSS styling for desktop about:reader content

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

I punted on this in previous bugs, so we still need to implement the CSS styles for desktop about:reader.

Hopefully we can share a lot with mobile here, but the toolbar will need to be styled differently. Also, we'll need to hide the toolbar items related to reading list, since there isn't actually a reading list on desktop yet.

mmaslaney, do you have an updated spec document that you would like me to work from? I can always start with what you have here:

http://people.mozilla.org/~mmaslaney/readermode/type_light.html

Comment 1

[Michael Maslaney [:mmaslaney] ([e-mail])]

Margaret, 

I'll have an updated spec for you early this week. Feel free to start on the framework.

Couple notable changes to anticipate:

• Using Fira San, not Clear San

• Type specs have been updated: https://docs.google.com/a/mozilla.com/spreadsheets/d/16GjU0uiIFaT214LzPUm0-hknzoEylbmgpZDuA10EOM0/edit#gid=1869969898

• New Print theme (we may make this a v2 if the workload is heavy)

• Colors on the controls will slightly differ from spec

Comment 2

[:Margaret Leibovic]

I started a patch for this bug to create a shared aboutReaderContent.css file, and hopefully we can just use responsive design to share mobile/desktop content styling.

I filed bug 1120735 about implementing the controls for desktop, and antlam filed bug 1120004 about updating the controls for mobile.

Comment 3

[:Margaret Leibovic]

I'm actually going to take this opportunity to update both the desktop/mobile content here, since I'm making a shared content file.

Our goal is to be consistent across platforms, and there's nothing more consistent than a shared CSS file! :)

I have some WIP build here for feedback:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c57b64247945/try-macosx64/firefox-38.0a1.en-US.mac.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c57b64247945/try-android-api-11/fennec-38.0a1.en-US.android-arm.apk

If you are so inclined to peek at the details, here are the actual CSS changes I made:
https://reviewboard.mozilla.org/r/2773/diff/0

I tried to make changes to match these documents:
http://people.mozilla.org/~mmaslaney/readermode/type_light.html
https://docs.google.com/a/mozilla.com/spreadsheets/d/16GjU0uiIFaT214LzPUm0-hknzoEylbmgpZDuA10EOM0/edit#gid=1869969898

However, I only implemented the desktop size spec to start, and I used rem values to try to make it easier to account for users incresing/decreasing font sizes.

I'm thinking that to implement some of the size differences for mobile/tablet, I would like to use media queries to check for screen size, but this will also affect small desktop windows. However, the current specs for desktop/tablet are different, and we couldn't distinguish between these two with media queries. antlam, can you explain the reasoning behind having different sizes on tablet? Why do we want these to be larger than they would be on a desktop screen of similar size?

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #3)
> I'm thinking that to implement some of the size differences for
> mobile/tablet, I would like to use media queries to check for screen size,
> but this will also affect small desktop windows. However, the current specs
> for desktop/tablet are different, and we couldn't distinguish between these
> two with media queries. 

Hm, good point.. Maybe we can

> antlam, can you explain the reasoning behind having
> different sizes on tablet? Why do we want these to be larger than they would
> be on a desktop screen of similar size?

From reading a couple studies, (and from what I've noticed myself as well) the different sizes on Tablets are really there for the way users use those devices. Specifically, they hold them further away, they interact with them (usually) in a different setting, and so this has a direct effect on type and legibility that is more finicky. 

E.g. the same font size held further away, looks smaller is is therefore less legible.

With that being said, I think if we have trouble distinguishing, we should try to make them as similar as possible

Comment 5

[:Margaret Leibovic]

Change of plans. After talking to mmaslaney and antlam, I'm just going to focus on implementing the desktop CSS here, and we can look at consolidating shared styles in the future.

Comment 6

[:Margaret Leibovic]

Attachment: Implement reader mode content CSS for desktop

This is a slimmed-down version of the Android aboutReader.css, updated to follow the specs here:

https://projects.invisionapp.com/share/XT21LURF8#/screens/57292979?maintainScrollPosition=false
https://docs.google.com/a/mozilla.com/spreadsheets/d/16GjU0uiIFaT214LzPUm0-hknzoEylbmgpZDuA10EOM0/edit#gid=1869969898

This doesn't account for giving the user the ability to increase/decrease the font size. I'll deal with that when I implement the controls for that in bug 1120735.

I'm not sure why the Android CSS has so many !important declarations, but this seems to work fine without them... it looks like Readability.js strips out other styles, so I'm not sure what the !important would need to override.

bnicholson, do you remember how we originally came up with some of the selectors for the Android styles? It seems a bit odd that we have what seems like wordpress-specific styles in there.

st3fan, I remember us talking about the image margins on IRC the other day, and I'm trying to remember what fix you implemented for that. Moving around the _updateImageMargins call in here appears to fix things for me. Is that what you did as well?

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to :Margaret Leibovic from comment #6)

> st3fan, I remember us talking about the image margins on IRC the other day,
> and I'm trying to remember what fix you implemented for that. Moving around
> the _updateImageMargins call in here appears to fix things for me. Is that
> what you did as well?

Yup. Here is what I have in the iOS code with a comment that explains it.

 // The order here is important. Because updateImageMargins depends on contentElement.offsetWidth which
 // will not be set until contentElement is visible. If this leads to annoying content reflowing then we
 // need to look at an alternative way to do this.
 _firefox_ReaderMode.showContent();
 _firefox_ReaderMode.updateImageMargins();

I *think* this is correct but I am not a layout expert. Maybe there is a better way to do this? I am wondering if all the things we do programatically in updateImageMargins can't be handled with CSS instead.

https://github.com/mozilla/firefox-ios/blob/a2104fa7211b7cebcd1aa52167f3239db12ca791/Client/Frontend/Reader/ReaderMode.js#L137

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8554431 [details] [diff] [review]
Implement reader mode content CSS for desktop

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

(In reply to :Margaret Leibovic from comment #6)
> bnicholson, do you remember how we originally came up with some of the
> selectors for the Android styles? It seems a bit odd that we have what seems
> like wordpress-specific styles in there.

I never really touched the style parts of reader mode, but my guess is that Lucas ran the parser on different pages and tweaked things that needed fixing. Since WordPress apparently doesn't use <figure> or <figcaption> elements, I suppose it makes sense to have the WP-specific selectors since WP is so common.

Comment 9

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

Comment on attachment 8554431 [details] [diff] [review]
Implement reader mode content CSS for desktop

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

r+ if you remove the `transition-*` properties from the CSS  until we have a UI switch for the color schemes (bug 1120735).

Can you please file a bug to move toolkit/themes/windows/global/aboutReader.css to toolkit/themes/shared ? We shouldn't be storing shared theme files like this in /windows. The bug can also cover about.css, aboutCache.css, aboutCacheEntry.css, aboutMemory.css, aboutSupport.css, and appPicker.css.

::: toolkit/themes/windows/global/aboutReader.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +html {
> +  -moz-text-size-adjust: none;

This value only applies on mobile devices. It shouldn't be necessary to clear it on desktop.

@@ +11,5 @@
> +  max-width: 660px;
> +  margin-left: auto;
> +  margin-right: auto;
> +  transition-property: background-color, color;
> +  transition-duration: 0.4s;

This creates an awkward flash when the color_scheme is set to 'dark' upon loading the page. The page probably defaults to a white background, and then when the 'dark' class is applied the transition takes hold.

We should probably only set these transition properties after the page has finished loading so that we don't have the flash of different colors during loading.

Comment 10

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)

> Can you please file a bug to move
> toolkit/themes/windows/global/aboutReader.css to toolkit/themes/shared ? We
> shouldn't be storing shared theme files like this in /windows. The bug can
> also cover about.css, aboutCache.css, aboutCacheEntry.css, aboutMemory.css,
> aboutSupport.css, and appPicker.css.

Filed bug 1127234.

> @@ +11,5 @@
> > +  max-width: 660px;
> > +  margin-left: auto;
> > +  margin-right: auto;
> > +  transition-property: background-color, color;
> > +  transition-duration: 0.4s;
> 
> This creates an awkward flash when the color_scheme is set to 'dark' upon
> loading the page. The page probably defaults to a white background, and then
> when the 'dark' class is applied the transition takes hold.
> 
> We should probably only set these transition properties after the page has
> finished loading so that we don't have the flash of different colors during
> loading.

Good call. This is probably a bug on Android, too :/

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/8798baba7a29

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8798baba7a29

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8554431 [details] [diff] [review]
Implement reader mode content CSS for desktop

I don't have much feedback. I made note of jared's comment about content flashing when loaded. We have the same issue on iOS. Filed bug 1127846 there.

Comment 14

[Alfred Kayser]

Why is the background for print not exactly white?
.print {
  color: #333333;
  background-color: #fff1df;
}

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Alfred Kayser from comment #14)
> Why is the background for print not exactly white?
> .print {
>   color: #333333;
>   background-color: #fff1df;
> }

I don't know. Is this print mode actually to print pages or is it supposed to be an 'easier for the eyes' kind of mode like Sepia in other readers?

 S.

Comment 16

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

(In reply to Stefan Arentz [:st3fan] from comment #15)
> (In reply to Alfred Kayser from comment #14)
> > Why is the background for print not exactly white?
> > .print {
> >   color: #333333;
> >   background-color: #fff1df;
> > }
> 
> I don't know. Is this print mode actually to print pages or is it supposed
> to be an 'easier for the eyes' kind of mode like Sepia in other readers?

I believe it is supposed to match a Sepia-style 'easier for the eyes' mode. In my experience, most printers will default to not printing background color unless explicitly requested.

Comment 17

[:Margaret Leibovic]

I filed bug 1128724 to address the confusion around the "Print" theme. I also find that confusing.

Comment 18

[Andrei Vaida [:avaida]]

Margaret, is there anything manual QA should look at here?

Comment 19

[:Margaret Leibovic]

(In reply to Andrei Vaida, QA [:avaida] from comment #18)
> Margaret, is there anything manual QA should look at here?

Nope, this bug just added the styles for the reader view content on desktop. So as long as the text is styled, we're good :)

Comment 20

[Jonathan Allard]

Why is my font preference (sans-serif) changed in the print media? It's not very convenient.