Closed Bug 1175282 Opened 6 years ago Closed 5 years ago

[Fennec-APZ] Unable to scroll outside scrollable subframe after zooming on a non-scrollable document

Categories

(Core :: Panning and Zooming, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

STR:
- Build Fennec with --enable-android-apz
- On a Nexus 4 load people.mozilla.com/~kgupta/griddiv.html
- Zoom in so that the root page is scrollable
- Try to scroll the page by putting your finger outside the scrollable div and dragging

Expected:
- scroll happens

Actual:
- scroll doesn't happen

I investigated this and figured out what was going on. The layer tree looks like this:

ContainerLayer A for root XUL document (with APZ)
  Thebes Layer B
  ContainerLayer C for content document (with APZ)
    Thebes Layer D
    Thebes Layer E
    Thebes Layer F for subframe (with APZ)

Touching outside the subframe hits thebes layer E, which in the APZ code, correctly picks the APZ from C as the target. However, the main-thread code that calls SetTargetAPZC changes the target to the APZ from A. This results in a failure to scroll when dragging, because the touchmoves are getting processed by A instead of C.

The reason the main-thread code finds A as the target is that the RCDRSF returns false for WantAsyncScroll(), and so the GetScrollableAncestorFrame call at [1] returns null and the code below that returns the root XUL document's root element's guid. In various places in the code we treat the RCDRSF specially and I think we need to do so here as well. Specifically, even if WantAsyncScroll() returns false we should treat the RCDRSF as async-scrollable, at least on platforms where we support zooming (because if it can be zoomed, it can probably be scrolled after that).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=24bd667a035a#544
Attached patch PatchSplinter Review
This fixes the problem. I'm not sure if this code should be pushed inside WantAsyncScroll() but I'm leaning towards no since there are other things that call WantAsyncScroll() and it doesn't look like we want this behaviour everywhere.
Also this patch should make the code to handle a null scrollAncestor from bug 1156401 unnecessary. It might be worth leaving that in anyway.
OS: Gonk (Firefox OS) → Android
Comment on attachment 8623296 [details] [diff] [review]
Patch

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

Any thoughts on this? I can't think of anything better but I also feel like we should somehow refactor the code to reduce the number the places we do these special-casing checks.
Attachment #8623296 - Flags: review?(botond)
Comment on attachment 8623296 [details] [diff] [review]
Patch

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

First, this looks like this is going to regress bug 1139464, where the root content document *in the parent process* (yes, there is one...) was unnecessarily layerized. So we at least need to restrict this to the child process on B2G.

Second, for Fennec this means that the root content document will always be layerized (in addition to the root display document), even if:
  (1) the page is non-scrollable and non-zoomable
  (2) the page is non-scrollable at initial zoom, is zoomable, but not currently zoomed
Can we do something smarter to avoid layerization in the above two cases? (1) at least should be doable; (2) might be more tricky.
Attachment #8623296 - Flags: review?(botond)
I might be missing something but I didn't think this would impact layerization at all. The new code I added is contained inside the |aFlags & SCROLLABLE_ONLY_ASYNC_SCROLLABLE| condition, and that flag is only passed from one call site, in APZCCallbackHelper. Arguably this makes the function more confusing, but it shouldn't affect layerization at least.
Oh wait I see what you mean now. Let me think about it a bit more...
With the patches on bug 1201529 the STR from comment 0 work as expected.
Status: NEW → RESOLVED
Closed: 5 years ago
Depends on: 1201529
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.