Panning is too fast for in-process content

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: cyu)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

(Whiteboard: [LOE:M])

Attachments

(3 attachments, 1 obsolete attachment)

STR
 (1) Load the settings app (currently blacklisted for OOP)
 (2) Place finger on "Personalization"
 (3) Pan upwards to top of screen without lifting finger

It's expected that your finger will finish on or very near the same document pixel it started on.  Instead, my finger finishes on the "Brightness" list item.

This does *not* happen with OOP content.  I suspect we end up getting two pan/zoom listeners for the same content, somehow, and they make us pan twice as fast.
Suggest blocking because it's a very user-visible difference in panning behavior across different contexts (homescreen different than settings different than browser-async different than browser-sync).
blocking-basecamp: --- → ?

Updated

7 years ago
Assignee: nobody → cyu

Updated

7 years ago
blocking-basecamp: ? → +
Assignee

Updated

7 years ago
Whiteboard: [LOE:M]
One mousemove event is handled twice. shell.js creates one <iframe mozbrowser> to load the homescreen and homescreen loads the settings app. So the mousemove event is handled first BrowserElementChild.js in loading the settings app and then bubbles up to the iframe to load the homescreen. I guess if we have a deeper iframe chain then we will see scrolling going extremely fast!

If I add evt.stopPropagation() at the end of function onTouchEnd in BrowserElementScrolling.js then the event doesn't bubble up once scrolling happens and scolling speed is correct. But I am not sure if this breaks other things.

Tim suggested a safer method: checking whether we are in the iframe where the event target lies in, so the event does bubble up, but only one iframe handles it. I need to figure out how to do this.
Posted patch wip-0.1Splinter Review
This wip use device's dpi while panning. This seems to resolve most of the issue in the Settings App.

There is still an issue with floating units since content scrolling should be integers. So this wip needs to be improve to make sure to add the value that has not been scrolled in the next scrolling step.
Comment 2 suggests the problem is that content receives multiple input events for the same real user input.  That seems consistent with the fact that (synchronous) panning works fine in content process.

So not sure what you're tuning here ...
To fix this problem, we may add a check in ContentPanning.onTouchMove(evt) and don't scroll if content !== evt.view.top. So only the containing iframe of the event target will scroll.

Another way to fix this is adding evt.stopPropagation(), and this might be a better and cleaner way to fix.

Because currently these mouse events are handled during capturing. The outermost iframe element handles the mouse event first. So if a page implements a drawing app, the page does scroll even if a canvas element handles the event and stops propagation. To reproduce, open this page: http://www.williammalone.com/articles/create-html5-canvas-javascript-drawing-app/#demo-complete and draw something in the canvas. You will see the page scrolls during drawing. The events should be handled in bubbling phase.
Sorry, the URL in comment #5 is not a good example because the canvas doesn't stop the event from propagation.

This is the correct example: http://ppt.cc/zntA
Bug 784298 is opened for tracking the event bubbling issue.
Oh!  I bet this is a regression from bug 775403.

The two apps that will be in-process for v1 are the system app and the browser.  Do either of those consume both touch events *and* mouse events?  I know both have some scrollable displays, but if neither requires multitouch, we should be able to to get away with just listening for mouse events.  Then we can blocking- this.
Blocks: 775403
blocking-basecamp: + → ?
Browser has multitouch gesture support. I think it consumes both touch events and mouse events.
Note that this patch still can't pass the drawing app test. gesture_detector.js might also need update.
Attachment #653727 - Flags: feedback?(jones.chris.g)
Comment on attachment 653727 [details] [diff] [review]
Fix mousemove event handled twice in content panning

Hmmm ... I'm not exactly sure why this would work.  Are we dispatching events in one <iframe mozbrowser>, sending a message to the child-side content, but then also dispatching from an outer <iframe mozbrowser> and sending the message again to the same child side?  And this prevents the event from propagating to the enclosing <iframe mozbrowser>?
I don't really understand what you mean by "sending a message to the child-side content". Do you mean message passing between BrowserElementParent and BrowserElentChild? I didn't see messages passed during scrolling.

In short, the problem is one nsDOMMouseEvent handled by 2 <iframe mozbrowser>. In BrowserElementScrolling.js, mouse event is registered to use capture. So during scrolling the outer iframe will handle the event first. It gets the event target (e.g. some element in settings app), finds the containing HTML body element, and scroll. The mouse event propagates to the inner iframe in one dispatch (nsEventDispatcher::Dispatch()), and the event is handled in the same way again.

The patch changes the addEventListener calls to use bubbling so the inner iframe (e.g. the settings app) has the chance to handle the event first. If the event is really handled, it is stopped from propagating to the outer iframe.
Comment on attachment 653727 [details] [diff] [review]
Fix mousemove event handled twice in content panning

OK!  Makes sense.  Thanks for the explanation.
Attachment #653727 - Flags: feedback?(jones.chris.g) → feedback+
Comment on attachment 653727 [details] [diff] [review]
Fix mousemove event handled twice in content panning

My review is sufficient for this.  Please land it! :)
Attachment #653727 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fca63fc533e2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
In the Gallery app, when I swipe up to scroll through the thumbnails, the panning works fine, but then when I lift my finger, I get a click event on the thumbnail it was over, and the gallery switches into single-photo mode to view that photo.  This means that I cannot actually scroll down to see photos that are off the screen.

I don't know that this bug caused it, but I'm guessing that it did.
Reopen since the original fix has regression in the gallery app. Handling click shouldn't be changed to use bubbling.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #655304 - Flags: review?(jones.chris.g)
Comment on attachment 655304 [details] [diff] [review]
Fix the regression in the gallery app

Cervantes, in the future let's file new bugs for new problems that come up.

Thanks!
Attachment #655304 - Flags: review?(jones.chris.g) → review+
Got it. Thanks for the quick review.
m-i:  https://hg.mozilla.org/integration/mozilla-inbound/rev/8f79f5a6b073

However, author name was wrong due to the original patch not have the "user name" line. So backed it out.

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf3ae02890e
Add author to the patch.
Attachment #655304 - Attachment is obsolete: true
Attachment #655522 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a684417d8536
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.