479161 - Double-tap zooming doesn't continue to zoom in for nested elements

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Madhava Enros [:madhava]]

Double-tapping will zoom in to an element, but if there's an element within that one (i.e. a photo within a paragraph of a newspaper, where you've already zoomed to the paragraph), doubletapping on that second element should zoom further in to fit that to the screen.  Only when you're already zoomed to the size of the double-tapped-on element should the double tap cycle you back out to full page width.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

it would be nice to not have to cycle through several layers of elements before zooming out.  Perhaps triple tap could be mapped to either zoom out one level or zoom to 100%.

Comment 2

[Madhava Enros [:madhava]]

Agreed - but I think what works is that if you double tap on an area that is fully zoomed, for example the paragraph in the description above, you'd zoom back out.  Only doubletapping on a smaller element within that element would cause you to zoom further in.

Comment 3

[Jaakko Viitanen]

here is a small example of my view how the double-tap zoom should work:

the user has enters a web page fitted to the screen. The web page has a two column design with one picture. The first column is cell 1 and the second column is cell 2 and a picture on the web page is cell 3. The user double taps on cell 1. Cell 1 is zoomed (zoom level 156%) and centered on screen. The user pans and reads the column and moves to cell 2. The user double taps cell 2. Cell 2 is centered on screen and zoomed (zoom level 134%) the user double taps on Cell 2 again and the zoom level is set to the default value (page fitted to the screen).  The user pans the page to the picture and double taps on It. Cell 3 is centered on screen and zoomed (zoom level 240%). The user decides to read the accompanying column and double taps it. Cell 2 is centered on screen and zoomed (zoom level 156%). The user double taps on Cell 2 and zoom level is returned to the default value. 


one of the critical things is also to think about how the cell is centered on screen. the best option here is if the user double-taps on a certain point on a cell that point should then be at the middle of the screen (or that column, x-axis).

Comment 4

[Brian Crowder]

Is there a test page or site that represents the kind of nested elements we care about for this?  Or a URL that exhibits that sort of design?

Comment 5

[Brian Crowder]

Attachment: keep on zooming until we're zooming on the same element

I kind of like this...  it makes moving around on the page a little easier and also makes "backing out" possible, by letting you double-click on space outside of an element if you're zoomed too close (thus letting you slide out to its container).  I think maybe we should have better logic for positioning the newly-zoomed element to line up with the screen edges, but perhaps that is for another bug.

Comment 6

[Jaakko Viitanen]

a nice transition would be good too. So that the user wont get confused when the zoom zaps to the column where the user is zooming to.

~300ms long smooth transition zooming in or out.

what do you think?

Comment 7

[Brian Crowder]

Yeah, sounds like a good idea...  is there already scaffolding for move-to animations like that, or does that need to be built?

Comment 8

[Brian Crowder]

Per discussion on IRC, gonna hold off on animating until perf is better and we've got a system in place for this sort of thing.

Comment 9

[Brian Crowder]

Comment on attachment 373913 [details] [diff] [review]
keep on zooming until we're zooming on the same element

Not sure if this is what we're after or not, but I figured the best way to find out was to solicit review.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 373913 [details] [diff] [review]
keep on zooming until we're zooming on the same element

Let's change the zoom clamp to see how well we can zoom into nested elements. A max of 2 is too limiting.

Comment 11

[Brian Crowder]

Attachment: backup

Better than previous, but seems to have a bug on zooming out to an enclosing element; tracking that down.  Also, this has some debugging junk in it.

Comment 12

[Brian Crowder]

Attachment: for review

I'm -almost- happy with how this works.  There seem to be a couple of other bugs, but they aren't coming from this patch, I think.  I'll track them down elsewhere.  Will do vertical orientation for small-enough elements in bug 484011.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 375820 [details] [diff] [review]
for review

>diff --git a/chrome/content/CanvasBrowser.js b/chrome/content/CanvasBrowser.js
>-    /* Try to set zoom-level such that once zoomed element is as wide
>-     *  as the visible viewport */
>-    let zoomLevel = visibleViewportWidth / (elWidth + (2 * margin));
>-    ws.beginUpdateBatch();

We should check with Taras about the need for a batch

>+    let realWidth = Math.ceil(this._visibleBounds.width);
>+    let viewportWidth = this._pageToScreen(this._visibleBounds.width);
> 
>-    this.zoomLevel = zoomLevel;
>+    // Try to make the zoomed element be as wide as the viewport
>+    this.zoomLevel = viewportWidth / (elRect.width + (2 * margin));
> 
>-    // uncomment this to force the zoom level to 1.0 on zoom, instead of
>-    // doing a real zoom to element
>-    //this.zoomLevel = 1.0;
>+    // _visibleBounds has been recalculated by the zoomLevel setter.
>+    let realWidth = Math.ceil(this._visibleBounds.width);

realWidth is declared above. Probably want to drop the "let"

>+    // Prevent zooming from pulling in URL-bar or sidebars by panning off of
>+    // the edge of the page.
>+    let cw = this._browser.contentWindow;
>+    let endX = Math.min(elRect.x - margin, cw.innerWidth - realWidth);
>+    endX = Math.floor(this._pageToScreen(endX));
>+
>+    let endY = this._pageToScreen(elRect.y - margin);

Does this need a Math.floor?

