Closed Bug 1540545 Opened 6 years ago Closed 6 years ago

Text in subframes isn't properly selected after zooming in

Categories

(Core :: Panning and Zooming, defect, P2)

Product:
Core
Component:
Panning and Zooming
Version:
68 Branch
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: csheany, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:p2])

Attachments

(1 file)

Bug 1540545 - Use consistent hit test options in AccessibleCaretManager. r=TYLin
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review

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

Steps to reproduce:

  1. Open this bug
  2. Zoom in
  3. Lons press (this) word
  4. Drag the caret down

Actual results:

The top half is selected

Expected results:

The bottom half is selected

Would you mind taking a look?

Flags: needinfo?(m_kato)

This seems to be accessibility caret issue. Also, I can reproduce this with reference browser (GV 68-20190321104132) too.

Component: Text Selection → Selection
Flags: needinfo?(m_kato)
Product: Firefox for Android → Core
Whiteboard: [geckoview]
Version: Firefox 68 → 68 Branch

Thank your for your response.

I wasn't sure which component it should fall under.

Are you able to reproduce in Release/ Beta?

Ting-Yu is the resident AccessibleCaret expert :)

Flags: needinfo?(aethanyc)

The caret dragging is broken under certain zoom scale. I can reproduce on Fennec or Fenix. Debugging ...

Blocks: AccessibleCaret
Status: UNCONFIRMED → NEW
Ever confirmed: true

Just a shot in the dark, but does the bug reproduce with layout.scroll.root-frame-containers=1?

Does it require restarting?

I can still reproduce with layout.scroll.root-frame-containers=1 after restarting Fennec. The touch event positions seem shifted more if I scroll down more. For example, if I scroll down to this comment and zoom in, I cannot even do long tap to select a word. FWIW, this doesn't like a caret bug to me.

Kats, Botond, maybe this is a regression?

Flags: needinfo?(kats)
Flags: needinfo?(botond)
Flags: needinfo?(aethanyc)
See Also: → 1539418

This looks like it's been broken for a while. I just tried 2019-01-01 and 2018-07-01 nightlies on Fennec and they're both broken. I suspect it's just broken with subframes.

I've tried as far back as Firefox 48 (early 2016) and the bug reproduces there, so this is not a recent regression, and I don't think a regression range is going to be useful.

My guess, based on the fact that the caret code is receiving incorrect touch event coordinates, would be that the issue is related to the callback transform / resolution un-application stuff.

Component: Selection → Panning and Zooming
Flags: needinfo?(botond)
status-firefox66: --- → wontfix
status-firefox67: --- → fix-optional
status-firefox68: --- → affected
Flags: needinfo?(kats)
Priority: -- → P2
Summary: Text isn't properly selected after zooming in → Text in subframes isn't properly selected after zooming in
Blocks: 1542832
OS: Unspecified → Android
Whiteboard: [geckoview] → [geckoview:p2]

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

The touch event positions seem shifted more if I scroll down more.

Could you elaborate on this?

I added logging in two places:

  • When receiving a long-tap event, log point here.
  • When receiving a touch-move event, log point here.

Then I performed the STR of this bug, and looked at the coordinates logged -- they were very similar. So, at least there does not seem to be a problem with the event coordinates being provided by APZ, like I suspected in comment 10.

Given that those two points are very close to each other, I don't understand why the first one causes selection of the word at the correct location, while the second one causes dragging the caret to a clearly incorrect location very far away.

For example, if I scroll down to this comment and zoom in, I cannot even do long tap to select a word.

This part I cannot reproduce.

Flags: needinfo?(aethanyc)

The issue seems to be related to the outcome of this hit test: when the bug occurs, the outcome is the root HTMLScroll frame; when the bug does not occur, the outcome is a block or text frame.

What is curious is that the outcome of the hit test varies by zoom level even though the input coordinates are basically the same.

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

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

The touch event positions seem shifted more if I scroll down more.

Could you elaborate on this?

Sorry that I didn't add logs to dump the actual touch event positions to verify the above claim. I just notice that it's harder to select the text by long-tapping when I scroll down more.

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

The issue seems to be related to the outcome of this hit test: when the bug occurs, the outcome is the root HTMLScroll frame; when the bug does not occur, the outcome is a block or text frame.

What is curious is that the outcome of the hit test varies by zoom level even though the input coordinates are basically the same.

Ah, so the touch positions are correct, but the frame returned by hit test is wrong at certain zoom level ...

Flags: needinfo?(aethanyc)

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

For example, if I scroll down to this comment and zoom in, I cannot even do long tap to select a word.

This part I cannot reproduce.

