808031 - Double tapping to focus on a text column does not work

Status: RESOLVED WORKSFORME

(Core :: General, defect, P3)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Screen shot of the bug

Build 2012-11-01

See screenshot.

To reproduce:

1) Open http://bits.blogs.nytimes.com/2012/11/02/android-nexus-strategy
2) Try to double tap on the main text column to make it zoom to fit.

May be related to zoom factor of the page?

Comment 1

[Dylan Oliver [:doliver]]

blocking+ -- per Josh on usability of the browser

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

To clarify: double tap works. The problem is that the positioning and zooming that happens after the double tap is not correct. (Like in my screenshot: the zoomed content is too wide for the display)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I actually don't know where the code for double-tap zoom lives.  I thought for Fennec Android it lives in the front end; I'd have guessed the same for B2G given that I don't recall seeing patches to move it into Gecko (though I could certainly have missed something).

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Baron [:dbaron] from comment #3)
> I actually don't know where the code for double-tap zoom lives.  I thought
> for Fennec Android it lives in the front end; I'd have guessed the same for
> B2G given that I don't recall seeing patches to move it into Gecko (though I
> could certainly have missed something).

I think the code has landed in https://bugzilla.mozilla.org/show_bug.cgi?id=750977.

The logic is likely http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#574

Adding cjones to the discussion.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The code that decides where to zoom is

http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js#216

AsyncPanZoomController just zooms to whatever rect BrowserElementScrolling tells it to.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So does anyone have a good idea as to who should own this bug?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In other words:  if there's an owner for this code who isn't overloaded and wants to look at this, they should go ahead; otherwise either I or sjohnson should probably take it.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Please do!

Comment 9

[Benjamin Chen [:bechen]]

Hi David
My patch in bug 805535 may be able to fix it.

Comment 10

[Benjamin Chen [:bechen]]

Sorry, should be bug 804985, attachment 682972 [details] [diff] [review]

Comment 11

[Jet Villegas (inactive)]

+cc: ttaubert to try with the same build he's using to test bug 804985

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Bug 804985 doesn't seem to make a difference for me. Works well with and without the patch.

Comment 13

[Benjamin Chen [:bechen]]

The attachment 677780 [details] looks like the zoom ratio is very large after double tapping it. And according to my description in bug 804985 and Bug 805535, the large scale number appears when the current zoom is very small and then double tapping.
I guess the reproduce steps:
1. Reproduce Bug 805535 to get the small zoom
1.1 open browser go to any webpage
1.2 find a text area, zoom in to a large scale
1.3 double tap the area
2. double tapping some text area

If the target rect ideal zoom ratio is 5 and after step 1, the scale number should be smaller than 1, assume 0.5 ( On screen area of the webpage area now is 1/4).
At step 2 when double tapping, we will get 10 for the zoom. 
10*0.5 = 5; but we set the zoom to 10. 

Maybe the case can explain attachment 677780 [details].

Comment 14

[Scott Johnson (:jwir3)]

One thing I notice that's odd about this code, as compared to mobile/android/chrome/content/browser.js (where the double-tap to zoom code seems to work as expected):

BrowserElementScrolling.js
> 243       let bRect = new Rect(Math.max(cssPageRect.x, rect.x - margin),
> 244                            rect.y,
> 245                            rect.w + 2 * margin,
> 246                            rect.h);
> 247       // constrict the rect to the screen's right edge
> 248       bRect.width = Math.min(bRect.width, cssPageRect.right - bRect.x);

browser.js:
> let bRect = new Rect(Math.max(viewport.cssPageLeft, rect.x - margin),
>                      rect.y,
>                      rect.w + 2 * margin,
>                      rect.h);
>   // constrict the rect to the screen's right edge
>   bRect.width = Math.min(bRect.width, viewport.cssPageRight - bRect.x);

Why, in BrowserElementScrolling.js, is the left edge specified by cssPageRect.x, but the right edge specified by cssPageRect.right? Perhaps cssPageRect.right is an alias to cssPageRect.x + cssPageRect.width? (I can't seem to find the definition of this data structure, so I'm not entirely sure where it comes from, other than it being set in _recvViewportChange)

Comment 15

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #14)
> One thing I notice that's odd about this code, as compared to
> mobile/android/chrome/content/browser.js (where the double-tap to zoom code
> seems to work as expected):
> 
> BrowserElementScrolling.js
> > 243       let bRect = new Rect(Math.max(cssPageRect.x, rect.x - margin),
> > 244                            rect.y,
> > 245                            rect.w + 2 * margin,
> > 246                            rect.h);
> > 247       // constrict the rect to the screen's right edge
> > 248       bRect.width = Math.min(bRect.width, cssPageRect.right - bRect.x);
> 
> browser.js:
> > let bRect = new Rect(Math.max(viewport.cssPageLeft, rect.x - margin),
> >                      rect.y,
> >                      rect.w + 2 * margin,
> >                      rect.h);
> >   // constrict the rect to the screen's right edge
> >   bRect.width = Math.min(bRect.width, viewport.cssPageRight - bRect.x);
> 
> Why, in BrowserElementScrolling.js, is the left edge specified by
> cssPageRect.x, but the right edge specified by cssPageRect.right? Perhaps
> cssPageRect.right is an alias to cssPageRect.x + cssPageRect.width? (I can't
> seem to find the definition of this data structure, so I'm not entirely sure
> where it comes from, other than it being set in _recvViewportChange)

BTW - I'm not suggesting this is the cause of the problem, I'm just curious where the code that will eventually call _recvViewportChange() gets initialized (i.e. where does the viewport data come from?)

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We do some awesome printf-JSON-ifying here

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1169

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The STR in comment 0 wfm.  Please file a new bug if you see new, related symptoms.