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

• First bad: 3ea90cc2e91732daccfc382206926d0c9dc90e24
• Last good: e05dc78eb4272fff00d1aee1b1d629c0163ca343
• Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e05dc78eb4272fff00d1aee1b1d629c0163ca343&tochange=3ea90cc2e91732daccfc382206926d0c9dc90e24
• Potential regressor: 1135593

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.

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

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

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.

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

! 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.)

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

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

https://hg.mozilla.org/releases/mozilla-release/rev/ee5ddded2724

https://hg.mozilla.org/releases/mozilla-esr68/rev/ee5ddded2724

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

OK, I think I figured this out.

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?

@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).

https://hg.mozilla.org/mozilla-central/rev/a719791b50e3

I think we need a Beta approval request here?

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

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

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

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

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

https://hg.mozilla.org/releases/mozilla-beta/rev/de26254af992

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.