Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make synchronous-scrolling fallback for subframes more graceful

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: drs)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:?, blocking-basecamp:-)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Brad says we shipped Firefox for Android with similar behaviour.  Not a blocker for now.
blocking-basecamp: ? → -
(Assignee)

Comment 2

5 years ago
Created attachment 647277 [details] [diff] [review]
Disable async scrolling when we detect a scrollable subframe
Assignee: nobody → bugzilla
Attachment #647277 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
Created attachment 647279 [details] [diff] [review]
Add gesture cancelling support during sync subframe scrolling
Attachment #647279 - Flags: review?(jones.chris.g)
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.
Attachment #647277 - Flags: review?(jones.chris.g)
Attachment #647279 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 648633 [details] [diff] [review]
Disable async scrolling when we detect a scrollable subframe
Attachment #647277 - Attachment is obsolete: true
Attachment #648633 - Flags: review?(jones.chris.g)
Attachment #648633 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2afc5b5e538
(Assignee)

Comment 7

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

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a2afc5b5e538
https://hg.mozilla.org/mozilla-central/rev/10695750e4e9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 10

5 years ago
This was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/282bfd3e56e0
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/174a57bbcb22
https://hg.mozilla.org/mozilla-central/rev/282bfd3e56e0
https://hg.mozilla.org/mozilla-central/rev/174a57bbcb22
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.