1269521 - User should be able to set line height in Reader mode

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Bram Pitoyo [:bram]]

Based on the UI work done on bug 1151200, we’d like to give Reader Mode users the ability to tweak the article’s line-height value.

See attachment 8715640 [details] for mockup.

See attachment 8746343 [details] and attachment 8746344 [details] for icon assets.

Refer to bug 1151200 comment 21 for a proposal of the line-height values to use.

Comment 1

[Dan Huang[:danhuang]]

Attachment: Patch v1

Hi Stephen,

Please help feedback this patch.
This patch is used to adjust content's line height in reader mode.
The line height control has 9 different levels which base on bug 1151200 comment 21.

Comment 2

[Dan Huang[:danhuang]]

Attachment: Screen shot for patch v1

Comment 3

[Bram Pitoyo [:bram]]

Paging :jaws for patch feedback. Are you the right person to talk to about this? If not, do you know of who to ask?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8748048 [details] [diff] [review]
Patch v1

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

::: toolkit/components/reader/AboutReader.jsm
@@ +346,5 @@
> +    contentClasses.add("line-height" + this._lineHeight);
> +    return AsyncPrefs.set("reader.line_height", this._lineHeight);
> +  },
> +
> +  _setupLintHeightButtons: function() {

typo, this should be _setupLineHeightButtons

::: toolkit/themes/shared/reader/RM-Line-Height-Plus-38x24.svg
@@ +14,5 @@
> +     fill="#808080">
> +
> +  <rect id="-" x="0" y="0" width="28" height="2"/>
> +  <rect id="-" x="0" y="11" width="38" height="2"/>
> +  <rect id="-" x="0" y="22" width="18" height="2"/>

Please remove the id attributes on these as they're not needed and may cause confusion.

Comment 5

[Bram Pitoyo [:bram]]

Dan, can you remove the id attributes from the files? If not, I can fix them and reupload them for you.

Comment 6

[Dan Huang[:danhuang]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Comment on attachment 8748048 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8748048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/reader/AboutReader.jsm
> @@ +346,5 @@
> > +    contentClasses.add("line-height" + this._lineHeight);
> > +    return AsyncPrefs.set("reader.line_height", this._lineHeight);
> > +  },
> > +
> > +  _setupLintHeightButtons: function() {
> 
> typo, this should be _setupLineHeightButtons
> 
> ::: toolkit/themes/shared/reader/RM-Line-Height-Plus-38x24.svg
> @@ +14,5 @@
> > +     fill="#808080">
> > +
> > +  <rect id="-" x="0" y="0" width="28" height="2"/>
> > +  <rect id="-" x="0" y="11" width="38" height="2"/>
> > +  <rect id="-" x="0" y="22" width="18" height="2"/>
> 
> Please remove the id attributes on these as they're not needed and may cause
> confusion.

Thanks for the review, I would update this nits in next patch.

Comment 7

[Dan Huang[:danhuang]]

Attachment: Patch v2

Hi Bram,

This patch fix the nits(fix typo and remove id attribute) which mentioned in Comment 4.
Do you think we need to send a review request to :jaws for this patch again?

Comment 8

[Bram Pitoyo [:bram]]

No, we don’t need to send another review request.

But would you make sure that the rest of the SVG files also has no id attribute? I’m worried that this file isn’t the only one, but I might be wrong.

When that’s good to go, I think this patch can land.

Comment 9

[Dan Huang[:danhuang]]

I had checked the rest of the svg files under /themes/shared/reader. 
There is only RM-Close-24x24.svg contains the id attribute, and this attribute is needed to style tag.

So next step to load this patch is to add the checkin-needed keyword to the bug?

Comment 10

[Bram Pitoyo [:bram]]

Yes. Thanks a lot for your work on this!

Comment 11

[Bram Pitoyo [:bram]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Reader Mode is more customisable than ever (the other customisation being bug 1151200)
[Suggested wording]: Adjust the line spacing of Reader Mode text
[Links (documentation, blog post, etc)]: n/a

Comment 12

[Carsten Book [:Tomcat]]

has problems to apply:

Hunk #6 FAILED at 297
Hunk #7 succeeded at 361 with fuzz 1 (offset 20 lines).
6 out of 7 hunks FAILED -- saving rejects to file toolkit/themes/shared/aboutReaderControls.css.rej
patching file toolkit/themes/shared/jar.inc.mn
Hunk #1 FAILED at 51
1 out of 1 hunks FAILED -- saving rejects to file toolkit/themes/shared/jar.inc.mn.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1269521-Add-line-height-contral-in-reader-mode.-.patch

Comment 13

[Dan Huang[:danhuang]]

Attachment: Patch rebased

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/095afcba84a3

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/095afcba84a3

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Added to relnotes for 49 along with bug 1151200.

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

The user can correctly set the line height in reader mode using Fx 49.0b7 (buildID 20160825132718) across platforms.