862445 - Adjust reader mode content layout for serif fonts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

Follow-up to bug 857989 comment 5.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Reader refinements

Global changes:

* Reduce title size to 28sp
* Change link colour to blue #00acff 
* Make tapping the title link (vanityfair.com) open the original article
* Change horizontal divider to stop in the middle of the screen
* Change horizontal divider colour / size to #777777 and 1dp thick


Serif-specific changes:

* Only apply serif typeface to titles and content. (leave URL, author / date / caption text in Open Sans)
* Make article title bold

Comment 2

[Mark Capella [:capella]]

Attachment: Quick Hack for feedback

Here's a quick put together of everything (I think) ... the only thing I didn't do was with |pressing something to go back something| .... we can already press the back key or click the close-readermode button in the awesomebar to do this ...

So, what am I missing?

Comment 3

[Mark Capella [:capella]]

Attachment: Mixed Serif

Comment 4

[Mark Capella [:capella]]

Attachment: All Sans

Comment 5

[:Margaret Leibovic]

Comment on attachment 742426 [details] [diff] [review]
Quick Hack for feedback

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

Just offering some drive-by feedback...

::: mobile/android/chrome/content/aboutReader.html
@@ +14,5 @@
>      <div id="reader-domain" class="domain"></div>
> +    <div>
> +      <div class="domain-border"></div>
> +    <div>
> +    <br>

I don't really like the fact that we're making another element here just for the border, but I'm not sure how we would otherwise make the border only extend for half of the width of the page. Can we at least try to get rid of this <br> and the <div> that wraps the .domain-border <div>?

