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.

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

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.

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

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

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/45b6b015f792

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.

Pushed to mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f8bb07e34140

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)