686701 - Calling preventDefault on touchevents does not prevent chrome panning

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Calling preventDefault on touchstart or the first touchmove event should prevent panning the chrome. At some point this has regressed and no longer works.

This works fine on Aurora on my tablet.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase1

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

Comment 3

[Martijn Wargers (dead)]

I could reproduce it with the testcases on desktop Fennec, but suddenly, after a certain point, I couldn't reproduce it anymore.

Anyway, I did get a regression range before I couldn't reproduce it anymore. I got a regression range between 2011-08-24 and 2011-08-25:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-08-24+03%3A00%3A00&enddate=2011-08-25+09%3A00%3A00

Changeset of 2011-08-25 build:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=e58e98a89827

I guess it could be a regression from bug 659350, but really weird that I suddenly can't reproduce it anymore!

Comment 4

[Wesley Johnston (:wesj)]

Just to be careful Martijn, we allow panning chrome in two situations:

1.) If any chrome is showing when the pan starts (i.e. if the urlbar is showing, you can pan the sidebars in or out). I'd like to change this so that we only pan the chrome out of the way, but not in.

2.) If the pan starts at the top of the screen.

Comment 5

[Martijn Wargers (dead)]

Yeah, sorry, I should have mentioned that. I took care that I wasn't falling in any of the situations mentioned in comment 4. For some reason, I can't reproduce this bug anymore.
Anyway, perhaps someone can confirm the regression range in comment 3, or else the regression range is bogus, I guess.

Comment 6

[Matt Brubeck (:mbrubeck)]

On desktop Linux fennec, I can reproduce this bug in phone mode, but not in tablet mode.

Comment 7

[Matt Brubeck (:mbrubeck)]

In ContentTouchHansder.updateCanCancel, the "bcr.top == 0" check is failing because bcr.top is between 0 and 1 (for example, 0.883331).  For some reason the browser is not positioned all the way at the top of the screen.

Comment 8

[Wesley Johnston (:wesj)]

Hmm... on my desktop this only affects honeycomb. Honeycomb in phone mode which is an odd combination.

Comment 9

[Matt Brubeck (:mbrubeck)]

The 0.883331 seems to be related to the #toolbar-main min-height in themes/honeycomb/browser.css.  If I change the min-height to an exact px length, the bug goes away.

When the bug is present, then _panChrome fails to pan the top toolbar away completely.  It calls _panScroller with doffset.y = 1, but the position of the page-scrollbox does not change, possibly because there is not a whole pixel left for it to scroll (and we can't pass non-integer values to nsIScrollBoxObject.scrollBy).

Comment 10

[Wesley Johnston (:wesj)]

I'm going to try to switch to using "scrollTo(0,0)" when we hit this situation. Trick is just finding a good clean place to stick that (it would be nice if it worked for all scrollboxes and not just this one) but as I mentioned above, its a weird situation.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I've been floundering around trying to force this to do something all day, and at the end decided that what I really want is for preventDefault to not prevent panning the urlbar/sidebars off screen, but DO allow them to prevent panning things back on. So that's what this does.

I'd be fine waiting for madhava or someone to approve this. There's a build at:
http://people.mozilla.com/~wjohnston/fennec-touchevents.apk

I'll add madhava/briandils to get their feedback

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

What's the status here? We have an r+ on the patch.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sorry. Slipped off my radar. Worry for me is that users will not be able to pan in the sidebars and will think something is broken? I think we're going to add a small escape border on the edges of these pages. 1/4"?

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 565279 [details]
Patch v2

This looks okay to me, but I think we should try setting both safetyX and safetyY much lower, and expect users to swipe from offscreen.  In bug 690739 I found that 5px at 160dpi (0.8mm, or 7.5/240") was enough to catch swipes from the edge of the screen to make sidebars visible.

(But maybe this will be more difficult on phones without glass that extends past the edge of the screen.  On the other hand, there's always the menu button.)

Comment 15

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/13115601c9ec

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13115601c9ec

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Not tracking. Doesn't show up in our shipping configurations.