Closed Bug 1151200 Opened 9 years ago Closed 8 years ago

User should be able to set margins widths in Reader mode

Categories

(Toolkit :: Reader Mode, defect)

38 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

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

People

(Reporter: klint, Assigned: danhuang)

References

Details

(Whiteboard: [qx:spec])

Attachments

(8 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150330154247

Steps to reproduce:

I displayed a page in Reader Mode


Actual results:

Empty margins were quite large and I was not able to update their width.


Expected results:

There could be a way to configure margins as one can set the size of the font and the color theme.
Component: Untriaged → Reading List
Please introduce option to format text in Reader Mode using full actual browser window width.
An optional multi-column layout e.g. with sideward scrolling could indeed be an improvement. (Desktop users, wider screens etc.)
There should be either user-selectable margin width setting in Preferences, or margins, not text column, should be of fixed width. Right now, the fixed width of text makes this mode absolutely unusable for reading (actually, it is anti-reader view, as often text width on the original page is much more comfortable for reading than this dedicated reading mode) and it is looking like a bug.
I agree with the original poster. Reader mode as is might be according to latest research guidelines but it's not usable for me in real life. Line width is much too narrow and should at least be configurable. Multi-column text might be feasible when keeping current line width but transforming web articles in multi-column text with graphics and all should be a pita hardly a sane alternative.
We used to have a margin control in the original Firefox for Android implementation of this feature.

I'm not sure who the UX contact for this feature is on desktop. Philipp, has anyone on the UX team been thinking about reader view lately?
Status: UNCONFIRMED → NEW
Component: Reading List → Reader Mode
Ever confirmed: true
Flags: needinfo?(philipp)
Product: Firefox → Toolkit
(In reply to :Margaret Leibovic from comment #5)
> We used to have a margin control in the original Firefox for Android
> implementation of this feature.
> 
> I'm not sure who the UX contact for this feature is on desktop. Philipp, has
> anyone on the UX team been thinking about reader view lately?

Margin controls make sense -- most other readers have it as well.
The person who was most involved in RV was mmaslaney, but since he's not here anymore, perhaps Bram can have a look.
Flags: needinfo?(philipp) → needinfo?(bram)
I’ve had a look at both the Android and desktop versions of the reader mode controls. They both lack paragraph width control.

In addition to width, I’d like to also propose a control for leading (line-height). This will make our reader mode even more customisable, and – I’m guessing – won’t be that much harder to implement.

A mockup of the new set of controls is attached.

What do you think?
Flags: needinfo?(bram)
Assignee: nobody → bram
The mock up looks good. When will this be updated?
Flags: needinfo?(bram)
That UI looks great
For Android implementation, :Margaret would be the person to ask. For desktop, :phlsa is the person.

Re: multi-column layout (comment 2) – while splitting the text may make it easier to read, at the end of the first column you would need to scroll all the way back up again to start reading the second column. This isn’t a good experience, especially if the text is long. I’m also guessing that multi-column is harder to implement than single.
Flags: needinfo?(bram)
We should file a separate bug if we want to do this for Android. Our controls look different, so antlam would be the person to ask about that.
Flags: needinfo?(alam)
Thanks for the NI! I have seen this in other applications too.

Yes, our controls are a little bit different on Android and iOS. Given that, I'm also not entirely sure how useful margin controls would be on Mobile since we just use the device width. Perhaps this is something more applicable to larger tablets?

Either way, I've filed bug 1252194 to keep track of this.
Flags: needinfo?(alam)
Is this request being worked upon for the desktops? Will this be available in the next Firefox update?
Flags: needinfo?(philipp)
The design is finished, but it hasn't been implemented yet.
This would probably be a great bug for a new contributor to work on, so I'm marking it as such.

alok, you can look at the status of this bug (currently NEW) to track how far away it is. It will likely first become »ASSIGNED« (being in active development) and eventually »RESOLVED«. That's when it's fully implemented and first available in pre-release builds of Firefox.
Blocks: fx-qx
Flags: needinfo?(philipp)
Whiteboard: [qx:spec]
Attached image reader-width.png
The width can be adjusted by changing the value of the "max-width" property for  the "#container" class. Its default value is "30em". See the attached screenshot where I updated the value to "50em".
Hi Bram,

I'm interested in this bug. Is anything I can help here?
Flags: needinfo?(bram)
Flags: needinfo?(bram)
(In reply to Dan Huang[:danhuang] from comment #16)
> I'm interested in this bug. Is anything I can help here?

I’ve uploaded icon assets for adjusting width and line-height.

The way we implement font sizes in Reader Mode is through having 9 different levels.

These levels are (in px): 12, 14, 16, 18 (default), 20, 22, 24, 26, 28.

With this in mind, we may want to use the same level-based approach for adjusting width and line-height values.

Principles I followed:
* Each increment should feel ‘just right’. Not too small so that you can adjust quickly to get to what you want, but not too large so that you miss the in-between values you might want.
* Have a sensible minimum and maximum value, so that the layout don’t break. For example, if the value is too low, images will turn into thumbnail size and I can barely read a line of text. And if the value is too high, the article width might exceed the window width.

What that in mind, I came up with a rough proposal:

* line-height (in em): 1, 1.2, 1.4, 1.6 (default), 1.8, 2, 2.2, 2.4, 2.6
* width (in em): 20, 25, 30 (default), 35, 40, 45, 50, 55, 60

Other than this, you might like to contact the module owner (I’m not sure who it is) to talk about implementation details.
(In reply to Bram Pitoyo [:bram] from comment #21)
> (In reply to Dan Huang[:danhuang] from comment #16)
> > I'm interested in this bug. Is anything I can help here?
> 
> I’ve uploaded icon assets for adjusting width and line-height.
> 
> The way we implement font sizes in Reader Mode is through having 9 different
> levels.
> 
> These levels are (in px): 12, 14, 16, 18 (default), 20, 22, 24, 26, 28.
> 
> With this in mind, we may want to use the same level-based approach for
> adjusting width and line-height values.
> 
> Principles I followed:
> * Each increment should feel ‘just right’. Not too small so that you can
> adjust quickly to get to what you want, but not too large so that you miss
> the in-between values you might want.
> * Have a sensible minimum and maximum value, so that the layout don’t break.
> For example, if the value is too low, images will turn into thumbnail size
> and I can barely read a line of text. And if the value is too high, the
> article width might exceed the window width.
> 
> What that in mind, I came up with a rough proposal:
> 
> * line-height (in em): 1, 1.2, 1.4, 1.6 (default), 1.8, 2, 2.2, 2.4, 2.6
> * width (in em): 20, 25, 30 (default), 35, 40, 45, 50, 55, 60
> 
> Other than this, you might like to contact the module owner (I’m not sure
> who it is) to talk about implementation details.

Thanks for your replay. It's really helpful! I will follow the suggestion to implement.
Hi Stephen,

I guess you are the right person to review patch from Philipp's bugzilla title.

But If I'm wrong, please tell me who is the right person to go. And it's really sorry for bothering you here.
Attachment #8747517 - Flags: feedback?(shorlander)
Attached image Screen shot for patch part1 (obsolete) —
Hi Dan,

Yes. Stephen is the right person to review the visual design of this work.

One thing I noticed in your screenshot is the fact that the icon isn’t scaled properly. Compare it to the one I took in attachment 8715640 [details], and you’ll see that the thickness of the arrows and the paragraph lines matches the plus and minus symbols exactly.

This may be because the SVG was designed to fit inside a background-size value of 18x18 pixels. My icons are larger than this, so you’ll have to specify its exact dimension in order for it to fit well.

Is this something you can do? If not, let me know, and we’ll work something out! I’ll most likely redesign the icons to fit within the 18x18 dimension.

Let’s hold off on the feedback until this issue has been resolved.

Thanks!
Flags: needinfo?(dhuang)
Comment on attachment 8747517 [details] [diff] [review]
patch: part 1 - add line height control to reader mode

Removing the feedback for now, until the icon size problem is resolved.
Attachment #8747517 - Flags: feedback?(shorlander)
Hi Brams,

Thanks for your feedback.
I could update the background-size style for scaling the line-height icon properly.
This screen shot is the updated reader mode control after adjusting the background size.

Do you think this style update make the icon scale properly ?
Attachment #8747518 - Attachment is obsolete: true
Flags: needinfo?(dhuang) → needinfo?(bram)
This looks great! Thanks for making such a quick revision.

One question: this bug is about setting width, not line-height. Should we open a new bug for you to submit patch for line-height, and then you can submit a separate patch here for adjusting width? I’m guessing that you’ll want to submit two separate patches, but I thought I’d check.
Flags: needinfo?(bram)
Flags: needinfo?(dhuang)
That would be great if you can create another bug for adjusting line height.
Flags: needinfo?(dhuang)
Attached patch Patch v1Splinter Review
Attachment #8747517 - Attachment is obsolete: true
Attachment #8747581 - Attachment is obsolete: true
Hi Bram,

This screen shot is take from the attachment 8747662 [details] [diff] [review](content width control).
Could you take a look to see if the icon scaled properly as the attachment 8715640 [details]? 

Thanks.
Flags: needinfo?(bram)
(In reply to Dan Huang[:danhuang] from comment #31)
> Created attachment 8747663 [details]
> Screen shot for patch v1

Hi Dan, the arrow icons do scale properly.

And I’ll file a bug for line-height, too.
Flags: needinfo?(bram)
Filed the bug for adjusting line-height at bug 1269521.
Assignee: bram → dhuang
Comment on attachment 8747662 [details] [diff] [review]
Patch v1

Hi Stephen,

Please help feedback this patch.
This patch is used to adjust content width in reader mode.
The content width control has 9 different levels which base on Comment 21.
Attachment #8747662 - 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 #8747662 - Attachment is patch: true
Flags: needinfo?(jaws)
Attachment #8747662 - Flags: feedback?(jaws)
Attachment #8747662 - Flags: feedback?(jaws) → review+
It seems like we are good to go here.

One thing we have to do is add the features you’ve implemented to the Firefox release notes when it lands on release (https://www.mozilla.org/en-US/firefox/releases/). Do you know who we should contact about this? If you don’t, I can find out.
Flags: needinfo?(dhuang)
Hi Bram,

This is my first patch going to lands on release. 
So I'm not sure what is the next step to do.
It would be appreciate if you could help me to find out who should contact with.
Flags: needinfo?(dhuang) → needinfo?(bram)
Attachment #8747662 - Flags: feedback?(shorlander)
(In reply to Dan Huang[:danhuang] from comment #37)
> Hi Bram,
> 
> This is my first patch going to lands on release. 
> So I'm not sure what is the next step to do.
> It would be appreciate if you could help me to find out who should contact
> with.

Cameron McCormack has directed me to Ritu Kothari as a person to contact, so I’ll make sure to do that when this patch has been checked in.
Flags: needinfo?(bram)
Keywords: checkin-needed
(In reply to Bram Pitoyo [:bram] from comment #38)
> (In reply to Dan Huang[:danhuang] from comment #37)
> > Hi Bram,
> > 
> > This is my first patch going to lands on release. 
> > So I'm not sure what is the next step to do.
> > It would be appreciate if you could help me to find out who should contact
> > with.
> 
> Cameron McCormack has directed me to Ritu Kothari as a person to contact, so
> I’ll make sure to do that when this patch has been checked in.

Thanks for your help. It's glad to work with you.
Release Note Request (optional, but appreciated)
[Why is this notable]: Reader Mode is more customisable than ever (the other customisation being bug 1269521)
[Suggested wording]: Adjust the width of Reader Mode text
[Links (documentation, blog post, etc)]: n/a
relnote-firefox: --- → ?
https://hg.mozilla.org/mozilla-central/rev/1cc3611ab83d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I might be a little late, but the variable naming is not the best:

https://hg.mozilla.org/mozilla-central/rev/1cc3611ab83d#l2.47
> currentLineHeight = Math.max(CONTENT_WIDTH_MIN, Math.min(CONTENT_WIDTH_MAX, currentLineHeight));

You use a variable called "currentLineHeight" when you are in fact adjusting the content width. A better variable name would be "currentContentWidth".
(In reply to Rares Vernica from comment #43)
> I might be a little late, but the variable naming is not the best:
> 
> https://hg.mozilla.org/mozilla-central/rev/1cc3611ab83d#l2.47
> > currentLineHeight = Math.max(CONTENT_WIDTH_MIN, Math.min(CONTENT_WIDTH_MAX, currentLineHeight));
> 
> You use a variable called "currentLineHeight" when you are in fact adjusting
> the content width. A better variable name would be "currentContentWidth".

Thanks for catching that. I filed bug 1271742 to get that fixed.
Combining release note with one for bug 1269521:  Customize margin widths and line height for Reader mode
Flags: qe-verify+
Verfied fixed on Firefox 49.0 beta 7, build ID 20160825132718.
Tested on Windows 10x64, Ubuntu 12.04x86 and Mac OS X 10.9.5
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.