Closed Bug 1557256 Opened 1 year ago Closed 1 year ago

accel + Mouse scroll still uses full page zoom (instead of font size changes) in Reader mode

Categories

(Toolkit :: Reader Mode, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- fixed
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 + fixed
firefox69 --- verified
firefox70 --- verified

People

(Reporter: csasca, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached image Zoom.gif

Affected versions

  • Firefox Beta 68.0b7
  • Firefox Nightly 69.0a1

Affected platforms

  • Windows 10 (x64)
  • macOS 10.14
  • Ubuntu 18.04 (x64)

Steps to reproduce

  1. Start Firefox.
  2. Enter for example: https://en.wikipedia.org/wiki/Smythe%27s_Megalith.
  3. Enter Reader Mode by clicking the associated button from the Location Bar.
  4. Zoom in or out from the page by using ctrl + and - combinations from the keyboard.

Expected result

  • The page zooms normally, the value of the zoom is shown in the Location Bar.

Actual result

  • The ctrl + and - combination for zooming has different behavior than using ctrl + scroll from the mouse.

Regression range

Additional notes

  • Firefox 67.0.2 and 60.7.0esr are not affected.
  • The reset zoom is broken as well if the page is zoomed by using the mouse scroll.
  • Please see the attachment for the issue.
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: Different behaviors for zooming in Reader mode → accel + Mouse scroll still uses full page zoom (instead of font size changes) in Reader mode

[Tracking Requested - why for this release]:
This is confusing for users and needs tidying up.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P1

Masayuki, I wonder if you saw https://phabricator.services.mozilla.com/D34152#1011695 ? Given this is tracking 68 it would be good to figure out a direction for a solution sooner rather than later...

Flags: needinfo?(masayuki)

Oops, really sorry. I forgot to reply it because I tried to look for a better solution, but I couldn't. I'll reply it soon.

Flags: needinfo?(masayuki)

smaug: Could you check the Fabricator's comment? Do you like to create CustomEvent in C++ code?

Flags: needinfo?(bugs)
Attachment #9070571 - Attachment description: Bug 1557256 - override page zoom scrolling for reader mode, r?masayuki → Bug 1557256 - override page zoom scrolling for reader mode, r?smaug

! In D34152#1060846, @smaug wrote:
But isn't the current ctrl+wheel handling more correct than ctrl++/-

Well, having more than 1 way to do basically the same thing was problematic, and the downside of the full page zoom stuff was that it also zoomed in the toolbar, which wasn't what people expected (cf. bug 1135593).

The latter doesn't deal with images, so the result looks rather bad.

It does [look bad]? Can you provide an example where you tested this?

Anyhow, I'd just use UIEvent here so that we can avoid use of JSAPI

How do I do that / is there an example I can follow?

(doing this in bugzilla because it's easier to keep track of than phab comments - I would request needinfo but there's already one out on smaug...)

I was using the wikipedia link from this bug for testing (in reader mode).
ctrl+wheel zooms the content of that page correctly, though the % marker in the location bar is broken (clicking it doesn't reset zoom level).
ctrl++/- zooms only the text, so the layout is kind of broken.

But for UI Event, just do something like
UIEventInit eventInit;
eventInit.mDetail = <some magic number>;
...
RefPtr<UIEvent> event = UIEvent::Construct(global, NS_LITERAL_STRING("your magical event"), eventInit, rv);
...
DispatchEvent(event);

(And yes, keeping conversations in Bugzilla is always easier than in phabricator.)

Flags: needinfo?(bugs)

We built the last beta for 68 so it's probably too late for this.

(In reply to Julien Cristau [:jcristau] from comment #8)

We built the last beta for 68 so it's probably too late for this.

Would we consider backing out https://bugzilla.mozilla.org/show_bug.cgi?id=1135593 and https://bugzilla.mozilla.org/show_bug.cgi?id=1536620 for 68? I'd rather we don't ship the inconsistent state (some zoom control things use reader mode state, some use page zoom, and the latter ones can still cause the in-urlbar thing to show up which is then non-functional), but I accept it's late in the cycle now. The patch for this bug has proven trickier to write than I would have liked. :-(

Flags: needinfo?(jcristau)

Beta/Release Uplift Approval Request

  • User impact if declined: Inconsistent zoom behaviour (between accel+mousewheel and shortcut / UI buttons)
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This backs out bug 1135593 and bug 1536620 from beta, so it returns us to the zoom behaviour in reader mode that we had before (normal page/text zoom as per user prefs).
  • String changes made/needed: none
Attachment #9074829 - Flags: approval-mozilla-beta?
Comment on attachment 9074829 [details] [diff] [review]
Back out patch for beta

backout a couple of changes to avoid shipping a regression in reader mode, approved for 68.0 rc1
Flags: needinfo?(jcristau)
Attachment #9074829 - Flags: approval-mozilla-release+
Attachment #9074829 - Flags: approval-mozilla-esr68+
Attachment #9074829 - Flags: approval-mozilla-beta?

(In reply to Olli Pettay [:smaug] from comment #7)

But for UI Event, just do something like
UIEventInit eventInit;
eventInit.mDetail = <some magic number>;
...
RefPtr<UIEvent> event = UIEvent::Construct(global, NS_LITERAL_STRING("your magical event"), eventInit, rv);
...
DispatchEvent(event);

Sorry, I don't see how to get a global here, as there's no JS caller - how do I get a reference to the right dom GlobalObject ? It seems to be different from nsIGlobalObject...
I'm also assuming you mean UIEvent::Constructor, which still has no C++ callers, only webidl codegen stuff that provisions for calling it from JS...

Flags: needinfo?(bugs)

OK, I think I figured this out.

Flags: needinfo?(bugs)

Abraham, there's some question over the best way forward here.

In bug 1135593, people raised the fact that page zoom shouldn't affect the reader mode controls (currently sidebar, soon toolbar). Because the reader mode controls are in the same browser and document as the reader mode content, there is no way to isolate the effect of page zoom that way. However, reader mode already had controls to change the text size we use in reader mode. So in that bug, we changed things such that all page zoom operations instead controlled the text size in reader mode.

We missed the modifier key + scroll zoom operator, which is what this bug is about (so that still uses full page zoom, which alters the size of the reader mode controls, instead of altering the text size in reader mode).

Olli pointed out that changing the font size for the content does not zoom other non-text content (like images). So the question is which trade-off we want here:

  1. have page zoom affect the reader mode controls even though it shouldn't
  2. have page zoom only affect text in the content, fixing (1) but now leaving images the same size irrespective of the user attempting to zoom.

Which would you prefer we go with?

Flags: needinfo?(awallin)

@gijs, this is a tricky one. We don't want reader mode controls to adjust with page zoom but would like all content on the page to adjust.

In this case I lean towards option 2 being the lessor of two evils as adjusting the size of reader mode buttons degrades the experience more than fixed size images (which are often stripped out of reader mode entirely).

Flags: needinfo?(awallin)
Attachment #9070571 - Attachment is obsolete: true
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a719791b50e3
handle scrollwheel zoom in reader mode, r=jaws
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

I think we need a Beta approval request here?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

I think we need a Beta approval request here?

Yes, but I'm out today and it needs a beta patch, because the hunk in the original phab patch in browser-fullZoom got fixed by the pdfjs bug - so there's no changes there in the landed patch but there should be on beta. I'll get to this ASAP when I'm back.

Attached patch Patch for betaSplinter Review

Beta/Release Uplift Approval Request

  • User impact if declined: Confusing zoom scrollwheel behaviour when using modifier key + scroll in reader mode
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment #0
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Frontend-only patch that makes some small changes to reader mode code only.
  • String changes made/needed: nope
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9079565 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

The issue is no longer reproducible on latest Nightly 70.0a1 (2019-07-21). Tests were performed under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.14

Comment on attachment 9079565 [details] [diff] [review]
Patch for beta

Fix for confusing zoom scrollwheel behaviour when using modifier key + scroll in reader mode. Approved for 69.0b7.
Attachment #9079565 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Conflict when I tried to uplift this patch:
warning: conflicts while merging browser/base/content/browser-fullZoom.js

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Razvan Maries from comment #26)

Conflict when I tried to uplift this patch:
warning: conflicts while merging browser/base/content/browser-fullZoom.js

You tried to use https://bugzilla.mozilla.org/attachment.cgi?id=9079565 ?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rmaries)
Flags: needinfo?(rmaries)

The issue has been fixed on Firefox 69.0b7 too. Tests were performed under WIndows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.10.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1578454
Regressions: 1583837
You need to log in before you can comment on or make changes to this bug.