433578 - panning does not work on planet.mozilla.org

Status: VERIFIED FIXED

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

Description

[Doug Turner (:dougt)]

drawWindow either fails or the math is screwed up.  we should also check on other page with lots of content.

Comment 1

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

This is happening because the <canvas> used for panning is set to a height that is way too big (plaent.mozilla.org is very tall).

I am refactoring the panning code to be more vlad-like and not use so much canvas.

Comment 2

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

Attachment: replaces the scrollbox method of panning

In addition to replacing the <scrollbox>, this patch also minimizes the size of the <canvas> used to render the panning image to the size of the viewport only (which fixes the planet.mozilla.org bug)

Note: I am painting the background of the <canvas> red to show the code is working right. I'll remove it later.

Comment 3

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

Comment on attachment 321190 [details] [diff] [review]
replaces the scrollbox method of panning

I found a problem with panning on the device that didn't happen on my desktop. Subsequent pans (not the first) seem to offset downward right away.

Also, getting the pan to start seems sluggish, then it works fine.

Consider this a WIP

Comment 4

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

Attachment: v2 with fixes for on-device panning

Same as before but changes from event.clientX/Y to event.screenX/Y in mouse handling. Fixes problems last patch had on device. Also reduced the "jitter" buffer for smoother panning.

Comment 5

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

Attachment: v3 with more tweaks

this patch is basically the same as v2 but with the following additions:
* turns off simulated double click code
* turns off simulated tap-n-hold context menu code
* uses "white" in drawWindow call instead of "rgb(0,0,0,0)"
* adds | moz-opaque="true" | to the canvas for when bug 430906 lands
* reorders <stack> children so we only need to toggle visibility of <browser>, not <canvas> too

Gavin asked me to land this to make it easier to test, so I am.

Comment 6

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

Additional note: I found that this code works better when _not_ SSHing into the device to run the browser.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 321460 [details] [diff] [review]
v3 with more tweaks

Looks good, nothing but nits:

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-          if(uri != "" && uri[0] != '-'){
>+          if (uri != "" && uri[0] != '-') {

was-there-before-this-patch nit: avoid explicit comparisons to ""/null/undefined? (I find them harder to read).

>+  mousemove: function(aEvent)

>+    if (40 > delta || (2 > Math.abs(x) && 2 > Math.abs(y)))
>+      return;

Would be nice to add named constants for these.

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml

>       <method name="doPan">

>+            this._updateTimer = setTimeout(_doUpdate, 300);

Here too.

Comment 8

[Doug Turner (:dougt)]

this works for me now.  Marking as fixed.

Comment 9

[Aakash Desai [:aakashd]]

This bug depends on the resolution of bug 500301 and it's verification will be revisited once that bug is fixed.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

verified with 20090914 on 1.9.2 nightly with my n810