Closed Bug 941592 Opened 6 years ago Closed 6 years ago

Defect - double-tap to zoom fails if the user zooms out

Categories

(Firefox for Metro Graveyard :: Pan and Zoom, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: jimm, Assigned: mbrubeck)

References

Details

(Whiteboard: [block28] feature=defect c=tbd u=tbd p=2)

Attachments

(1 file, 2 obsolete files)

str:

1) double tap to zoom in
2) manually zoom out
3) double tab to zoom in again

result:

no action

We should check browser scale and reset the zoomed flag here - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/Content.js#377
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #8336009 - Flags: review?(mbrubeck)
Comment on attachment 8336009 [details] [diff] [review]
fix

There's no need to store _isZoomedIn as a flag if we're just going to overwrite it based on the zoom level each time we read it.  We could just check the zoom level directly.

Just checking if (resolution > 1) is not ideal either -- it creates an opposite problem, where double-tap to zoom *in* is broken if you're already zoomed in a little.  Really we should check whether zooming to the tapped element would change the zoom level significantly from its current value.  If so, zoom to the element; otherwise zoom out.
Attachment #8336009 - Flags: review?(mbrubeck) → review-
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Just checking if (resolution > 1) is not ideal either -- it creates an
> opposite problem, where double-tap to zoom *in* is broken if you're already
> zoomed in a little.  Really we should check whether zooming to the tapped
> element would change the zoom level significantly from its current value. 
> If so, zoom to the element; otherwise zoom out.

Alternately we could make this code listen for "manual" (pinch/keyboard/wheel) zoom events, and set _isZoomedIn to false when one of those happens.
Summary: double-tap to zoom fails if the user zooms out → Defect - double-tap to zoom fails if the user zooms out
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0
Assignee: jmathies → nobody
Assignee: nobody → mbrubeck
Blocks: metrov1it20
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=2
Attached patch WIP (obsolete) — Splinter Review
This is the start of a patch that shamelessly copies a bunch of code from TabChild/BrowserElementPanning.
Attachment #8336009 - Attachment is obsolete: true
Attached patch patchSplinter Review
I realized we can do this without schlepping a ton of data to the content script on every update (as in my previous patch).  Instead we do the calculation in the chrome script where we already have this data.

This also fixes a bug where browser.contentViewportBounds was returning all NaN because it was passing an nsIDOMClientRect instead of a Rect object, and a bug in ptClientToBrowser and rectClientToBrowser where scroll coordinates were scaled incorrectly, and cleans up some code in apzc.js so it no longer adds and removes listeners for every tab.
Attachment #8339628 - Attachment is obsolete: true
Attachment #8341783 - Flags: review?(jmathies)
Comment on attachment 8341783 [details] [diff] [review]
patch

Review of attachment 8341783 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes up a selection bounds bug I spotted last weekend as well. Thanks!
Attachment #8341783 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/7b8cc2b3568b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.