468869 - CanvasBrowser ensureElementIsVisible is broken

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

It's broken since we lost the ability to easily find the size of the "viewport" (visible area of page content). Need to reimplement viewportDimensions in CanvasBrowser.js

Comment 1

[(dormant account)]

so looks like this is pretty much there.

get _effectiveViewportDimensions() {
    let r = ws.viewingRect
    return [this._screenToPage(r.width), this._screenToPage(r.height)] 
}

seems to work. What code needs it so I can test it out? I know for my needs width/height is not enough. I'd rather just use ws.viewingRect

Comment 2

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

To test, you can add that method, remove the early return in ensureElementIsVisible, and then try to use spatial nav (arrow keys) to bring offscreen elements into view.

Comment 3

[(dormant account)]

Attachment: fixed it

I solved the visibility stuff needed for this patch in bug 474689, so might as well get that landed.

Comment 4

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

Comment on attachment 358872 [details] [diff] [review]
fixed it

>+    // viewingRect property returns a new bounds object
>+    let visibleBounds = ws.viewingRect
>+    visibleBounds.top = Math.floor(this._screenToPage(visibleBounds.top));
>+    visibleBounds.left = Math.floor(this._screenToPage(visibleBounds.left));
>+    visibleBounds.bottom = Math.ceil(this._screenToPage(visibleBounds.bottom));
>+    visibleBounds.right = Math.ceil(this._screenToPage(visibleBounds.right));
>+    this._visibleRect = {x:visibleBounds.x,
>+                         y:visibleBounds.y,
>+                         width:visibleBounds.width,
>+                         height:visibleBounds.height}


Why copy the visibleBounds into this._visibleRect? Couldn't you just use the visibleBounds object itself?

this._visibleRect = visibleBounds;

Comment 5

[(dormant account)]

> 
> Why copy the visibleBounds into this._visibleRect? Couldn't you just use the
> visibleBounds object itself?

consistency with other code, but Stuart says we should just use the bounds objects for everything.

Gavin raises a point that I want to use the intersection of the viewingRect and the viewportBound for this

Comment 6

[(dormant account)]

Attachment: fixed it

This does the intersect now, but I think WidgetStack is lying cos most of the time the positions don't account for the location bar.

I also: 
a) changed the calculations so they make sense
Before when panning happened, the right/bottom most part of the element would be displayed(ie if it is bigger than the screen) which was highly ridiculous
Now it's possible to jump around links without getting completely confused and disoriented(once you discount horrid performance...I haven't looked at perf yet, but do we have to do this via CSS as opposed to drawing on the canvas directly?)
b)made panning account for zoom. That's right, now we pans in steps relative to the zoom level :)

Gavin, unless you see anything wrong with this code I'd like to commit this and fix the viewport lying in another bug.

Comment 7

[(dormant account)]

Attachment: fix + switch _page* to wsRect + remove some dead code

Comment 8

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

This landed: http://hg.mozilla.org/mobile-browser/rev/d9deda3eb847