1580669 - Dragging text selection markers selects the wrong text on Android

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Emily Kager [:ekager]]

Copied from: https://github.com/mozilla-mobile/fenix/issues/5037

Steps to reproduce

1-Launch Fenix
2-Go to any website for example: https://en.wikipedia.org/wiki/Main_Page
3-long press on any paragraph to select a word and try to slide the selector bubble to select more words from the article.

Expected behavior

Select word by word or maybe by letters it depends on how fast you move the selector bubble

Actual behavior

when long press on a word it will be selected but when trying to select more words the selector goes to the other direction, it's quite impossible to select a sentence.

Device information

Huawei P8 lite 2017

• Android device: Android 8.0
• Fenix version:
  Nightly 190830 06:05 (Build #12420615)
  📦: 10.0.1, a573b4116
  🦎: 70.0a1-20190826094255

Trying to select the paragraph
From "when" to "sentence."
Gif of Fenix behavior:
https://user-images.githubusercontent.com/50635951/64729857-0a132100-d4ac-11e9-823b-28200e639b52.gif

Comment 1

[Jan Henning [:JanH]]

The Fennec versions-that-never-will-be (i.e. a recentish local mozilla-central build) behave similarly, as does the geckoview_example_app.
ESR68-Fennec on the other hand still seems to work fine as far as I can tell.

Comment 2

[Chris Peterson [:cpeterson]]

I can reproduce this bug in Fenix and the Reference Browser, but not Fennec 68 or Focus (GeckoView 69). That suggests this bug is a regression in 70.

Ting-Yu, does this look like a Layout bug? Is the Gecko Layout code responsible for positioning the blue text selection markers? Were there any text selection changes in Gecko 70 that might have caused this regression?

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Chris Peterson [:cpeterson] from comment #2)

I can reproduce this bug in Fenix and the Reference Browser, but not Fennec 68 or Focus (GeckoView 69). That suggests this bug is a regression in 70.

Ting-Yu, does this look like a Layout bug? Is the Gecko Layout code responsible for positioning the blue text selection markers? Were there any text selection changes in Gecko 70 that might have caused this regression?

This reminds me of bug 1540545. AccessibleCaret itself isn't change for a while. A regression range would be very helpful.

Botond, is there any hit test change recently that might cause this?

Comment 5

[Botond Ballo [:botond]]

Bug 1565512 is a hit testing change affecting Android (including text selection, fixing bug 1542832) that landed in 70, but it was uplifted to 69.

Bug 1540545 is another one but it landed in 68.

If this is really a regression in 70, we should get a regression window.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

If this is really a regression in 70, we should get a regression window.

Note that I discovered a way to get platform regression windows on mobile in the 68-70 range which I documented in bug 1523514 comment 12.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)

Note that I discovered a way to get platform regression windows on mobile in the 68-70 range which I documented in bug 1523514 comment 12.

I used this method to get a regression window and got:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2ea007508cd3e7a3431b74dfc8b52af6dceb40bb&tochange=0b8b9de44b8bcff5180b539c7c920e0f289100d0

which is bug 1545393. Makoto, could you have a look?

Comment 8

[Timothy Nikkel (:tnikkel)]

Is there a reason we wanted to enable that pref?

Comment 9

[Botond Ballo [:botond]]

If I had to guess: synthesized mouse move events may not undergo all the same coordinate transformations as native mouse move events, and so they might cause different (wrong) coordinates to arrive at AccessibleCaret.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)

If I had to guess: synthesized mouse move events may not undergo all the same coordinate transformations as native mouse move events, and so they might cause different (wrong) coordinates to arrive at AccessibleCaret.