::: mobile/android/themes/core/aboutReader.css
@@ +43,5 @@
>  }
>  
> +.domain,
> +.credits {
> +  font-family: "Droid Sans",helvetica,arial,clean,sans-serif;

You should use "Open Sans" instead of "Droid Sans". I'm also not sure we want the other fallbacks besides sans-serif.

@@ +53,5 @@
> +}
> +
> +.domain-border {
> +  border-bottom: 1.5px solid;
> +  border-bottom-color: #777777;

You could combine these two lines into:
border-bottom: 1.5px solid #777777;

@@ +54,5 @@
> +
> +.domain-border {
> +  border-bottom: 1.5px solid;
> +  border-bottom-color: #777777;
> +  float: left;

Is this float necessary?

@@ +60,4 @@
>  }
>  
>  .header > h1 {
> +  font-size: 66px; /* 42px; assume xfactor 1.5 */

We probably want font-size to be in em, if that's what line-height is in.

@@ +252,5 @@
>  /* Image caption text */
>  .content .caption,
>  .content .wp-caption-text,
>  .content figcaption {
> +  font-family: "Droid Sans",helvetica,arial,clean,sans-serif;

Same comment re: "Open Sans".

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

Thanks Margaret!

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 742668 [details] [diff] [review]
Patch (v2)

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

Looks good, just need to keep the title style for sans font untouched I think.

::: mobile/android/themes/core/aboutReader.css
@@ +58,5 @@
>  }
>  
>  .header > h1 {
> +  font-size: 2.625em; /* 28sp assume xfactor 1.5 is 42px / 16 yeilds 2.625em */
> +  font-weight: 700; /* bold */

You should only apply these new properties to the ".header > h1" element when reader is using serif fonts. The style for sans uses Open Sans Light and should probably be kept untouched. See screenshot.

I don't consider these comments specially useful to be honest. I'd probably remove them.

@@ +191,5 @@
>  .light > .content a,
>  .light > .content a:visited,
>  .light > .content a:hover,
>  .light > .content a:active {
> +  color: #00acff !important;  /* blue start w-the same */

Comment not very useful...

@@ +198,5 @@
>  .dark > .content a,
>  .dark > .content a:visited,
>  .dark > .content a:hover,
>  .dark > .content a:active {
> +  color: #00acff !important;  /* blue start w-the same */

Ditto.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Mark Capella [:capella] from comment #3)
> Created attachment 742431 [details]
> Mixed Serif

Couple comments so far Mark:

* Still need to change the link colour to blue
* The article title should be "bold" in the serif view, but "light" in the sans serif view.

Would love to try a build with these changes, when you're ready.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok, this one with the change to article title font weight based on latest requirement clarifications ... not sure whats left with changing links to blue, that was in the last patch ... are we saying the same thing?

Quick test build:
https://www.dropbox.com/s/i5y1g28bkctkdmq/fennec-23.0a1.en-US.android-arm.apk

Comment 10

[Ian Barlow (:ibarlow)]

Hi Mark, couldn't get this build to install. :(

Try uploading again?

Comment 11

[Mark Capella [:capella]]

Odd, loaded and instakked onto my S3 ...

Let;s try to provide it through TRY server
https://tbpl.mozilla.org/?tree=Try&rev=37de5c09c69e

Comment 12

[Ian Barlow (:ibarlow)]

Almost there!

* Still need to change the link colour to blue. This seems to be done in content, but not for the one above the article title, so we need to update that.
* The serif article title needs to be set back to bold. Something must have changed there when you flipped the sans-serif one to be Light.

Would love to try a build with these changes, when you're ready.

Comment 13

[Mark Capella [:capella]]

In comment #12 ibarlow mentions "change the link colour to blue ... the one above the article title" ... that's the (now half page width) underline below the page title?

Can you clarify the story here ... it's not an actual link ... we talked about changing it to be one, but I didn't see the case for it ...

Comment 14

[Ian Barlow (:ibarlow)]

(In reply to Mark Capella [:capella] from comment #13)
> In comment #12 ibarlow mentions "change the link colour to blue ... the one
> above the article title" ... that's the (now half page width) underline
> below the page title?
> 
> Can you clarify the story here ... it's not an actual link ... we talked
> about changing it to be one, but I didn't see the case for it ...

Sorry, something that got lost in the shuffle at some point... this should indeed be a link, and it should load the webpage that the Reader article is based on. 

Not crucial functionality given that there is also the icon in the title bar, but a nice detail for people who try tapping on it.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v4)

Ok then, asking anyone for more feedback to make sure I'm clear now on the req's ... later will ask lucasr? ibarlow? for review ...

Summary of changes as I understand the request (and believe this patch provides)

* Reduce article title size to 28sp
* The article title should be "bold" in the serif view, but "light" in the sans serif view

* Change link colours to blue #00acff

* Make tapping the title *link*/domain open the original article
* Change the title *link*/domain horizontal divider to stop in the middle of the screen, and have colour/size of #777777 and 1dp thick

* Only apply serif typeface to article title and content. (leave URL, author / date / caption text in Open Sans)

Comment 16

[Ian Barlow (:ibarlow)]

Mark, do you have a build I can try?

Comment 17

[Mark Capella [:capella]]

Combined TRY build for bug 874036 and bug 862445 (also bug 840144 if you want to play with that)... 

builds ok but 2.2 M() and R() testing is failing 2400-sec timeouts and I'm not sure why ... I downloaded from TRY and tested Ok... just fyi for now (?)

https://tbpl.mozilla.org/?tree=Try&rev=b095e4b909ca

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 753226 [details] [diff] [review]
Patch (v4)

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

Looks good overall but needs some cleanups on the handling of serif header.

::: mobile/android/chrome/content/aboutReader.js
@@ +375,5 @@
> +    if (this._fontType == "serif") {
> +      titleClasses.add("heavy");
> +    } else {
> +      titleClasses.remove("heavy");
> +    }

No need to do this in javascript. You can accomplish the same with plain CSS.

@@ +522,5 @@
>      this._messageElement.style.display = "none";
>  
>      this._article = article;
>  
> +    this._domainElement.href = article.url;

Tapping on the domain takes to the original article? Nice.

::: mobile/android/themes/core/aboutReader.css
@@ +71,5 @@
>    margin-bottom: 16px;
>    padding: 0px;
>  }
>  
> +.header > h1 .heavy {

Replace the js code for handling 'heavy' with something like:

.serif > .header > h1 {
  font-weight: 700;
}

Comment 19

[Mark Capella [:capella]]

Attachment: Patch (v5)

Coooool ....

This addresses those last couple nits ... pushed to TRY looking good also: https://tbpl.mozilla.org/?tree=Try&rev=81b3e9d004b2

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 753768 [details] [diff] [review]
Patch (v5)

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

Looks good.

Comment 21

[Mark Capella [:capella]]

on to inbound after a bump .... https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb7f64f278e

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ebb7f64f278e