1218386 - Page scroll is not prevented on touch screen on the right edge

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Istvan Halmen]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36
Firefox for Android

Steps to reproduce:

Trying to disable page scroll on touch screen by calling preventDefault on the touchmove event.

Tested on:
Android 6.0 on Google Nexus 5
Firefox 41.0.2



Actual results:

If the gesture was started on the right edge of the screen, the scroll is not prevented, and the touchmove event is not firing at all.

Also, even if page scroll is prevented, it does not prevent the show/hide of the address bar (unless full screen navigation is turned off), which makes hard to use advanced touch widgets, e.g. scrollers like this: http://demo.mobiscroll.com/datetime/date/


Expected results:

If preventDefault is called on the touchmove event, any kind of page scroll should be disabled.

Comment 1

[Markus Stange [:mstange]]

Kats, do we respect preventDefault() on touchmove events?

Comment 2

[Istvan Halmen]

(In reply to Markus Stange [:mstange] from comment #1)
> Kats, do we respect preventDefault() on touchmove events?

preventDefault() is respected correctly on the touchmove event. The problem is that if the gesture is started close to the right edge of the screen, the touchmove event is not even fired.

Comment 3

[Markus Stange [:mstange]]

I see, interesting. I don't know much about Android / touch event handling.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I'm not able to reproduce the issue on my nexus 4. Do you see it on any other device or only the nexus 5?

As for the toolbar scrolling on and off, that is intentional - without that the page would be able to lock the user into the page so that they wouldn't be able to escape, which is a security issue.

Comment 5

[Istvan Halmen]

Thanks for answering.

1. So far I could not reproduce it on other devices, only on Nexus 5. I'll try a few more devices and let you know if it occurs on other devices as well.

2. I understand the security issue. Interestingly iOS Safari and Google Chrome allows this behavior. However if this has to remain this way, I would expect that the address bar appears and disappears without scrolling the page content (something like the old stock browser does on Android 4.x).
The other problem with the current solution is, that nothing indicates that the address bar appeared or not, window.innerHeight remains the same, although the size of the actual viewport is changed. So if I try to position my widget to the bottom (with absolute positioning: widget.style.top = window.scrollY + window.innerHeight - widget.offsetHeight), the bottom of the widget flows out of the viewport, if the address bar is visible. Fixed positioning works as expected.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Istvan Halmen from comment #5)
> 2. I understand the security issue. Interestingly iOS Safari and Google
> Chrome allows this behavior. However if this has to remain this way, I would
> expect that the address bar appears and disappears without scrolling the
> page content (something like the old stock browser does on Android 4.x).
> The other problem with the current solution is, that nothing indicates that
> the address bar appeared or not, window.innerHeight remains the same,
> although the size of the actual viewport is changed. So if I try to position
> my widget to the bottom (with absolute positioning: widget.style.top =
> window.scrollY + window.innerHeight - widget.offsetHeight), the bottom of
> the widget flows out of the viewport, if the address bar is visible. Fixed
> positioning works as expected.

As of bug 1180295 (which is in Firefox 43 and up) the behaviour has been changed to accomodate the issues you describe. In particular, when the addressbar goes on and off, the page content doesn't scroll. Also after the addressbar is moved on or off, the page receives a resize event and the window.innerHeight is adjusted to reflect the actual visible content height. Hopefully that will address your concerns.

Comment 7

[Istvan Halmen]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to Istvan Halmen from comment #5)
> > 2. I understand the security issue. Interestingly iOS Safari and Google
> > Chrome allows this behavior. However if this has to remain this way, I would
> > expect that the address bar appears and disappears without scrolling the
> > page content (something like the old stock browser does on Android 4.x).
> > The other problem with the current solution is, that nothing indicates that
> > the address bar appeared or not, window.innerHeight remains the same,
> > although the size of the actual viewport is changed. So if I try to position
> > my widget to the bottom (with absolute positioning: widget.style.top =
> > window.scrollY + window.innerHeight - widget.offsetHeight), the bottom of
> > the widget flows out of the viewport, if the address bar is visible. Fixed
> > positioning works as expected.
> 
> As of bug 1180295 (which is in Firefox 43 and up) the behaviour has been
> changed to accomodate the issues you describe. In particular, when the
> addressbar goes on and off, the page content doesn't scroll. Also after the
> addressbar is moved on or off, the page receives a resize event and the
> window.innerHeight is adjusted to reflect the actual visible content height.
> Hopefully that will address your concerns.

That sounds great. So I consider the address bar issue as solved.

Regarding the original issue: it is also reproducible on Samsung Galaxy S5 / Android 5.0 / Firefox 41.0.2.

I could also reproduce it on Nexus 5 with Firefox 42beta.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Can you attach a video that demonstrates the problem? That might expose some hidden assumptions in the steps to reproduce. Thanks!

Comment 9

[Istvan Halmen]

Here is the video: https://dl.dropboxusercontent.com/u/34664815/firefox-bug.mp4

When swiping on the middle of the screen, only the address bar is scrolling. When swiping on the right edge, the whole page begins to scroll.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

So turns out I can reproduce this on my nexus 4, but only if I start my touch on the rightmost column of pixels of the screen. That is, the touchstart event dispatched needs to have a x=768. I suspect it's not getting delivered to the content but is instead going to the xul document or something. I can dig into it a bit more.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Looks like it doesn't even got to the xul document. I believe the problem is the rounding at [1]. Android sends us touch points in floats, and in this case I see x coordinates of 767.5 which gets rounded to 768. However on a screen with width 768, the legal values are [0,768) so 768 is actually an improper value. It goes through the Java code just fine (allowing panning) but then gets thrown out in gecko so content never sees it. Replacing the rounding with a truncation to int (or floor) fixes the problem.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java?rev=d9882266e0b3#337

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 13

[Istvan Halmen]

Great, thanks for looking into this!

Comment 14

[nesterran]

Great, thanks for the fast fix ;) !

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8680665 [details] [diff] [review]
Patch

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

I think using Math.floor() probably makes it more clear what you want to do here, but hopefully we're nuking this code soon anyway :)

Comment 16

[Kartikaya Gupta (email:[e-mail])]

This code is going to live on with APZ; I can add Math.floor but that returns a double too so I'll have to keep the cast to int.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/12bb0d1c9024

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12bb0d1c9024

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/12bb0d1c9024