(This is a theory for how enabling that pref causes this regression. I don't know what the motivation is for enabling that pref in the first place.)

Comment 11

[Makoto Kato [:m_kato]]

Bug 1545393 is that mouse event (mousemove) sampling rate change to same rate of Desktop. I guess that root cause is another reason that is not flush frame during mousemove?

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Makoto Kato [:m_kato] from comment #11)

Bug 1545393 is that mouse event (mousemove) sampling rate change to same rate of Desktop. I guess that root cause is another reason that is not flush frame during mousemove?

Bug 1545393 enables synth mouse moves (sending a mouse move after the page contents change via reflow or scrolling so the mouse cursor gets updated) on mobile. Is there a reason we want that on mobile at all? What does it give us? AFAIK there is no mouse cursor on mobile.

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Timothy Nikkel (:tnikkel) from comment #12)

(In reply to Makoto Kato [:m_kato] from comment #11)

Bug 1545393 is that mouse event (mousemove) sampling rate change to same rate of Desktop. I guess that root cause is another reason that is not flush frame during mousemove?

Bug 1545393 enables synth mouse moves (sending a mouse move after the page contents change via reflow or scrolling so the mouse cursor gets updated) on mobile. Is there a reason we want that on mobile at all? What does it give us? AFAIK there is no mouse cursor on mobile.

When I test mouse device on FireTV with GV and GV on table, I found that mouse event's sample rate is sparse against Desktop. Actually, mouse device supports on Android device now via Bluetooth.

Comment 14

[Timothy Nikkel (:tnikkel)]

Oh okay, thanks for the information. It is weird that you are seeing the sample rate improves with that pref, I would expect that only if a reflow or scrolling happens do you see improved refresh rate. It sounds like there is a problem with the real mouse moves getting through then.

Comment 15

[Makoto Kato [:m_kato]]

I am interesting whether this occurs on Windows or Linux with touch device. layout.reflow.synthMouseMove was added by Fennec/Maemo since that was no touch event at that time and that device was too low end device now.

But I will clone this issue since I suspect original root cause may be APZ or something. then I will back out Bug 1545393 by this issue. After root cause is fixed, I will remove layout.reflow.synthMouseMove again.

Comment 16

[Makoto Kato [:m_kato]]

cloned as bug 1582363 to investigate root cause.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 1580669 - Backed out bug 1545393 until bug 1582363 is fixed. r?snorp

Accessibility caret receives incorrect mouse move event, so I revert layout.reflow.synthMouseMove preference until bug 1582363 is fixed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e0813214854
Backed out bug 1545393 until bug 1582363 is fixed. r=snorp

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5e0813214854

Comment 20

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I believe Firefox 70 needs this patch to fix the usability issue. Could you request an uplift, please?

Comment 21

[Makoto Kato [:m_kato]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #20)

I believe Firefox 70 needs this patch to fix the usability issue. Could you request an uplift, please?

Yes, I will uplift since Fenix uses 70.

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 9093846 [details]
Bug 1580669 - Backed out bug 1545393 until bug 1582363 is fixed. r?snorp

Beta/Release Uplift Approval Request

• User impact if declined: This is regression by bug 1545393, so this fix backs out it.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This backs out bug 1545393
• String changes made/needed: no

Comment 23

[Pascal Chevrel:pascalc (PTO until April 26)]

The patch has no automatic tests and the uplift request says that no manual testing is required. It also says that the fix was verified in Nightly, yet the status of the tracking flag for 70 is fixed, not verified and I am not seeing any request to QA for bug verification in this bug.

Setting qe-verify+ flaf to get the fix verified on Nightly by QA before evaluating an uplift.

Comment 24

[Laurentiu Apahidean]

I tested the issue on Fenix 2.0.0 (Build #12610151) 9/18 and the issue still occurs, see issue https://github.com/mozilla-mobile/fenix/issues/5037 for further information.

Mozilla central builds are no longer valid and do not reach users only the esr versions do.
If you want to test this issue for Fennec we will need an esr try-build, after the issue is tested on that build an uplift can be requested for the Beta build.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Laurentiu can't you test with the latest Fennec nightly from the play store? I'm using it and it updates automatically on my phone, daily. Maybe I am misunderstanding though.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Julien just explained this to me; we need to uplift to beta (which is really an esr branch I guess) for the change to appear in nightly builds for fenix and for fennec. In this case why don't we just uplift now and verify after since we still have 2 weeks of beta builds before RC week.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9093846 [details]
Bug 1580669 - Backed out bug 1545393 until bug 1582363 is fixed. r?snorp

Uplift now, verify after.

Comment 28

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/42b9ba8aa2c2

Comment 29

[Laurentiu Apahidean]

I have tested the issue on the latest Firefox builds: Release 68.1.1, Nightly 68.2a1 (2019-10-05), 71.0a1 (2019-10-4) and 70.0b13 using a Samsung Galaxy S8+ (Android 8.0.0) and the issue no longer occurs. I will mark this issue accordingly.