Implement CSS styling for desktop about:reader content

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Flags: needinfo?(mmaslaney)
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
Flags: needinfo?(mmaslaney)
Depends on: 1120518
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.
Summary: Implement CSS styling for desktop about:reader → Implement CSS styling for desktop about:reader content
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/mleibovic@mozilla.com-c57b64247945/try-macosx64/firefox-38.0a1.en-US.mac.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mleibovic@mozilla.com-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?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(alam)
Summary: Implement CSS styling for desktop about:reader content → Implement updated CSS styling for about:reader content
Blocks: 1124011
(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
Flags: needinfo?(alam)
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.
Flags: needinfo?(mmaslaney)
Summary: Implement updated CSS styling for about:reader content → Implement CSS styling for desktop about:reader content
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?
Attachment #8554431 - Flags: review?(jaws)
Attachment #8554431 - Flags: feedback?(sarentz)
Attachment #8554431 - Flags: feedback?(bnicholson)
(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 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.
Attachment #8554431 - Flags: feedback?(bnicholson) → feedback+
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.
Attachment #8554431 - Flags: review?(jaws) → review+
Blocks: 1127234
(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 :/
https://hg.mozilla.org/mozilla-central/rev/8798baba7a29
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8554431 - Flags: feedback?(sarentz) → feedback+
Why is the background for print not exactly white?
.print {
  color: #333333;
  background-color: #fff1df;
}
(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.
(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.
I filed bug 1128724 to address the confusion around the "Print" theme. I also find that confusing.
Margaret, is there anything manual QA should look at here?
Flags: qe-verify?
Flags: needinfo?(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 :)
Flags: needinfo?(margaret.leibovic)
Flags: qe-verify? → qe-verify-
Why is my font preference (sans-serif) changed in the print media? It's not very convenient.
You need to log in before you can comment on or make changes to this bug.