1119834 - Text selection action bar is shown on div element containing no text

Status: RESOLVED FIXED

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

Description

[Geoff Brown [:gbrown]]

Long press on a div element brings up the text selection action bar, even if the div contains no text.

Consider http://hg.mozilla.org/mozilla-central/annotate/7c03738e7a95/mobile/android/base/tests/robocop_boxes.html -- a page of div "boxes" of different colors. Long press anywhere on the page brings up the action bar, which seems wrong.

I have verified that adding div to the list of exclusions in SelectionHandler.canSelect -- http://hg.mozilla.org/mozilla-central/annotate/b3f84cf78dc2/mobile/android/chrome/content/SelectionHandler.js#l252 -- prevents the action bar from being invoked, but I wonder if it's more complicated: What if the div contains text? Would canSelect see the text element, or the div?

I suppose there is a similar issue for span....and maybe other elements??


Of course robocop_boxes.html is used in several robocop tests, and this behavior is part of the cause of intermittent failures in bug 851861.

Comment 1

[Mark Capella [:capella]]

Attachment: bug1119834.diff

Interesting, the long-press on this page does produce a valid nsISelection object, whose anchor/focus nodes wrap a single character |LF|, which our reasonability logic accepts.

However, testing the selection text after toString().trim() produces a correct determination.

Comment 2

[Mark Capella [:capella]]

gbrown: So the random orange failures in most tests such as |testFlingCorrectness|, seems like it might be dur to triggering LONG-PRESS initiating text SelectionHandler by it's dragSync() calls [0], which use a default drag duration of 1s [1] ... or perhaps the final sleep() in the method [2] ?

[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testFlingCorrectness.java?rev=523e67cf4b9e&mark=26-27#23
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/MotionEventHelper.java?rev=082c445306e8&mark=97-97#86
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/MotionEventHelper.java?rev=082c445306e8&mark=71-74#54

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Mark Capella [:capella] from comment #2)
> gbrown: So the random orange failures in most tests such as
> |testFlingCorrectness|, seems like it might be dur to triggering LONG-PRESS
> initiating text SelectionHandler by it's dragSync() calls [0], which use a
> default drag duration of 1s [1] ... or perhaps the final sleep() in the
> method [2] ?

I agree in general, but I'm not sure I understand how the dragSync down/move/up operations translate to a long-press. That 1s drag duration should be chopped up into 20 = DRAG_EVENTS_PER_SECOND events, or approx one event every 50 ms, which should be a long way away from a long-press. Lots of latency in event processing in the test environment? We always suspect that, but I'm not sure how to verify that for this case. And if that is the cause of translating a drag into a long-press, what can we do about it? Your patch might be sufficient and may be the best we can do.

Comment 4

[Mark Capella [:capella]]

The dragAsync() [1] method does a MotionEvent down(), followed by enough move()'s to take 1000ms, then sleeps for another 1000ms, followed by a final move()/up() pair, for an estimated minimum of 2000ms.

I repro-ed the failure in this test I pushed to try [2] (with logging / interesting bits tagged with XYZZY). The entire dragAsync() method takes around the expected duration of 2578ms, and we see a |longpress| triggered around 631ms into the drag-loop.

I confirmed we're using Androids DEFAULT_LONG_PRESS_TIMEOUT of 500ms, so this is reasonable, and seems to explain why Bug 851861 has been an issue for so long.

Double-check my work. This may patch the issue, but the tests might should be corrected also.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/MotionEventHelper.java?rev=082c445306e8&mark=62-80#54
[2] https://tbpl.mozilla.org/php/getParsedLog.php?id=56357547&tree=Try&full=1

Comment 5

[Geoff Brown [:gbrown]]

That's a cool try run - thanks. This is the bit I like:

01-12 20:10:03.494 D/XYZZY   ( 2227): [3210238204044] testFlingCorrectness: First dragSync() Starts
01-12 20:10:03.514 D/XYZZY   ( 2227): [3210247580178] MotionEventHelper: dragAsync().run() Starts
01-12 20:10:03.514 D/XYZZY   ( 2227): [3210253842745] MotionEventHelper:    Drag Starts
01-12 20:10:03.514 D/RobocopMotionEventHelper( 2227): Triggering down at (10.0,150.0)
01-12 20:10:03.554 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,150.0)
01-12 20:10:03.624 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,145.0)
01-12 20:10:04.103 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,140.0)
01-12 20:10:04.143 D/XYZZY   ( 2227): [3210885116897] JavaPanZoomController: onLongPress() Starts
01-12 20:10:04.153 D/XYZZY   ( 2227): [3210897933429] JavaPanZoomController: onLongPress() Finishes
01-12 20:10:04.230 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,135.0)

