703573 - HTML iframe cannot be panned

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Cristian Nicolae (:xti)]

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111118
Firefox/11.0a1 Fennec/11.0a1
Devices: Samsung Galaxy Nexus S
OS: Android 2.3.4

Steps to reproduce:
1. Open Fennec App
2. Browse to http://www.htmlcodetutorial.com/frames/_IFRAME.html
3. Pan the iframe content

Expected result:
The content of the iframe can be panned.

Actual result:
The content of the iframe cannot be panned

Comment 1

[Aaron Train [:aaronmt]]

blocking bug 695448?

Comment 2

[Chris Lord [:cwiiis]]

Really don't know the best way to fix this with our current Java compositor scheme.

One thing we can do;

Get browser.js to inform us of scrollable sub-frames (and their extents) as part of the drawing metadata it provides us, then pass through events in those areas and let browser.js handle scrolling.

I'm not sure how sucky this would feel - it would probably mean sub-frames lag terribly during loading/on intensive pages/when zoomed, etc. and I can't imagine the update performance would be great either (it'd need to upload the region of the sub-frame for every position change) - certainly until we eliminate the required texture upload.

Another thing;

We could just disable overflow and not handle iframes - this is what early iOS/Android did. Not many sites use scrollable iframes, or rely that much on the behaviour of overflow. Whether this is acceptable now that all our competition handle this correctly though, is another question. I would say it isn't, but it's not like the alternative is so great either...

I did bring this up when we discussed the idea of a Java compositor, but apparently it wasn't an issue then?

Maybe pcwalton has more ideas in this area.

Comment 3

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

I think GMail and Google Reader both use scrollable iframes, at least in the versions that are served to Fennec.

Comment 4

[Patrick Walton (:pcwalton)]

Yeah, I figured we'd do one of those two things. iOS (even now) is really bad at subdocument frames too, so we aren't alone here.

The Gmail and Google Reader is unfortunate. We should have some kind of solution here.

Comment 5

[Antonio Gomes (tonikitoo)]

(In reply to Patrick Walton (:pcwalton) from comment #4)
> Yeah, I figured we'd do one of those two things. iOS (even now) is really
> bad at subdocument frames too, so we aren't alone here.

Not sure why you say that. iOS since ages uses a feature called frame-flatten, which makes an inner frame viewport as big as its contents size, making it non-scrollable. It scrolls together with the main frame as one canvas. It breaks some sites, but eliminates the in-region scrolling overhead. I dislike it, personally.

They now also do a nice work on scrollable divs with iOS5. See http://functionsource.com/post/scrollability-can-hopefully-become-a-shim

Comment 6

[Chris Lord [:cwiiis]]

I'd like to go with my first suggestion of how to handle this.

We'll handle the events in the Java and browser.js will have a signal it can send to say "I'm handling viewport updates" when a touch event occurs. In this situation, Java will then continue to process panning/flinging, but instead of updating the viewport, it'll send the events to JS to handle. This will stop us from having multiple versions of the kinetic panning code lying around.

When browser.js sends this signal, Java will undo any scrolling its done (which shouldn't happen often, as there's a drag threshold and Gecko should be able to respond quite quickly, other than during page-load) and hand-off to browser.js to do its thing.

I'd have hoped that sub-frames are usually smaller than the viewport, and we've already shown that Gecko can update at 60Hz in a lot of cases without async scrolling, so this shouldn't be too bad (as long as we can minimise/remove the texture-upload overhead).

I'd like this to depend on bug #697701, as it has code that will make this an easier change to implement.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Add scrolling support for document sub-frames

This patch adds scrolling support in the way I described in comment #6.

It doesn't do the 'undo scroll' step, I'm not sure if we really want that or not (and it can be fixed in another bug if we do).

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 578273 [details] [diff] [review]
Add scrolling support for document sub-frames

I have some review comments below, but after reviewing half of the patch I realized I was either misunderstanding it or it's got a large bug. My initial comments are below, but the big question I have is - is the Ack message an ack for Panning:Override? Or for Scroll:Gesture? It seems like the latter, but that makes the name very confusing - it should be more like Scroll:GestureAck or something. And mOverridePanningAck variable is never reset to false which it definitely should be.

>+
>+        mX.lastDisplacement = mX.displacement;
>+        mY.lastDisplacement = mY.displacement;

lastDisplacement is never used. This can be removed.

> 
>+        public float displacement;
>+        public float lastDisplacement;
>+        private float mSnapDisplacement;

I'd prefer this be named mSnapPosition rather than mSnapDisplacement. The way it's used is as a position, and the displacement is calculated by taking the difference between two positions.

>         // Starts a snap-into-place operation.
>         public void startSnap() {
>             switch (getOverscroll()) {
>             case MINUS:
>-                mSnapAnim = new EaseOutAnimation(viewportPos, viewportPos + getExcess());
>+                mSnapAnim = new EaseOutAnimation(0, getExcess());
>                 break;
>             case PLUS:
>-                mSnapAnim = new EaseOutAnimation(viewportPos, viewportPos - getExcess());
>+                mSnapAnim = new EaseOutAnimation(0, -getExcess());
>                 break;
>             default:
>                 // no overscroll to deal with, so we're done
>                 mFlingState = FlingStates.STOPPED;
>                 return;
>             }
> 
>             mFlingState = FlingStates.SNAPPING;
>+            mSnapDisplacement = 0;

Will mSnapAnim.getPosition() return 0 at this point? (It might throw an exception of some sort). If so, mSnapPosition = mSnapAnim.getPosition() makes more sense, so that it's clear that this value is getting initialized to the initial position of the snap animation, rather than using the magic number "0".

>         private void snap() {
>             mSnapAnim.advance();
>-            viewportPos = mSnapAnim.getPosition();
>+            displacement = mSnapAnim.getPosition() - mSnapDisplacement;

Shouldn't this be += instead of =?

>   observe: function(aSubject, aTopic, aData) {
>-    if (aTopic == "Gesture:CancelTouch") {
>+    if (aTopic == "Gesture:Scroll") {
>+      // If we've lost our scrollable element, return. Don't cancel the
>+      // override, as we probably don't want Java to handle panning until the
>+      // user releases their finger.
>+      if (this._scrollableElement == null) {
>+        sendMessageToJava({ gecko: { type: "Panning:CancelOverride" } });
>+        return;
>+      }

Comment contradicts code.

>-  _highlihtElement: null,
>+  _highlightElement: null,

Makes me hate javascript so much!

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Add scrolling support for document sub-frames (revised)

Hopefully addressed all comments.

I'm not particularly happy with the readability of this code, but hopefully bug #701594 will tidy things up a bit, and then perhaps some follow-up to separate the gesture detection from the actual panning/zooming code.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 579079 [details] [diff] [review]
Add scrolling support for document sub-frames (revised)

From kats' review on pastebin:

1. mOverridePanningAck could be renamed to mOverrideScrollAck, similarly mOverridePanningPending could be mOverrideScrollPending
2. viewportPos in Axis should be renamed mViewportPos since it's private now

-            float excess = getExcess();
-            if (FloatUtils.fuzzyEquals(excess, 0.0f))
-                setFlingState(FlingStates.STOPPED);
-            else
+            setFlingState(FlingStates.STOPPED);
+            if (!disableSnap && !FloatUtils.fuzzyEquals(getExcess(), 0.0f))
                 setFlingState(FlingStates.WAITING_TO_SNAP);

3. This condition should be inverted to "if (disableSnap || FloatUtils....)" for clarity and consistency with the other hunk later.

Comment 12

[Chris Lord [:cwiiis]]

http://hg.mozilla.org/projects/birch/rev/a9ef8b930aab

Comment 13

[Brion Vibber]

Testing 20111207 nightly, I can confirm that this LinkedIn article using an iframe scrolls slightly better, but still has some problems:

http://www.linkedin.com/news?viewArticle=&articleID=952157269&gid=3853987&type=member&item=83091217&articleURL=http%3A%2F%2Fengineering.linkedin.com%2Fnodejs%2Fblazing-fast-nodejs-10-performance-tips-linkedin-mobile&urlhash=s_P3&goback=.gde_3853987_member_83091217&_mSplash=1

Flicking over the main content area quickly still usually pans the entire page, while slowly tapping-and-dragging (sslloowwllyy) pans the iframe.

Comment 15

[Carla Nadastean]

Environment:

Aurora 2012-01-19 Firefox/11.0a2 Fennec/11.0a (2012-01-20)
Nightly 2012-01-19 Firefox/12.0a1 Fennec/12.0a (2012-01-20)
Device:HTC Desire Z Android(2.3.3)

Bug is still reproducible on Aurora and Nightly as originally described in bug description.

Reopening bug

Comment 16

[Doug Turner (:dougt)]

wfm today's nightly.

Comment 17

[Shane Tomlinson [:stomlinson]]

This is NOT working for me in Firefox 21.0 for Android.

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Shane Tomlinson [:stomlinson] from comment #17)
> This is NOT working for me in Firefox 21.0 for Android.

This code has basically been entirely rewritten - the original bug was over a year ago :) If you have issues with iframes and reasonable STR, please open a new bug and cc me and :kats