Closed Bug 1526268 Opened 9 months ago Closed 16 days ago

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

Categories

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

67 Branch
Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla72
Webcompat Priority ?
Tracking Status
firefox-esr68 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: csheany, Assigned: TYLin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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

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.

Component: General → Selection
Product: Firefox for Android → Core
Version: Firefox 67 → 67 Branch
Summary: Cursor handle is seperated on about:config → Text selection caret incorrectly position in about:config search box
Summary: Text selection caret incorrectly position in about:config search box → Text selection caret incorrectly positioned in about:config search box

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.

Priority: -- → P3

I am also seeing this on the desktop Reddit site.

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

(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.

Summary: Text selection caret incorrectly positioned in about:config search box → Text selection caret incorrectly positioned in search box inside position:fixed element
Duplicate of this bug: 1575451

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

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Duplicate of this bug: 1586636

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

Webcompat Priority: --- → ?

(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.

Flags: needinfo?(aethanyc)

(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.

Assignee: nobody → aethanyc
No longer blocks: 1249201
Status: NEW → ASSIGNED
Flags: needinfo?(aethanyc)
Regressed by: 1249201

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.

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

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

Pushed by aethanyc@gmail.com:
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

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

You need to log in before you can comment on or make changes to this bug.