Closed Bug 452274 Opened 13 years ago Closed 13 years ago

spatial navigation doesn't pan content area

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0a1

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

Attachments

(1 file, 2 obsolete files)

We can use the existing callback to pan the element into view pretty easily, I think.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
OS: Windows Vista → All
Hardware: PC → All
(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.
Flags: wanted-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec A1
Duplicate of this bug: 455612
Attached patch better patch (obsolete) — Splinter Review
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).
Attachment #335565 - Attachment is obsolete: true
Attached patch updated patchSplinter Review
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 :)
Attachment #339726 - Attachment is obsolete: true
Attachment #340931 - Flags: review?(mark.finkle)
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
Attachment #340931 - Flags: review?(mark.finkle) → review+
(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.
http://hg.mozilla.org/mobile-browser/rev/31f56bf4590a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.