1135593 - The Reader Mode controls sidebar is affected by page zoom actions

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Ubuntu 14.04 (x64)

Reproducible on:
Nightly 38.0a1 (2015-02-22)

Affected platforms:
Windows 7 (x64), Windows 8.1 (x86), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Steps to reproduce:
1. Launch Firefox with a clean profile.
2. Enable Reader Mode from about:config by setting 'reader.parse-on-load.enabled' to true. Restart the browser.
3. Open an article from the web - (e.g.) http://www.bbc.com/news/world-australia-31579804
4. Enter Reader Mode by clicking the associated button from the Location Bar.
5. Zoom in or out from the page and check the sidebar.

Expected result:
The Reader Mode UI is not affected by zoom in/zoom out actions.

Actual result:
Both the sidebar and any opened panels such as the page styling panel are affected by zoom actions.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

On Windows at least, using the panel controls to zoom does not change impact panel, but using "normal" zoom features (eg, hamburger menu, ctrl-+ etc) does.

Comment 2

[Justin Dolske [:Dolske]]

Yeah, this bug is about the normal browser page-zoom controls, not the ones that reader view itself exposes.

I'm not sure there's a non-gross way to fix this. AFAIK, we don't have any way for a page to suppress the browser's zooming.

Same bug exists for pdf.js... If you load a PDF and zoom the page with the hamburger menu controls, its controls change size too. It does seem to grab Control/Command +/-/0, so using the keyboard shortcuts will result in PDF.js zooming just the PDF, and the actual page-zoom level is unchanged.

Maybe grabbing the keyboard shortcuts would be a "good enough" fix for this bug.

Comment 4

[David Lawrence [:dkl]]

Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl

Comment 5

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #2)

Same bug exists for pdf.js... If you load a PDF and zoom the page with the
hamburger menu controls, its controls change size too. It does seem to grab
Control/Command +/-/0, so using the keyboard shortcuts will result in PDF.js
zooming just the PDF, and the actual page-zoom level is unchanged.

Maybe grabbing the keyboard shortcuts would be a "good enough" fix for this
bug.

This should be mentorable. The zoom code lives in the parent process in the browser, and the browser knows whether we're in reader mode, and could just not do what it does by default, instead sending a message to the content browser to increase text size. Probably somewhere here:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/base/content/browser-fullZoom.js#269-287

where we could send a message to reader mode like we do here: https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/base/content/browser.js#5420 .

Reader mode would have to listen for that message, increment/decrement the font size (and maybe the text width?), and we'd need to come up with some feedback in the reader mode sidebar that indicates something changed.

Comment 6

[Berkay Barlas]

I would be really happy to try solve this issue under your mentorship Gijs.

Comment 7

[:Gijs (he/him)]

(In reply to Berkay Barlas from comment #6)

I would be really happy to try solve this issue under your mentorship Gijs.

Sure. Let me know if you have more questions after comment #5 ?

Comment 8

[Berkay Barlas]

(In reply to :Gijs (he/him) from comment #7)

Sure. Let me know if you have more questions after comment #5 ?

I added sendAsyncMessage that sends Reader:ZoomIn and Reader:ZoomOut to Reader in https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/base/content/browser-fullZoom.js#269-287

Also reader mode listens that message and changes font size accordingly, however, I couldn't find a method in tabbrowser.js
that returns whether it is in reader or not.

I found a method isAboutReader() in AboutReaderChild.js,but, I'm not sure how to use it.

Comment 9

[:Gijs (he/him)]

(In reply to Berkay Barlas from comment #8)

(In reply to :Gijs (he/him) from comment #7)

Sure. Let me know if you have more questions after comment #5 ?

I added sendAsyncMessage that sends Reader:ZoomIn and Reader:ZoomOut to Reader in https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/base/content/browser-fullZoom.js#269-287

Also reader mode listens that message and changes font size accordingly, however, I couldn't find a method in tabbrowser.js
that returns whether it is in reader or not.

I found a method isAboutReader() in AboutReaderChild.js,but, I'm not sure how to use it.

You can use the same check as here:

https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/browser/modules/ReaderParent.jsm#66

Comment 10

[Berkay Barlas]

Attachment: Bug 1135593 Make The Reader Mode controls sidebar not affected by page zoom actions

Comment 11

[Berkay Barlas]

Thank you for you suggestion Gijs.
I created a revision and here is a gif that shows current behavior.
ReaderSideBar

I was going to ask some questions but seems like you already answered them :D.

I have one more question, as in the gif, clicking zoomIn/zoomOut button doesn't increments/decrements the percentage in Reader Mode currently. Should it also update the zoom percentage?

Comment 12

[:Gijs (he/him)]

(In reply to Berkay Barlas from comment #11)

Thank you for you suggestion Gijs.
I created a revision and here is a gif that shows current behavior.
ReaderSideBar

I was going to ask some questions but seems like you already answered them :D.

I have one more question, as in the gif, clicking zoomIn/zoomOut button doesn't increments/decrements the percentage in Reader Mode currently. Should it also update the zoom percentage?

I think it would be very tricky, so I wouldn't worry about the zoom percentage here.

Comment 13

[Berkay Barlas]

Attachment: Bug 1135593 created changeFontSize method

Depends on D23147

Comment 14

[Berkay Barlas]

Attachment: Bug 1135593 eslint fix

Depends on D23340

Comment 15

[Berkay Barlas]

Gijs I'm updating the revision with

hg commit -m "message"
moz-phab submit

``
Is it the right way to do it?

Comment 16

[:Gijs (he/him)]

(In reply to Berkay Barlas from comment #15)

Gijs I'm updating the revision with

hg commit -m "message"
moz-phab submit

``
Is it the right way to do it?

Please use hg histedit to merge the different commits (roll the "fixup" commits into the top commit). When updating the commits after that, use hg amend or hg commit --amend to update the existing commit.

Comment 17

[Berkay Barlas]

Okay, thank you Gijs. I updated it.

By the way I changed the name of buttonControls in UpdateFontSizeButton to UpdateFontSizeButtonControls because I need it bind to 'this' in order to use in changeFontSize method.

Comment 18

[:Gijs (he/him)]

Can you close the other 2 revisions you created? (There's a dropdown above the comment box at the bottom that has a "Close revision" option)

Comment 19

[Berkay Barlas]

(In reply to :Gijs (he/him) from comment #18)

Can you close the other 2 revisions you created? (There's a dropdown above the comment box at the bottom that has a "Close revision" option)

Okay, I abandoned two other revisions.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ea90cc2e917
Make The Reader Mode controls sidebar not affected by page zoom actions r=Gijs

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3ea90cc2e917

Comment 22

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Beta 67 and Release 66.
Verified - Fixed on latest Nightly 68.0a1 (2019-03-20) (64-bit).

Comment 23

[:Gijs (he/him)]

Clearing 'affected' flags for older versions as this is effectively a feature implementation that we won't be uplifting.

Comment 24

[:Gijs (he/him)]

This was backed out of release and esr version 68 in bug 1557256. The changes remain in 69 (soon on beta).