Closed Bug 1269521 Opened 4 years ago Closed 4 years ago

User should be able to set line height in Reader mode

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox49 --- verified
relnote-firefox --- 49+

People

(Reporter: bram, Assigned: danhuang)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → dhuang
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8748048 - Flags: feedback?(shorlander)
Paging :jaws for patch feedback. Are you the right person to talk to about this? If not, do you know of who to ask?
Flags: needinfo?(jaws)
Attachment #8748048 - Attachment is patch: true
Flags: needinfo?(jaws)
Attachment #8748048 - Flags: feedback?(jaws)
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.
Attachment #8748048 - Flags: feedback?(jaws) → review+
Flags: needinfo?(dhuang)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #8748048 - Attachment is obsolete: true
Attachment #8748048 - Flags: feedback?(shorlander)
Flags: needinfo?(dhuang) → needinfo?(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.
Flags: needinfo?(bram) → needinfo?(dhuang)
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?
Flags: needinfo?(dhuang) → needinfo?(bram)
Yes. Thanks a lot for your work on this!
Flags: needinfo?(bram)
Keywords: checkin-needed
Attachment #8750152 - Flags: review+
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
relnote-firefox: --- → ?
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
Flags: needinfo?(dhuang)
Keywords: checkin-needed
Attached patch Patch rebasedSplinter Review
Attachment #8750152 - Attachment is obsolete: true
Flags: needinfo?(dhuang)
Attachment #8750564 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/095afcba84a3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272677
Added to relnotes for 49 along with bug 1151200.
Flags: qe-verify+
The user can correctly set the line height in reader mode using Fx 49.0b7 (buildID 20160825132718) across platforms.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.