The Reader Mode controls sidebar is affected by page zoom actions

VERIFIED FIXED in Firefox 69

Status

()

defect
P3
normal
VERIFIED FIXED
5 years ago
2 months ago

People

(Reporter: avaida, Assigned: fpberkay, Mentored)

Tracking

38 Branch
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 wontfix, firefox69 verified)

Details

(Whiteboard: [about-reader-ui])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Flags: qe-verify+
Depends on: 1120735
Priority: -- → P3
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.
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.
Duplicate of this bug: 1151092
Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl
Version: Firefox 38 → 38 Branch
Component: General → Reader Mode
Product: Firefox → Toolkit
Whiteboard: [about-reader-ui]

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

Mentor: gijskruitbosch+bugs

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

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

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

(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

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?

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

Depends on D23147

Posted file Bug 1135593 eslint fix (obsolete) —

Depends on D23340

Gijs I'm updating the revision with

hg commit -m "message"
moz-phab submit

``
Is it the right way to do it?

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

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.

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)

Flags: needinfo?(fpberkay)
Attachment #9050686 - Attachment is obsolete: true
Attachment #9050687 - Attachment is obsolete: true

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

Flags: needinfo?(fpberkay)
Assignee: nobody → fpberkay
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3ea90cc2e917
Make The Reader Mode controls sidebar not affected by page zoom actions r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1536620

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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

Regressions: 1557256

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

Target Milestone: mozilla68 → mozilla69
You need to log in before you can comment on or make changes to this bug.