Closed Bug 1442176 Opened 2 years ago Closed Last year

The page scrolls when the user drags text selection handles

Categories

(GeckoView :: General, defect, P1)

All
Android
defect

Tracking

(geckoview62 fixed, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- fixed
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: sebastian, Assigned: jchen)

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(3 files)

When moving the text selection handles the current page starts to scroll too. This is weird and confusing; and a different behavior than you see in WebView or native apps. WebView does not scroll unless you move the handles outside of the current viewport.
Comparing this with Fennec this might be caused by our NestedGeckoView implementation. I'll file a bug on GitHub too.
Looks like Sebastian owns this one. Please adjust if not.
Assignee: nobody → s.kaspari
Priority: -- → P1
Sebastian, is there any GeckoView work that needs to be done to fix this text selection issue in Klar? Or is this something Klar can fix in NestedGeckoView?

You and Emily were discussing NestedGeckoView in https://github.com/mozilla-mobile/focus-android/issues/2194
Flags: needinfo?(s.kaspari)
I'm not fully sure about that yet.

We know that the scrolling happens if our NestedGeckoView implementation is used and it doesn't happen if we just use plain GeckoView. However we are using the same approach for WebView and do not see this happening. So what we do not know yet is whether the NestedGeckoView implementation is the issue or whether GeckoView is providing the wrong scrolling state information to NestedGeckoView.

Help debugging this would be appreciated. I haven't found the time yet. NestedGeckoView is used to scroll the toolbar away in certain situations (using default components from the Android support library). I'd be interested to know if the platform team thinks this is a good approach or if they'd recommend implementing this differently. I remember that there's a lot of complex code for this in Fennec.
Flags: needinfo?(s.kaspari)
Randall, is this a core APZ scrolling problem or something we need to fix in GeckoView? Moving the text selection handles should not cause the page to scroll.
Assignee: s.kaspari → nobody
Flags: needinfo?(rbarker)
This sounds like a GeckoView issue. In fennec the page does not scroll unless the selection handle reaches the top or bottom of the page. I'm not sure what is different between Fennec and GV.
Flags: needinfo?(rbarker)
Assignee: nobody → snorp
James, do you have any updates on the text selection handles? Why is this an issue in Focus+GV but not Fennec? Is this a Focus app bug or a GV bug?
Flags: needinfo?(snorp)
This doesn't appear to happen in Fennec Custom Tabs, which are also powered by GeckoView.
However, I can easily reproduce it in Focus (both in browser and in Focus-powered Custom Tabs).
[geckoview:klar:p2] because this bug is annoying but probably not a Focus+GV blocker.
Summary: GeckoView: Do not scroll while moving text selection handles → The page scrolls when the user drags text selection handles
Whiteboard: [geckoview:klar] → [geckoview:klar:p2]
I haven't really been looking at this.
Flags: needinfo?(snorp)
Assignee: snorp → nobody
Sending this bug back to GV triage because the Focus team would like this bug might be a Focus+GV release blocker.
Priority: P1 → --
Whiteboard: [geckoview:klar:p2]
Jim said he'd take a look.
Assignee: nobody → nchen
Priority: -- → P1
Whiteboard: [geckoview:klar:p1]
Remove the current code that pins the toolbar on caret drag, which only
works for Fennec.
Add a flag for whether the session should be pinned to the screen. The
app would check the flag and prevent scrolling of the session when it's
pinned.
Send the new "GeckoView:PinToScreen" event in Fennec code so the Fennec
toolbar is pinned when dragging selection carets.
Comment on attachment 9007028 [details]
Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh

Dylan Roeh (:droeh) has approved the revision.
Attachment #9007028 - Flags: review+
Comment on attachment 9007029 [details]
Bug 1442176 - 3. Use new toolbar pinning code in Fennec; r=petru

Petru-Mugurel Lingurar[:petru] has approved the revision.
Attachment #9007029 - Flags: review+
Comment on attachment 9007028 [details]
Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh

Dylan Roeh (:droeh) has been removed from the revision.
Attachment #9007028 - Flags: review+
Comment on attachment 9007027 [details]
Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp

James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.
Attachment #9007027 - Flags: review+
Product (Andreas, Barbara, Vesta) wouldn't block focus on this issue alone.
Attachment #9007028 - Attachment description: Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r?snorp r?droeh → Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r?snorp r=droeh
Attachment #9007029 - Attachment description: Bug 1442176 - 3. Use new toolbar pinning code in Fennec; r?petru → Bug 1442176 - 3. Use new toolbar pinning code in Fennec; r=petru
(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #22)
> Product (Andreas, Barbara, Vesta) wouldn't block focus on this issue alone.

status-geckoview62=fix-optional because we'd like to uplift this fix to GV 62 for Focus 7.x but we won't necessarily block the Focus 7.0 release.
Comment on attachment 9007028 [details]
Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh

James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.
Attachment #9007028 - Flags: review+
Attachment #9007027 - Attachment description: Bug 1442176 - 1. Remove current pin-on-caret-drag code; r?snorp → Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp
Attachment #9007028 - Attachment description: Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r?snorp r=droeh → Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f0ecb6ed63
1. Remove current pin-on-caret-drag code; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a6f4ef044a
2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/d78f916478af
3. Use new toolbar pinning code in Fennec; r=petru
Comment on attachment 9007027 [details]
Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: GeckoView consumers such as Focus will need the new API that this fix introduces.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fix preserves existing Fennec behavior but introduces a new API for Focus to use.
[String changes made/needed]: None
Attachment #9007027 - Flags: approval-mozilla-beta?
Comment on attachment 9007027 [details]
Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: Focus needs the new API that this fix introduces.

User impact if declined: Bad toolbar interaction during text selection in Focus.

Fix Landed on Version: 64

Risk to taking this patch (and alternatives if risky): Small; fix only adds the new API that Focus requires.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.
Attachment #9007027 - Flags: approval-mozilla-geckoview62?
Comment on attachment 9007027 [details]
Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp

Uplift approved for 63 beta 8, thanks.
Attachment #9007027 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi, I tried to uplift these patches but got this:

grafting 437140:10f0ecb6ed63 "Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp"
merging layout/base/AccessibleCaretManager.cpp
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerSession.java
merging widget/android/nsWindow.cpp
merging widget/android/nsWindow.h
grafting 437141:22a6f4ef044a "Bug 1442176 - 2. Add pinned-to-screen flag in GeckoSession; r=snorp r=droeh"
other [graft] changed mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java which local [local] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging mobile/android/chrome/geckoview/GeckoViewContent.js                     
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerSession.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Could you please take a look?
Flags: needinfo?(nchen)
Given extra time, we want to take this in 62 gv relbranch
Backed out for android build bustages at builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoDisplay.java

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=a733114f72eb86e7c9f6a2406f2c832a325a3bf2

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=200560749&repo=mozilla-beta&lineNumber=3597

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/be49f2604cfe40b4b1b463fcf34717f10f94bc89
Flags: needinfo?(nchen)
Comment on attachment 9007027 [details]
Bug 1442176 - 1. Remove current pin-on-caret-drag code; r=snorp

Setting the approval for GV62 retroactively since it was already uplifted anyway.
Attachment #9007027 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62+
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 64 → mozilla64

Andreea, you reopened one of the diffs for this on phabricator: https://phabricator.services.mozilla.com/D5190 -- it's not clear to me why. Are there any actual new changes I need to review, or can we safely re-close that?

Flags: needinfo?(apavel)

(In reply to Dylan Roeh (:droeh) (he/him) from comment #39)

Andreea, you reopened one of the diffs for this on phabricator: https://phabricator.services.mozilla.com/D5190 -- it's not clear to me why. Are there any actual new changes I need to review, or can we safely re-close that?

Hi, i'm not sure why this was reopened, i did not reopen it manually :-?

Aryx could it have been reopened due to changes I made on some other phab patches, uplifts or something of this sort?

Flags: needinfo?(apavel) → needinfo?(aryx.bugmail)

The phabricator diff got automatically reopened due to Phabricator maintenance.

Flags: needinfo?(aryx.bugmail)

Thank you.

Dylan i guess you can safely re-close it.

Flags: needinfo?(droeh)
Flags: needinfo?(droeh)
You need to log in before you can comment on or make changes to this bug.