779358 - Panning is too fast for in-process content

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

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.

Comment 1

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

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).

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.1

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 4

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

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 ...

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 7

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

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.

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

Browser has multitouch gesture support. I think it consumes both touch events and mouse events.

Comment 9

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Fix mousemove event handled twice in content panning

Note that this patch still can't pass the drawing app test. gesture_detector.js might also need update.

Comment 10

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

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>?

Comment 11

[Cervantes Yu [:cyu] [:cervantes]]

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 12

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

Comment on attachment 653727 [details] [diff] [review]
Fix mousemove event handled twice in content panning

OK!  Makes sense.  Thanks for the explanation.

Comment 13

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

Comment on attachment 653727 [details] [diff] [review]
Fix mousemove event handled twice in content panning

My review is sufficient for this.  Please land it! :)

Comment 14

[Eric Chou [:ericchou] [:echou]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fca63fc533e2

Comment 16

[David Flanagan [:djf]]

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.

Comment 17

[Cervantes Yu [:cyu] [:cervantes]]

Reopen since the original fix has regression in the gallery app. Handling click shouldn't be changed to use bubbling.

Comment 18

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Fix the regression in the gallery app

Comment 19

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

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!

Comment 20

[Cervantes Yu [:cyu] [:cervantes]]

Got it. Thanks for the quick review.

Comment 21

[Eric Chou [:ericchou] [:echou]]

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

Comment 22

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Fix the regression in the gallery app

Add author to the patch.

Comment 23

[Eric Chou [:ericchou] [:echou]]

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

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8f79f5a6b073
https://hg.mozilla.org/mozilla-central/rev/6bf3ae02890e

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a684417d8536