452274 - spatial navigation doesn't pan content area

Status: RESOLVED FIXED

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

Description

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

We can use the existing callback to pan the element into view pretty easily, I think.

Comment 1

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

Attachment: patch

This works, but isn't quite perfect, because it pan the element to 0,0 even if it's already visible. Really what we want is a "ensureElementIsVisible", I think.

Comment 2

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

(In reply to comment #1)
> Created an attachment (id=335565) [details]
> patch
> This works, but isn't quite perfect, because it pan the element to 0,0 even if
> it's already visible. Really what we want is a "ensureElementIsVisible", I
> think.

Agreed. We don't want to pan unless the element is partially or completely offscreen.

Comment 4

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

Attachment: better patch

This adds deckbrowser.ensureElementIsVisible, which mostly works, except for the fact that the canvas/browser is bigger than the viewport by default, which means things can still be hidden.

This also works around SpatialNav causing trouble when it calls advanceFocus/rewindFocus, which can result in the viewport being scrolled (which is problematic because of bug 452286).

Comment 5

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

Attachment: updated patch

I added code to adjust the pan based on the deckbrowser's position in the window, that assumes that the deckbrowser is fullscreen. Slightly icky, but not sure there's a cleaner way to do this. People who use deckbrowsers that aren't fullscreen will have to deal :)

Comment 6

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

Comment on attachment 340931 [details] [diff] [review]
updated patch

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml
>+          // Adjust for part of our viewport being offscreen
>+          // XXX this assumes that the browser is meant to be fullscreen
>+          let browserRect = this.getBoundingClientRect();
>+          curRect.height -= this._screenToPage(Math.abs(browserRect.top));
>+          if (browserRect.top < 0)
>+            curRect.y -= this._screenToPage(browserRect.top);
>+          curRect.width -= this._screenToPage(Math.abs(browserRect.left));
>+          if (browserRect.left < 0)
>+            curRect.x -= this._screenToPage(browserRect.left);

Could we add offseting like you did in the click redirects here later? Would that be enough to make non-fullscreen work?

>+      <!-- Pans X/Y pixels (in content coordinates) -->
>+      <method name="pan">
>+        <parameter name="dX"/>
>+        <parameter name="dY"/>

Rename this method to "panBy" ? Consistent with scrollTo / scrollBy

Comment 7

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

(In reply to comment #6)
> Could we add offseting like you did in the click redirects here later? Would
> that be enough to make non-fullscreen work?

The click retargeting is simpler because the click is guaranteed to be on a visible part of the browser, and we just need to adjust for its position in the window. For this code, the element can be anywhere in the content document, and we need to find what region of the browser actually visible on the screen. Could probably make this code better but I think we should go with this for now.

> Rename this method to "panBy" ? Consistent with scrollTo / scrollBy

Will do.

Comment 8

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

http://hg.mozilla.org/mobile-browser/rev/31f56bf4590a