It's easier to reproduce with

  1. Goto https://www.google.com/search?q=mozilla
  2. "Request desktop site" in the three vertical dot menu. (It's harder to reproduce with mobile site)
  3. Scroll down to the bottom to select some word.

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

The issue seems to be related to the outcome of this hit test: when the bug occurs, the outcome is the root HTMLScroll frame; when the bug does not occur, the outcome is a block or text frame.

The reason for the unexpected outcomes is that the hit test in DragCaretInternal is missing the IgnoreRootScrollFrame flag that's used by the hit test in SelectWordOrShortcut.

No longer blocks: 1542832

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

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

For example, if I scroll down to this comment and zoom in, I cannot even do long tap to select a word.

This part I cannot reproduce.

It's easier to reproduce with

  1. Goto https://www.google.com/search?q=mozilla
  2. "Request desktop site" in the three vertical dot menu. (It's harder to reproduce with mobile site)
  3. Scroll down to the bottom to select some word.

Unfortunately, I still cannot reproduce this.

Given the nature of the diagnosis above, it would be a different bug anyways.

It does sound similar to bug 1542832, which I can reproduce.

It's important to use the IgnoreRootScrollFrame for correct hit testing when
zoomed in or out on Android. The hit test in DragCaretInternal() was missing
this flag.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/144a2491cbd5 Use consistent hit test options in AccessibleCaretManager. r=TYLin

https://hg.mozilla.org/mozilla-central/rev/144a2491cbd5

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → botond

Botond, do you think we should uplift your hit test fix to Fennec 67 Beta? How easy is it to trigger this long-standing bug? We have a little over three weeks left in the 67 Beta cycle.

status-firefox67: fix-optional → ?
Flags: needinfo?(botond)

To weigh in...

The bug is fairly easy to trigger hence the STR as opposed to pointing elsewhere.

However the approach may need to be extended :)

I'm still noticing some weird behavior.

After long pressing open and dragging the caret to the left the top is selected.

(In reply to csheany from comment #21)

To weigh in...

The bug is fairly easy to trigger hence the STR as opposed to pointing elsewhere.

However the approach may need to be extended :)

I'm still noticing some weird behavior.

After long pressing open and dragging the caret to the left the top is selected.

I cannot reproduce the STR in comment #0 on Fennec 68.0a1 (2019-04-19). Perhaps the build you tested has not picked up the patch as of you wrote comment 21? Feel free to open a new bug for any other unexpected behavior :)

I am using that build as well and yes as filed I am unable to reproduce.

What I mentioned is slightly different and because you asked so nicely...

See also Bug 1545812

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

Botond, do you think we should uplift your hit test fix to Fennec 67 Beta? How easy is it to trigger this long-standing bug? We have a little over three weeks left in the 67 Beta cycle.

I don't think it's very common, as it likely would have been reported sooner then. However, as the fix is low-risk, I think we can proceed with an uplift.

Flags: needinfo?(botond)

Comment on attachment 9058479 [details]
Bug 1540545 - Use consistent hit test options in AccessibleCaretManager. r=TYLin

Beta/Release Uplift Approval Request

  • User impact if declined: On some pages, when zoomed in and selecting text, dragging the text selection caret produces unexpected results (the selection is extended to a location different from where you dragged, possibly far away).
  • 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 is a small, targeted fix to text selection code, in fact just carrying over a previous fix made for long-taps, to dragging.
  • String changes made/needed:
Attachment #9058479 - Flags: approval-mozilla-beta?
status-firefox67: ? → affected
Flags: qe-verify+

Comment on attachment 9058479 [details]
Bug 1540545 - Use consistent hit test options in AccessibleCaretManager. r=TYLin

Low-risk fix for some text selection weirdness. Approved for 67.0b13. Let's get QA to try to verify as well.

Attachment #9058479 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Hello,

I performed the test case posted in Comment 0 on a OnePlus A3000 (Android 6.0.1).
On Beta 67.0b13 the text selection can be done correctly.

Due to my findings, I'll mark this issue as verified in Firefox 67.

QA Whiteboard: [qa-triaged]
status-firefox67: fixed → verified
Flags: qe-verify+

I figured this was a known issue.

It was on Bugzilla after all :)

I don't think Bugzilla gets heavy usage on Android.

Desktop wasn't affected?

Text selection carets are only used in touchscreen environments. I'm not sure whether they work with touchscreens on desktop platforms, but even if they do, that's also a configuration where Bugzilla doesn't see heavy usage.

Thank you for clarifying.

Verified as fixed also on Nightly 68.0a1 (2019-04-23) using Nexus 9 (Android 7.1.1).

Status: RESOLVED → VERIFIED
status-firefox68: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: