772422 - Text is selected automatically when panning

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 16.0a1 (2012-07-10)
Device: Galaxy Nexus
OS: Android 4.0.4

Steps to reproduce:
1. Open Fennec
2. Browse to https://forums.craigslist.org/?forumID=76&areaID=1
3. Pan down and up the left frame

Expected result:
After step 3, the pan is performed without auto text selection.

Actual result:
After step 3, the text will be auto-selected. The selection length will be different each time when step 3 is performed.

Comment 1

[:Margaret Leibovic]

This is a regression from bug 732052. I think this happens on all pages, but it definitely happens on http://people.mozilla.com/~mleibovic/test/lorem_ipsum.html.

Comment 2

[Wesley Johnston (:wesj)]

So now, on touchmove events we are storing their capture target here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6533

but in this case, we're catching the touch events in fennec's browser.js, and redispatching mouse events (to trigger selection). I think(?) those mouse events are propagating through and being confused by the information stored by the touch event? cc'ing smaug to know if that even makes sense and if he knows a simple solution. Otherwise, I'll start digging through and trying to figure out where things are off the rails. We may have to use a different set of capturing API's for touch.

Comment 3

[Wesley Johnston (:wesj)]

Wait, this happens even with panning right? So it has nothing to do with our crazy event dispatching. And I have no idea why its causing selection either...

Simple hacky fix idea: don't call PushCurrentEventInfo if we're not capturing. i.e. store a boolean here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6521

to know if we're capturing content. If we aren't, don't bother pushing or popping the current event info.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Wesley Johnston (:wesj) from comment #2)

> but in this case, we're catching the touch events in fennec's browser.js,
So what code where? Does it end up calling preventDefault() or some such to prevent default
handling to happen in C++. (Though, I don't recall if touch events are cancelable.)

Comment 5

[:Margaret Leibovic]

(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to Wesley Johnston (:wesj) from comment #2)
> 
> > but in this case, we're catching the touch events in fennec's browser.js,
> So what code where? Does it end up calling preventDefault() or some such to
> prevent default
> handling to happen in C++. (Though, I don't recall if touch events are
> cancelable.)

This bug happens if you pan the page while text selection is active, so it's happening when you touch somewhere else other than the selection handles.

While text selection is active, there are touch listeners on the handles [1]. Is the presence of those listeners what's messing things up?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1821

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

So the patches in bug 732052 cause touch events to call HandlePress/Drag/Release function in frames. This fixes the problem by having us bail from HandlePress before... something bad happens. Its calling fc->SetMouseDownStat(true) which I suspect causes us to start selecting. We aren't firing any mouse events that I can see in nsPresShell::HandleEvent.

I'm not exactly sure why we don't hit this any time your finger is down (with the patches from bug 732052) either. Still digging to understand this a bit, but hoping that ehsan can tell me if I'm way off base.

Comment 7

[Wesley Johnston (:wesj)]

Note that slider's will still work after this because they have their own handlePress handler.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Can we get a test for this, please.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Test

Test that touch events don't affect the current selection. Fails before. Passes with.

Comment 10

[Cristian Nicolae (:xti)]

Something interesting happens here. Even if the patch was not landed on the latest Nightly, this issue doesn't occur anymore.

--
Firefox 16.0a1 (2012-07-11)
Device: Galaxy Nexus
OS: Android 4.0.4

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

That is probably because bug  732052 was backed out.

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e15fc86db728
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8063abe193f

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d8063abe193f
https://hg.mozilla.org/mozilla-central/rev/e15fc86db728

Comment 14

[Aaron Train [:aaronmt]]

Verified on inbound (07/13), GN (4.1.1)

Comment 15

[Paul Feher]

Verified on:
Nightly Fennec 17.0a1 (2012-08-28)
Aurora Fennec 16.0a2 (2012-08-27)
Device: HTC Desire Z 
OS: Android 2.3.3