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)
Tracking
()
VERIFIED
FIXED
mozilla49
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.
Comment 1•9 years ago
|
||
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.)
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
(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)
Comment 7•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → bram
The mock up looks good. When will this be updated?
Flags: needinfo?(bram)
Comment 9•8 years ago
|
||
That UI looks great
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
Is this request being worked upon for the desktops? Will this be available in the next Firefox update?
Flags: needinfo?(philipp)
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
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".
Assignee | ||
Comment 16•8 years ago
|
||
Hi Bram, I'm interested in this bug. Is anything I can help here?
Flags: needinfo?(bram)
Comment 17•8 years ago
|
||
Flags: needinfo?(bram)
Comment 18•8 years ago
|
||
Comment 19•8 years ago
|
||
Comment 20•8 years ago
|
||
Comment 21•8 years ago
|
||
(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.
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Assignee | ||
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(dhuang)
Assignee | ||
Comment 29•8 years ago
|
||
That would be great if you can create another bug for adjusting line height.
Flags: needinfo?(dhuang)
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8747517 -
Attachment is obsolete: true
Attachment #8747581 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
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)
Comment 32•8 years ago
|
||
(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)
Comment 33•8 years ago
|
||
Filed the bug for adjusting line-height at bug 1269521.
Updated•8 years ago
|
Assignee: bram → dhuang
Assignee | ||
Comment 34•8 years ago
|
||
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)
Comment 35•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8747662 -
Attachment is patch: true
Flags: needinfo?(jaws)
Attachment #8747662 -
Flags: feedback?(jaws)
Updated•8 years ago
|
Attachment #8747662 -
Flags: feedback?(jaws) → review+
Comment 36•8 years ago
|
||
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)
Assignee | ||
Comment 37•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8747662 -
Flags: feedback?(shorlander)
Comment 38•8 years ago
|
||
(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
Assignee | ||
Comment 39•8 years ago
|
||
(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.
Comment 40•8 years ago
|
||
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:
--- → ?
Comment 41•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1cc3611ab83d
Keywords: checkin-needed
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1cc3611ab83d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 43•8 years ago
|
||
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".
Comment 44•8 years ago
|
||
(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
Updated•7 years ago
|
Flags: qe-verify+
Comment 47•7 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•