Add support for full zoom via toolkit's ZoomManager

RESOLVED INCOMPLETE

Status

Firefox for Metro
Pan and Zoom
P1
normal
RESOLVED INCOMPLETE
4 years ago
2 years ago

People

(Reporter: smaug, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=5, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Currently zoom doesn't seem to work at all in Metrofox.
ctrl+/- or pinch gesture from touchpad don't do anything

Comment 1

4 years ago
This is expected. We've disabled the old toolkit zoom. To get this working we'll need to hook all the older input shortcuts and up to the apzc, which unfortunately we won't have for fx28.

Updated

4 years ago
Blocks: 861680
(In reply to Jim Mathies [:jimm] from comment #1)
> This is expected. We've disabled the old toolkit zoom. To get this working
> we'll need to hook all the older input shortcuts and up to the apzc, which
> unfortunately we won't have for fx28.

Actually, we could just make the keyboard/trackpad/wheel zoom use fullZoom as in desktop Firefox.  This is what IE and Chrome do, both on the desktop and in Metro:  APZC-style viewport zooming for touch, and desktop-style layout zooming for keyboard/trackpad/wheel.

Comment 3

4 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> (In reply to Jim Mathies [:jimm] from comment #1)
> > This is expected. We've disabled the old toolkit zoom. To get this working
> > we'll need to hook all the older input shortcuts and up to the apzc, which
> > unfortunately we won't have for fx28.
> 
> Actually, we could just make the keyboard/trackpad/wheel zoom use fullZoom
> as in desktop Firefox.  This is what IE and Chrome do, both on the desktop
> and in Metro:  APZC-style viewport zooming for touch, and desktop-style
> layout zooming for keyboard/trackpad/wheel.

Didn't we disable page zoom because it was in conflict with apzc zoom? I can't remember the bug number.
(In reply to Jim Mathies [:jimm] from comment #3)
> Didn't we disable page zoom because it was in conflict with apzc zoom? I
> can't remember the bug number.

We removed the old Control-Plus/Minus shortcuts which used the "AnimatedZoom" class which was how we did viewport zooming before APZC.  We can't just restore that old code since it won't work in the APZC world, as you say.  We'd add new code that used ZoomManager instead (like desktop Firefox).

Updated

4 years ago
Duplicate of this bug: 948328

Comment 6

4 years ago
Too late to block28 on this?
Whiteboard: [triage]

Updated

4 years ago
Summary: Zoom is disabled in Metrofox → Page zoom is currently disabled in Metro Firefox

Updated

4 years ago
Blocks: 948328

Updated

4 years ago
Blocks: 838081
No longer blocks: 861680

Updated

4 years ago
Blocks: 861680
No longer blocks: 838081
Whiteboard: [triage]
Duplicate of this bug: 948328
Whiteboard: [v1?]

Updated

4 years ago
Whiteboard: [v1?] → [feature] p=0

Updated

4 years ago
Priority: -- → P1
Whiteboard: [feature] p=0 → p=0

Updated

4 years ago
Assignee: nobody → jmathies
Whiteboard: p=0 → p=5

Comment 8

4 years ago
Created attachment 8387568 [details] [diff] [review]
wip v.1

This works great, except that apzc panning experiences heavy checkerboarding (black rects at the bounds) when zoom is applied.

Comment 9

4 years ago
You can also apzc zoom pages out that have been zoomed in, and sort of lock them into the center of the screen heavily cropped.
Although we've tried to write the APZ code so that it works with full zoom, it's quite possible that there are issues there because until now we've never been able to test it.

Updated

4 years ago
Depends on: 980906

Updated

4 years ago
Summary: Page zoom is currently disabled in Metro Firefox → Add support for font inflation via toolkit's ZoomManager

Comment 11

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Although we've tried to write the APZ code so that it works with full zoom,
> it's quite possible that there are issues there because until now we've
> never been able to test it.

now we do! :)

Comment 12

4 years ago
I'm using the wrong terminology I guess.
Summary: Add support for font inflation via toolkit's ZoomManager → Add support for full zoom via toolkit's ZoomManager

Updated

4 years ago
Depends on: 980923

Updated

4 years ago
Depends on: 980925

Updated

4 years ago
Depends on: 980926

Updated

4 years ago
Depends on: 980931

Comment 13

4 years ago
Created attachment 8387630 [details] [diff] [review]
wip v.2

- added keyboard shortcuts
Attachment #8387568 - Attachment is obsolete: true

Comment 14

4 years ago
Hmm, strange, text selection with the mouse has offset coordinates. I'd expect problems with our ui widget positioning, but not with platform code.

Comment 15

4 years ago
(In reply to Jim Mathies [:jimm] from comment #14)
> Hmm, strange, text selection with the mouse has offset coordinates. I'd
> expect problems with our ui widget positioning, but not with platform code.

Tracked this down, we need to override the fullZoom prop in our bindings so it points to the right nsIMarkupDocumentViewer.

Updated

4 years ago
Assignee: jmathies → nobody
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.