824472 - Cannot scroll content in an app with a large iframe

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 12/23/2012
Device: Unagi

Steps:

1. Go to http://jds2501.github.com/webapi-permissions-tests/
2. Install the hosted app per testing webapi permissions
3. Launch that app
4. Select view in off-origin iframe
5. Try scrolling the content

Expected:

The content should be scrollable.

Actual:

The content is next to near impossible to scroll.

Comment 1

[Doug Turner (:dougt)]

soft-blocking.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Currently, BrowserElementScrolling doesn't find a cross-document parent scrollable region.
In b2g-desktop build, the content is scrollable using mouse wheel. Maybe we can implement the same logic in BES just like how we handle mouse wheel event in [1].

[1] http://dxr.mozilla.org/mozilla-central/content/events/src/nsEventStateManager.cpp.html#l2793

Comment 3

[Josh Matthews [:jdm]]

Attachment: Attempt to scroll a cross-document parent frame when no other pannable target can be found

This makes the off-origin iframe in Jason's testcase pannable for me. I'm not certain the result is correct, since the scrollbar seems to indicate there's some more unpannable area beneath it.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 699851 [details] [diff] [review]
Attempt to scroll a cross-document parent frame when no other pannable target can be found

Review of attachment 699851 [details] [diff] [review]:
-----------------------------------------------------------------

The logic is correct, but I want to see how it works on multi-level iframe webapps and webpages. :)

Comment 5

[Josh Matthews [:jdm]]

You're right, this isn't working properly on http://www.joshmatthews.net/iframe1.html.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Search for a scrollable element starting from the containing frame of a fully-scrolled element.

This patch works quite a bit better in my tests of http://www.joshmatthews.net/iframe1.html, but it looks like I force it to revert to synchronous scrolling occasionally.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Search for a scrollable element starting from the containing frame of a fully-scrolled element.

It works!

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 701088 [details] [diff] [review]
Search for a scrollable element starting from the containing frame of a fully-scrolled element.

Review of attachment 701088 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/browser-element/BrowserElementScrolling.js
@@ +400,5 @@
> +        // If no scrolling occured, but the scroll target is scrollable,
> +        // we can infer that no further scrolling is possible. If the
> +        // target resides in a frame, we should attempt to scroll the
> +        // containing window now.
> +        if (content.frameElement) {

Could we move the logic into |_findPannable|? We might not be able to scroll the outer iframe if the touchstart is fired on an unscrollable inner iframe, because we'll not generate scrollCallback for an unscrollable iframe.

Comment 9

[Josh Matthews [:jdm]]

We don't have any direction information in _findPannable, so when we check scrollMaxX/scrollMaxY, we don't have any way to decide that we should ignore the scrollable element and return the frameElement instead.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We don't have to care about the scroll direction in |_findPannable|. In fact, we can only find the inner-most scrollable element at the touchstart and climb up the DOM tree after panning happened.
That's why we have the for-loop in |scroll|, right?
http://dxr.mozilla.org/mozilla-central/dom/browser-element/BrowserElementScrolling.js.html?string=browserelementscrolling.js#l388

Comment 11

[Josh Matthews [:jdm]]

Attachment: Traverse containing frames when looking for scrollable content for browser elements.

You're right. This patch is both less complex and works just as well.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 701846 [details] [diff] [review]
Traverse containing frames when looking for scrollable content for browser elements.

Review of attachment 701846 [details] [diff] [review]:
-----------------------------------------------------------------

great, looks good and works well.

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 701846 [details] [diff] [review]
Traverse containing frames when looking for scrollable content for browser elements.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some partial or whole pages will be impossible to scroll.
Testing completed: Manual.
Risk to taking this patch (and alternatives if risky): Very little. It's a laser-focused change.
String or UUID changes made by this patch: None.

Comment 14

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3c7f176f2c

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/823aa4ea5dd4

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e3c7f176f2c