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.

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.

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

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

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

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?

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.

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?

Yes. Thanks a lot for your work on this!

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

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

Added to relnotes for 49 along with bug 1151200.

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