>-
>-  /* Pans directly to a given content element */
>-  panToElement: function panToElement(aElement) {
>-    var elRect = this._getPagePosition(aElement);
>-
>-    this.panTo(elRect.x, elRect.y);
>-  },

Any reason for removing this one? We might want this for doing other programmatic panning

>-
>-  panTo: function panTo(x, y) {
>-    ws.panTo(x, y);
>-  }

Yeah, this seems redundant

Comment 14

[Brian Crowder]

(In reply to comment #13)
> We should check with Taras about the need for a batch

I need at least to move the beginUpdateBatch() to after the zoomLevel setter gets called, otherwise I can't use the calculations to position the screen following that.


> >+    let realWidth = Math.ceil(this._visibleBounds.width);
> realWidth is declared above. Probably want to drop the "let"

Right, will fix.


> Does this need a Math.floor?

If rounding is needed, ws.panTo() should be responsible for it, don't you think?


> Any reason for removing this one? We might want this for doing other
> programmatic panning

They're both totally unused right now, can we just reintroduce them when they actually do something?

Comment 15

[Brian Crowder]

Attachment: Fixing redundant realWidth

Carrying over mfinkle's review unless he wants to look again, and moving tglek's review to here.

Comment 16

[(dormant account)]

Comment on attachment 375820 [details] [diff] [review]
for review

taking out the transaction is bad(as discussed on irc)

Comment 17

[Brian Crowder]

Attachment: no drawing on zoom

The pain of the transaction is from drawing, so we can remove that with a light touch in CanvasBrowser and still get all the calculations we need done.

Comment 18

[(dormant account)]

Comment on attachment 375879 [details] [diff] [review]
no drawing on zoom

zoomToElement changes are addressing changes outside of the scope of this bug. This issue came up elsewhere (like in bug 475997) and needs to be fixed properly in own bug.

Comment 19

[Brian Crowder]

I don't think it's necessary to do a bunch of tiny patches in dependent bugs.  We all pretty much acknowledge that zooming is b0rked, can we just take a big swing at it here?

Comment 20

[Brian Crowder]

re: comment #19:  I'm not saying I expect this patch to fix everything.  But it's a pretty clean, minimal patch that fixes a good chunk of brokenness.  I'll close out the relevant bugs, if I can get buy-off on this patch.

Comment 21

[Brian Crowder]

Attachment: minimal

In the interests of moving this bug forward, I will open another for the remainder of the zoom fixes.

Comment 22

[Brian Crowder]

Comment on attachment 375895 [details] [diff] [review]
minimal

woops, wrong patch

Comment 23

[Brian Crowder]

Attachment: minimal, for real

Comment 24

[(dormant account)]

Comment on attachment 375897 [details] [diff] [review]
minimal, for real

>Bug 479161 - Double-tap zooming doesn't continue to zoom in for nested elements
>
>diff --git a/chrome/content/CanvasBrowser.js b/chrome/content/CanvasBrowser.js
>--- a/chrome/content/CanvasBrowser.js
>+++ b/chrome/content/CanvasBrowser.js
>@@ -486,7 +486,7 @@ CanvasBrowser.prototype = {
> 
>   _clampZoomLevel: function _clampZoomLevel(aZoomLevel) {
>     const min = 0.2;
>-    const max = 2.0;
>+    const max = 4.0;
> 
>     return Math.min(Math.max(min, aZoomLevel), max);
>   },
>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js
>--- a/chrome/content/InputHandler.js
>+++ b/chrome/content/InputHandler.js
>@@ -755,7 +755,7 @@ function ContentClickingModule(owner) {
> ContentClickingModule.prototype = {
>   _clickTimeout : -1,
>   _events : [],
>-  _zoomed : false,
>+  _zoomedTo : null,
> 
>   handleEvent: function handleEvent(aEvent) {
>     // exit early for events outside displayed content area
>@@ -824,36 +824,18 @@ ContentClickingModule.prototype = {
> 
>     function optimalElementForPoint(cX, cY) {
>       var element = Browser.canvasBrowser.elementFromPoint(cX, cY);
>-      if (!element)
>-        return null;
>-
>-      // Find the nearest non-inline ancestor
>-      while (element.parentNode) {
>-        let display = window.getComputedStyle(element, "").getPropertyValue("display");
>-        let zoomable = /table/.test(display) || /block/.test(display);
>-        if (zoomable)
>-          break;
>-
>-        element = element.parentNode;
>-      }
>       return element;
>     }
> 
>     let firstEvent = this._events[0].event;
>     let zoomElement = optimalElementForPoint(firstEvent.clientX, firstEvent.clientY);
> 
>-    if (zoomElement) {
>-      if (this._zoomed) {
>-        // zoom out
>-        this._zoomed = false;
>-        Browser.canvasBrowser.zoomFromElement(zoomElement);
>-      }
>-      else {
>-        // zoom in
>-        this._zoomed = true;
>-        Browser.canvasBrowser.zoomToElement(zoomElement);
>-      }
>-
>+    if (zoomElement != this._zoomedTo) {
>+      this._zoomedTo = zoomElement;
>+      Browser.canvasBrowser.zoomToElement(zoomElement);
>+    } else {
>+      this._zoomedTo = null;
>+      Browser.canvasBrowser.zoomFromElement(zoomElement);
>     }
> 
>     this._owner.ungrab(this);

Comment 25

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090821
Fennec/1.0b3pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre