Closed Bug 766948 Opened 8 years ago Closed 8 years ago

Refine content type styling in reader mode

Categories

(Firefox for Android :: Reader View, defect)

15 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox16 --- verified
firefox17 --- verified

People

(Reporter: ibarlow, Assigned: lucasr)

References

Details

Attachments

(2 files)

In our current Reader Mode implementation, it looks like we apply styling to two distinct content types:

* Title
* Text

We should look at how we can continue to refine our Reader Mode so that it can identify and style more kinds of content so it looks more polished:

* Author / Date (see grey text in this mockup http://cl.ly/232n3c3M3L2i2W0T2u3M)
* Photo Captions
* Quoted Text
Ian, quoted text refinement is covered in bug 766950, no?
Attached is a more complete set of design specs for Reader Mode styles. 

Included are styles for:

* top level domain (see notes below)
* Title
* Author / Date
* Captions
* Lists (unordered and ordered)
* Quotes
* Image placement

In addition, I have taken another pass at cleaning up the layout in other ways too:

* Removing the orange horizontal divider -- after using it for a while it became clear that the design was too severe and not very sophisticated
* Adjusting the styling of the title to be more magazine-like and modern
* Adding the top level domain from which the article originated -- a detail I found myself missing when using our current implementation. This will become even more useful in providing context for an article, when we enable navigating from article to article without having to return to a reading list. 
* Removing underlines from links -- colour should suffice here.
* Removing the paper texture from the background -- it simply didn't look good consistently across different kinds of devices. Let's fall back to flat colours for now.
* Removing the Sepia mode, for the same reason as removing the paper texture. On some phones it looked nice, on others it just looked jaundiced and dated.
Assignee: nobody → lucasr.at.mozilla
Uh oh. I just found out from Patryk that Open Sans has some restrictions in their license agreement that does not allow it to be distributed on a project like B2G. 

I am checking with Legal to see if this applies to our reader mode, too.
False alarm, we are good to use Open. As you were! http://cl.ly/image/2i3h472e2Z04
Assignee: lucasr.at.mozilla → nobody
Component: General → Reader Mode
Attachment #651464 - Flags: review?
Ian, a few notes for you:
- There's no way to fully implement the style for lists due to CSS limitations (i.e. you can't style the bullets and numbers separately from the text in each item)
- The edge-to-edge style will only apply to images following the certain patterns (using figure tag, wp-caption class, etc). Same for caption text.
- We don't fetch the byline content from articles yet (filed bug 782348 to track this) but I've already implemented the style here.
Assignee: nobody → lucasr.at.mozilla
Hi everyone,

First of all I would like to thank you for this amazing feature! I love it. 

I've been playing a bit with it and I would like to give you some feedback regarding Ian's mockup which is impressive. I've a phone with an AMOLED display so I usually choose a black background as it will look awesome and it will save battery. I know that only a few devices actually support this type of screens, but I was wondering if in the future the users could pick their own colors for the background, or just pick one from a given list as it is right now. What do you think, is even worthy to allow users to customize their own reading experience?

Cheers,

Daniel
Comment on attachment 651464 [details] [diff] [review]
Implement new Reader style


>diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js

>+  _updateImageMargins: function Reader_updateImageMargins() {

>+    let imagesSelector = ".content div > img, .content .wp-caption img, .content figure img";

This seems like something we would want to define as a CONST or commented well ? So we know we can update it as other CSS classes or images need to be handled. It's a bit buried here.

>diff --git a/mobile/android/themes/core/aboutReader.css b/mobile/android/themes/core/aboutReader.css

>+@font-face {
>+  font-family: 'OpenSansLight';
>+  src: url('chrome://browser/skin/fonts/opensans-light.ttf') format('truetype');
>+  font-weight: normal;
>+  font-style: normal;
>+}


>+.font-size1 > .content .wp-caption-text,
>+.font-size1 > .content figcaption,
>+.font-size1 > .header > .domain,
>+.font-size1 > .header > .credits {
>+  font-size: 6px;
>+}

Comments for these special classes would be nice

>diff --git a/mobile/android/themes/core/jar.mn b/mobile/android/themes/core/jar.mn

>   skin/fonts/opensans-regular.ttf           (fonts/opensans-regular.ttf)
>+  skin/fonts/opensans-light.ttf             (fonts/opensans-light.ttf)

Oh, a new font! Shouldn't we add this to content.css like the regular font?

r+ but fix the nits
Attachment #651464 - Flags: review? → review+
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 651464 [details] [diff] [review]
> Implement new Reader style
> 
> 
> >diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js
> 
> >+  _updateImageMargins: function Reader_updateImageMargins() {
> 
> >+    let imagesSelector = ".content div > img, .content .wp-caption img, .content figure img";
> 
> This seems like something we would want to define as a CONST or commented
> well ? So we know we can update it as other CSS classes or images need to be
> handled. It's a bit buried here.

Good point. Done.
 
> >diff --git a/mobile/android/themes/core/aboutReader.css b/mobile/android/themes/core/aboutReader.css
> 
> >+@font-face {
> >+  font-family: 'OpenSansLight';
> >+  src: url('chrome://browser/skin/fonts/opensans-light.ttf') format('truetype');
> >+  font-weight: normal;
> >+  font-style: normal;
> >+}
> 
> 
> >+.font-size1 > .content .wp-caption-text,
> >+.font-size1 > .content figcaption,
> >+.font-size1 > .header > .domain,
> >+.font-size1 > .header > .credits {
> >+  font-size: 6px;
> >+}
> 
> Comments for these special classes would be nice

Added.

> >diff --git a/mobile/android/themes/core/jar.mn b/mobile/android/themes/core/jar.mn
> 
> >   skin/fonts/opensans-regular.ttf           (fonts/opensans-regular.ttf)
> >+  skin/fonts/opensans-light.ttf             (fonts/opensans-light.ttf)
> 
> Oh, a new font! Shouldn't we add this to content.css like the regular font?

This will be done in Brian's patch in that security bug so I'm leaving this as is for now.
https://hg.mozilla.org/mozilla-central/rev/45b6b015f792
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Hi Daniel,

Sorry, I missed your comment somehow.

(In reply to Daniel Lombraña González from comment #7)
> First of all I would like to thank you for this amazing feature! I love it. 

Glad you're enjoying it, thanks!
 
> I've been playing a bit with it and I would like to give you some feedback
> regarding Ian's mockup which is impressive. I've a phone with an AMOLED
> display so I usually choose a black background as it will look awesome and
> it will save battery. I know that only a few devices actually support this
> type of screens, but I was wondering if in the future the users could pick
> their own colors for the background, or just pick one from a given list as
> it is right now. What do you think, is even worthy to allow users to
> customize their own reading experience?

I personally have a couple reasons to prefer the current setup with just two pre-defined colour schemes:

1. Allowing users to change background with arbitrary colours would probably add an unwanted complexity to the UX. We better just provide good default options which will cover the great majority of our users.
2. If you can change background to any colour, then we need to allow setting text colour as well. But then you need to be able to tweak the link colour as well. In other words, it wouldn't be just about setting the background colour.

I guess you'll be happy to know that the "dark" colour scheme uses plain black colour as background now? :-)
(In reply to Lucas Rocha (:lucasr) from comment #12)
> Hi Daniel,
> 
> Sorry, I missed your comment somehow.

Don't worry :-)

> 
> (In reply to Daniel Lombraña González from comment #7)
> > First of all I would like to thank you for this amazing feature! I love it. 
> 
> Glad you're enjoying it, thanks!

You are welcome!

>  
> > I've been playing a bit with it and I would like to give you some feedback
> > regarding Ian's mockup which is impressive. I've a phone with an AMOLED
> > display so I usually choose a black background as it will look awesome and
> > it will save battery. I know that only a few devices actually support this
> > type of screens, but I was wondering if in the future the users could pick
> > their own colors for the background, or just pick one from a given list as
> > it is right now. What do you think, is even worthy to allow users to
> > customize their own reading experience?
> 
> I personally have a couple reasons to prefer the current setup with just two
> pre-defined colour schemes:
> 
> 1. Allowing users to change background with arbitrary colours would probably
> add an unwanted complexity to the UX. We better just provide good default
> options which will cover the great majority of our users.
> 2. If you can change background to any colour, then we need to allow setting
> text colour as well. But then you need to be able to tweak the link colour
> as well. In other words, it wouldn't be just about setting the background
> colour.
> 
> I guess you'll be happy to know that the "dark" colour scheme uses plain
> black colour as background now? :-)

Thanks a lot. After reading your reply I completely understand that allowing people to use custom colors will be a mess :-D I'll try the new version today or tomorrow!

Cheers, and thanks for your good work!!

Daniel
Comment on attachment 651464 [details] [diff] [review]
Implement new Reader style

[Approval Request Comment]
User impact if declined: Final implementation of reader mode's content design. Reader mode is in Firefox 16 roadmap.
Testing completed (on m-c, etc.): A couple days in m-c, no regressions.
Risk to taking this patch (and alternatives if risky): We don't want to ship reader mode without the planned design.
String or UUID changes made by this patch: None.
Attachment #651464 - Flags: approval-mozilla-aurora?
Attachment #651464 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fix on: Firefox Mobile 16 beta 2, Nightly 18.0a1 2012-09-04 and Aurora 17.0a2 2012-09-05 on Samsung Galaxy Tab 2 7.0 (Android 4.0.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.