1657256 - Scrolling between text selection actions creates offset for selection cursor

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Makoto Kato [:m_kato]]

From https://github.com/mozilla-mobile/fenix/issues/13208.

Environment

• Firefox 81 (2020-08-04) on Windows 10
• Fenix Nightly

Steps to reproduce

1. Open a website with selectable text, e.g. https://www.mozilla.org/en-GB/mission/
2. Select some text
3. Scroll down while still keeping the selected part in your viewport
4. Drag the lower selection cursor down

Expected behavior

The lower selection cursor follows the touch pointer.

###Actual behavior
The lower selection cursor jumps below the context menu and now has an offset to the touch pointer.

Notes

This also occurs on Windows too. So it isn't related to GeckoView. I guess that this issue is accessibility caret.

Comment 1

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

mozregression gives me this range, so this bug should be a regression by bug 1526268.

Comment 2

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

Attachment: Bug 1657256 - Always update the cached mImaginaryCaretRect in SetPosition().

During scrolling, the caret's position relative to the
custom-content-container (cached in mImaginaryCaretRectInContainerFrame)
may not change, but its position relative to root frame can (cached in
mImaginaryCaretRect).

We need to update mImaginaryCaret each time we are in SetPosition().
Otherwise, the caret still remembers its pre-scrolling old position next
time when we drag it, resulting the caret jumping to its old
pre-scrolling position suddenly.

Note this bug only occurs on the root scroll frame where the APZ is
enabled, not in any sub-scroll frames where APZ is disable when the
caret is shown.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9373ef216922
Always update the cached mImaginaryCaretRect in SetPosition(). r=marionette-reviewers,mats,whimboo

Comment 4

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9373ef216922

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is this likely to be a noticeable regression for users upgrading from Fennec to Fenix? If so, should we consider uplifting to Beta for 81?

Comment 6

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

Comment on attachment 9172736 [details]
Bug 1657256 - Always update the cached mImaginaryCaretRect in SetPosition().

Beta/Release Uplift Approval Request

• User impact if declined: After scrolling, the caret can jump to the wrong place rather than following the finger smoothly.
• Is this code covered by automated tests?: Yes
• 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): A small patch that caches a value eagerly, which is essential to calculate the caret's position.
• String changes made/needed:

Comment 7

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)

Is this likely to be a noticeable regression for users upgrading from Fennec to Fenix? If so, should we consider uplifting to Beta for 81?

Yes, it's noticeable, and is reported originally as a fenix issue https://github.com/mozilla-mobile/fenix/issues/13208

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172736 [details]
Bug 1657256 - Always update the cached mImaginaryCaretRect in SetPosition().

Approved for 81.0b6.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b2192a97d01