775448 - Make synchronous-scrolling fallback for subframes more graceful

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If you load http://people.mozilla.com/~cjones/scrolling.html and try to scroll the subframes, you'll see that it works but the UX is bad.  The most reliable way to scroll the frames is by fooling the async pan/zoom gesture detector.  This stinks.

Instead, when we fall back on sync panning, we should send a message to the async pan/zoom controller that causes it to cancel animations.

Suggest blocking basecamp for UX badness, but we can let experience / data tell.  Some major sites use scrolled subframes, like google reader.

Comment 1

[Andrew Overholt [:overholt]]

Brad says we shipped Firefox for Android with similar behaviour.  Not a blocker for now.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Disable async scrolling when we detect a scrollable subframe

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Add gesture cancelling support during sync subframe scrolling

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 647277 [details] [diff] [review]
Disable async scrolling when we detect a scrollable subframe

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js
>--- a/dom/browser-element/BrowserElementScrolling.js
>+++ b/dom/browser-element/BrowserElementScrolling.js
>@@ -38,16 +38,23 @@ const ContentPanning = {
> 
>   onTouchStart: function cp_onTouchStart(evt) {
>     this.dragging = true;
>     this.panning = false;
> 
>     let oldTarget = this.target;
>     [this.target, this.scrollCallback] = this.getPannable(evt.target);
> 
>+    if ((this.target != null ||
>+        this.scrollBack != null) &&

This is obviously a typo, but I don't think you need to check this
anyway.  Evidence would suggest so too ;).

>+        ContentPanning._asyncPanZoomForViewportFrame) {

Please add a comment describing the logic here.

>+      var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+      os.notifyObservers(docShell, 'sync-panzoom-fallback', null);

What we're really doing here is preventing default pan/zoom, so I'd
prefer we called this "cancel-default-pan-zoom".  (Saying "prevent
default" is good in that it's reminiscent of the DOM semantics we're
badly aping, but it's also confusing in that we don't quite implement
them.  So compromise with "cancel".)

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    sync SyncPanZoomFallback();

Here and below, I'd rather use the active "CancelDefaultPanZoom" name.
This should go directly on PRenderFrame, and we should send the
message through tabChild->mRemoteFrame->SendCancelDefaultPanZoom().

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+  } else if (!strcmp(aTopic, "sync-panzoom-fallback")) {
>+    nsCOMPtr<nsIDocShell> docShell(do_GetInterface(aSubject));

This should be a QueryInterface.

The rest looks fine.  Would like to see next version.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Disable async scrolling when we detect a scrollable subframe

Comment 6

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2afc5b5e538

Comment 7

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10695750e4e9

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a2afc5b5e538
https://hg.mozilla.org/mozilla-central/rev/10695750e4e9

Comment 10

[Doug Sherk (:drs) (inactive)]

This was backed out.

Comment 11

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/282bfd3e56e0

Comment 12

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/174a57bbcb22

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/282bfd3e56e0
https://hg.mozilla.org/mozilla-central/rev/174a57bbcb22