Closed Bug 862445 Opened 8 years ago Closed 8 years ago

Adjust reader mode content layout for serif fonts

Categories

(Firefox for Android :: Reader View, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: Margaret, Assigned: capella)

References

Details

(Whiteboard: ui-hackathon, reader)

Attachments

(5 files, 3 obsolete files)

Follow-up to bug 857989 comment 5.
Flags: needinfo?(ibarlow)
Whiteboard: ui-hackathon
Attached image 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
Flags: needinfo?(ibarlow)
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?
Attachment #742426 - Flags: feedback?(lucasr.at.mozilla)
Assignee: nobody → markcapella
Attached image Mixed Serif
Attached image All Sans
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".
Attachment #742426 - Flags: feedback+
Attached patch Patch (v2) (obsolete) — Splinter Review
Thanks Margaret!
Attachment #742668 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Attachment #742426 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #742668 - Flags: review?(lucasr.at.mozilla) → review-
(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.
Attached patch Patch (v3) (obsolete) — Splinter Review
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
Attachment #742668 - Attachment is obsolete: true
Attachment #743280 - Flags: feedback?(ibarlow)
Hi Mark, couldn't get this build to install. :(

Try uploading again?
Odd, loaded and instakked onto my S3 ...

Let;s try to provide it through TRY server
https://tbpl.mozilla.org/?tree=Try&rev=37de5c09c69e
Whiteboard: ui-hackathon → ui-hackathon, reader
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.
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 ...
Flags: needinfo?(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.
Flags: needinfo?(ibarlow)
Attached patch Patch (v4) (obsolete) — Splinter Review
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)
Attachment #743280 - Attachment is obsolete: true
Attachment #743280 - Flags: feedback?(ibarlow)
Flags: needinfo?
Mark, do you have a build I can try?
Flags: needinfo?
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 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;
}
Attachment #753226 - Flags: review-
Attached patch Patch (v5)Splinter Review
Coooool ....

This addresses those last couple nits ... pushed to TRY looking good also: https://tbpl.mozilla.org/?tree=Try&rev=81b3e9d004b2
Attachment #753226 - Attachment is obsolete: true
Attachment #753768 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 753768 [details] [diff] [review]
Patch (v5)

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

Looks good.
Attachment #753768 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/ebb7f64f278e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.