If I translate the logcat timestamps to time deltas, I see:

01-12 20:10:03.514 D/RobocopMotionEventHelper( 2227): Triggering down at (10.0,150.0)
            +0.040 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,150.0)
            +0.070 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,145.0)
            +0.479 D/RobocopMotionEventHelper( 2227): Triggering move to (10.0,140.0)

It looks like 

   Thread.sleep(1000L / DRAG_EVENTS_PER_SECOND)

is usually successfully pausing for its target of 50 ms (40, 70 ms) between moves, but sometimes missing the mark substantially (479 ms). I assume the 479 ms pause is responsible for hitting the 500 ms long press timeout. How can we fix this?

(On the other hand, each move() MotionEvent has timestamps, which are calculated and not based on real time -- do those timestamps not matter to the long press timeout?)

Comment 6

[Mark Capella [:capella]]

I wonder if we should provide something like a simpleDrag() method for some test cases, where we don't preserve the previous downTime value we pass to move(), instead always doing something like:

MotionEvent.obtain(SystemClock.uptimeMillis(), SystemClock.uptimeMillis(), MotionEvent.ACTION_MOVE, fooX, fooY, 0);

So instead of one long swipe, Android we see a series of little swipes (?) and avoids the longpress trigger.

Other than that, maybe there's a way to override our longpress() method during testing in certain cases (?) etc.

I'm coming into this are from left-field so I think I'll cc: mcomella.

Comment 7

[Geoff Brown [:gbrown]]

:kats may have some ideas too!

:kats -- Any thoughts on the last few comments? We are puzzling over why the robocop MotionEventHelper's drag is sometimes triggering long press and how we might avoid that.

Comment 8

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

Oh neat, nice work figuring out the problem there! Clearly the long-press is firing when it's not supposed to. When I wrote that code I assumed that since the touchmove events had appropriate timestamps it would never trigger a longpress. However if you look at the GestureDetector code in Android what it does is that as soon as it gets the touch-down event it schedules the long-press event [1], and then later if it gets a sufficiently large touch-move event within the timeout it will cancel the long-press [2].

Based on the data in comment 5, I would guess that the first two move events are not "sufficiently large" to cancel the long-press (they only move a total of 5 pixels). So the way to combat that would be to either reduce the touch slop by fiddling with [3], or increasing the size of the first move event to be larger than the touch slop. This is probably the easier approach, but it might mean having to scroll back and forth a bit programatically.

The other problem that might happen is that the first move event doesn't get delivered in time at all, because of random other junk running on the UI thread. One way to deal with that would be to deliver the first move event right after the down event, without any sleep in between. Still the drag is running on a non-UI thread that's not guaranteed to work but it might help some. I can't think of anything else at the moment that will let us keep the async scroll simulation and be guaranteed not to trigger the long-press detector.

[1] https://github.com/android/platform_frameworks_base/blob/af1785f0b54bff4fcc6218619e34b9861e129cb9/core/java/android/view/GestureDetector.java#L546
[2] https://github.com/android/platform_frameworks_base/blob/af1785f0b54bff4fcc6218619e34b9861e129cb9/core/java/android/view/GestureDetector.java#L573
[3] https://github.com/android/platform_frameworks_base/blob/af1785f0b54bff4fcc6218619e34b9861e129cb9/core/java/android/view/ViewConfiguration.java#L310

Comment 9

[Mark Capella [:capella]]

final try: https://tbpl.mozilla.org/?tree=Try&rev=edab27f8ba77

Comment 10

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/260eb4adf6c6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/260eb4adf6c6