1151200 - User should be able to set margins widths in Reader mode

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[klint]

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

[Petr Vones]

Please introduce option to format text in Reader Mode using full actual browser window width.

Comment 2

[Arnd]

An optional multi-column layout e.g. with sideward scrolling could indeed be an improvement. (Desktop users, wider screens etc.)

Comment 3

[Yuriy Zakalyuzhnyy]

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

[beingalink]

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

[:Margaret Leibovic]

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?

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 7

[Bram Pitoyo [:bram]]

Attachment: reader mode - controls - i01

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?

Comment 8

[alok]

The mock up looks good. When will this be updated?

Comment 9

[bayners123]

That UI looks great

Comment 10

[Bram Pitoyo [:bram]]

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.

Comment 11

[:Margaret Leibovic]

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.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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.

Comment 13

[alok]

Is this request being worked upon for the desktops? Will this be available in the next Firefox update?

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

[Rares Vernica]

Attachment: 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".

Comment 16

[Dan Huang[:danhuang]]

Hi Bram,

I'm interested in this bug. Is anything I can help here?

Comment 17

[Bram Pitoyo [:bram]]

Attachment: increase-line-height-icon.svg

Comment 18

[Bram Pitoyo [:bram]]

Attachment: decrease-line-height-icon.svg

Comment 19

[Bram Pitoyo [:bram]]

Attachment: increase-width-icon.svg

Comment 20

[Bram Pitoyo [:bram]]

Attachment: decrease-width-icon.svg

Comment 21

[Bram Pitoyo [: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.

Comment 22

[Dan Huang[:danhuang]]

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

Comment 23

[Dan Huang[:danhuang]]

Attachment: patch: part 1 - add line height control to reader mode

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.

Comment 24

[Dan Huang[:danhuang]]

Attachment: Screen shot for patch part1

Comment 25

[Bram Pitoyo [:bram]]

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!

Comment 26

[Bram Pitoyo [:bram]]

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.

Comment 27

[Dan Huang[:danhuang]]

Attachment: Screen Shot for adjusting line height control icon

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 ?

Comment 28

[Bram Pitoyo [: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.

Comment 29

[Dan Huang[:danhuang]]

That would be great if you can create another bug for adjusting line height.

Comment 30

[Dan Huang[:danhuang]]

Attachment: Patch v1

Comment 31

[Dan Huang[:danhuang]]

Attachment: Screen shot for patch v1

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.

Comment 32

[Bram Pitoyo [: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.

Comment 33

[Bram Pitoyo [:bram]]

Filed the bug for adjusting line-height at bug 1269521.

Comment 34

[Dan Huang[:danhuang]]

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.

Comment 35

[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 36

[Bram Pitoyo [:bram]]

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.

Comment 37

[Dan Huang[:danhuang]]

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.

Comment 38

[Bram Pitoyo [:bram]]

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

Comment 39

[Dan Huang[:danhuang]]

(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

[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 1269521)
[Suggested wording]: Adjust the width of Reader Mode text
[Links (documentation, blog post, etc)]: n/a

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1cc3611ab83d

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cc3611ab83d

Comment 43

[Rares Vernica]

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

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

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

Comment 45

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Combining release note with one for bug 1269521:  Customize margin widths and line height for Reader mode

Comment 47

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

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