1526268 - Text selection caret incorrectly positioned in search box inside position:fixed element

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P3)

Description

[csheany]

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:67.0) Gecko/67.0 Firefox/67.0

Steps to reproduce:

1. Open about:config
2. Perform a search
3. Scroll the results
4. Tap to search

Actual results:

The handle drops down

Expected results:

The handle stays in place

Comment 1

[Botond Ballo [:botond]]

This regressed sometime between Firefox 48 and Firefox 57.

The appearance of the caret also changed between those two releases, from orange to blue; the two changes may be related (AccessibleCaret rewrite?)

Tentatively moving to Core :: Selection and cc'ing Ting-Yu.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

AccessibleCaret is a position:absolute div, so it is scrolling with the APZ with the search result page even if it was shown on the position:fixed editable field. I think this is due to the behavior changed in bug 1249201, i.e. we decide to show caret during scrolling. AccessibleCaret was used to hide during scrolling, and update it's position after scroll is ended.

We had some bugs like this one on overflow:scroll div (bug 1273045) and iframe (bug 1316318). We may need to do something similar.

Comment 3

[csheany]

I am also seeing this on the desktop Reddit site.

Should the summary be edited based on the type of page?

Comment 4

[Botond Ballo [:botond]]

(In reply to csheany from comment #3)

Should the summary be edited based on the type of page?

Yep, thanks. The common thread here is the text field being inside a position:fixed header.

Comment 6

[Chris Peterson [:cpeterson]]

This bug affects also GeckoView in Fenix (bug 1575451).

Webcompat bug https://webcompat.com/issues/32939

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Can we somehow change the position style to fixed in the case where the target nsIFrame is in inside position:fixed element?

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

Can we somehow change the position style to fixed in the case where the target nsIFrame is in inside position:fixed element?

I can take a look at this issue, and see if this idea is possible.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

Can we somehow change the position style to fixed in the case where the target nsIFrame is in inside position:fixed element?

The positioning of AccessibleCaret is built on its div being position: absolute. I experiment this idea a bit, but dragging carets becomes difficult, and it seems awkward to have duo positioning system. Also, we need to consider when the caret is on a position:sticky element. When scrolling a position:sticky element, it behaves like a position:static, but once it "stuck", it behaves like a position:fixed.

I'll pursue the fix by dynamically disable APZ in certain situations per comment 2.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1526268 Part 1 - Make AccessibleCaret's #text-overlay and #image children be normal in-flow elements.

The #text-overlay and #image child divs were "position: absolute" under
the main AccessibleCaret div. However, they don't necessary need to be
position:absolute to achieve the desired layout. We can make them normal
in-flow elements to simplify the frame structure. There should not be
any perceivable change to the user.

Also, AccessibleCaret's position can made more accurate by using float
CSS pixels when converting it from app unit.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1526268 Part 2 - Fix the logic to detect whether AccessibleCaret's position is changed.

This is the main patch to fix bug 1526268.

It is wrong to use the cached rects relative to the root
frame (ViewportFrame) to detect whether AccessibleCaret's position is
changed or not, because it doesn't account the root scroll frame's
scroll offset.

The effect is that we always produce "PositionChangedResult::Changed"
when scrolling on position:static elements, but
"PositionChangedResult::NotChanged" on position:fixed elements.

This patch fixes this by using the rect relative to custom content
container, which is the actually rect to set caret's position, to check
whether the position is changed or not.

Note that even with this patch, the caret on "position:fixed" element is
still jumpy during scrolling due to APZ. Part 3 will fixed this.

Depends on D51349

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1526268 Part 3 - Disable APZ if AccessibleCaret is in position:fixed subtree or its position is changed.

In common cases where the caret is in a position:static frame subtree,
the caret's position (relative to canvas frame's custom content
container) should not be changed during scrolling.

However, when the caret is in a position:fixed or "stuck"
position:sticky frame subtree, the caret's position will change during
scrolling. We need to disable APZ to avoid jumpy carets.

Depends on D51350

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4440195bda02
Part 1 - Make AccessibleCaret's #text-overlay and #image children be normal in-flow elements. r=mats
https://hg.mozilla.org/integration/autoland/rev/bad5d7c12e6b
Part 2 - Fix the logic to detect whether AccessibleCaret's position is changed. r=mats
https://hg.mozilla.org/integration/autoland/rev/47865c3e9794
Part 3 - Disable APZ if AccessibleCaret is in position:fixed subtree or its position is changed. r=botond,mats

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4440195bda02
https://hg.mozilla.org/mozilla-central/rev/bad5d7c12e6b
https://hg.mozilla.org/mozilla-central/rev/47865c3e9794

Comment 16

[Pascal Chevrel:pascalc]

P3, we shipped several releases with this bug and we are past the mid-beta cycle point, let's have it ship with 72.

Comment 17

[Laurentiu Apahidean]

I have tested the issue on an older Firefox Preview Nightly build: 191210 (Build #13440605) 12/10 and on the latest: 191112 (Build #13160608) 11/12 and I was unable to reproduce the issue.