1475325 - GeckoView: Floating text selection toolbar does not get dismissed if GeckoView gets destroyed

Status: RESOLVED FIXED

(GeckoView :: Toolbar, defect, P2)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

STR in a Focus build with GV:
* Load a website with text (It needs to be the first page you load! No history!)
* Select text
* Press back

Expected:
The tab gets destroyed, the home screen is displayed and the text selection toolbar is gone.

Actual:
The tab gets destroyed, the home screen is displayed and the floating text selection toolbar is still visible.

Note that it works fine if there's history and we go back in history.

Focus issue: https://github.com/mozilla-mobile/focus-android/issues/2633

Comment 1

[Chris Peterson [:cpeterson]]

P2 because this sounds like an uncommon scenario since this only happens on the first page and then the user has to press back to the home screen. The Focus issue is priority P3.

Comment 2

[Matt Brubeck (:mbrubeck)]

I can't reproduce this in the current GeckoView Focus nightly from the Play store (6.3 build #222071208 gecko 62.0.20180712042337), testing on a Pixel and a Pixel 2 XL, both running Android 8.1.0.  Can you still reproduce this?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I can't reproduce this in Focus Nightly anymore.

Comment 4

[Emily Kager [:ekager]]

We're seeing this behavior again in Nightly off the 62 Rel Branch. https://github.com/mozilla-mobile/focus-android/issues/3317

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Consensus in GV triage is that this should be a Focus fix.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

After talking to Emily, I think this is something we can address in GV.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1475325 - Hide selection action in some scenarios; r?droeh

Hide the selection action toolbar when GeckoView is blurred so the we
don't end up with leftover visible toolbars.

Comment 8

[Chris Peterson [:cpeterson]]

Jim, should we uplift this fix to GV 62?

Even if this fix doesn't make next Tuesday's Focus 7.0 release, we will want to continue fixing bugs in the GV 62 relbranch for future Focus releases (until they can update to GV 63).

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9008904 [details]
Bug 1475325 - Hide selection action in some scenarios; r?droeh

Dylan Roeh (:droeh) has approved the revision.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Yeah we should uplift

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Jim, can you request approval-mozilla-geckoview62 on the patch? (I _think_ that's the process here)

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Still working on the patch but yes I will request the flag.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14ca7a3794fa
Hide selection action in some scenarios; r=droeh

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/14ca7a3794fa

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9008904 [details]
Bug 1475325 - Hide selection action in some scenarios; r?droeh

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: The text selection UI can stay on the screen for longer than it should, causing annoyance.
[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?]: Small patch that only tries to dismiss the text selection UI when necessary.
[String changes made/needed]: None

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9008904 [details]
Bug 1475325 - Hide selection action in some scenarios; r?droeh

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: This causes a bug in Focus that's based on GV 62

User impact if declined: Text selection UI can stay on the screen for longer than it should, causing an annoyance

Fix Landed on Version: 64

Risk to taking this patch (and alternatives if risky): Very small; the patch only tries to dismiss the selection UI when necessary

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1475325 - 2. Add tests for hiding selection action in different cases; r=jchen

Add some tests for hiding selection action during navigation, blurring,
or clearing delegate.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef88efc93b44
2. Add tests for hiding selection action in different cases; r=jchen

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ef88efc93b44

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9008904 [details]
Bug 1475325 - Hide selection action in some scenarios; r?droeh

low-risk, Beta63+, GV62+

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f6a4fa55f724

Comment 22

[Dorel Luca [:dluca]]

Backed out changeset f6a4fa55f724 (bug 1475325) for failures in org.mozilla.geckoview.test.SelectionActionDelegateTest.clear. a=backout

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=200365764&repo=mozilla-beta&lineNumber=3315

INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=clearDelegate[#text]
[task 2018-09-20T05:43:10.177Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2018-09-20T05:43:10.177Z] 05:43:10  WARNING -  TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.SelectionActionDelegateTest.clearDelegate[#text] | status -2
[task 2018-09-20T05:43:10.178Z] 05:43:10     INFO -  TEST-INFO took 1010ms
[task 2018-09-20T05:43:10.245Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2018-09-20T05:43:10.246Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=118
[task 2018-09-20T05:43:10.247Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.SelectionActionDelegateTest
[task 2018-09-20T05:43:10.247Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2018-09-20T05:43:10.248Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=187
[task 2018-09-20T05:43:10.249Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=collapseToEnd[#input]
[task 2018-09-20T05:43:10.250Z] 05:43:10     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: 1
[task 2018-09-20T05:43:10.250Z] 05:43:10     INFO -  TEST-START | org.mozilla.geckoview.test.SelectionActionDelegateTest.collapseToEnd[#input]
[task 2018-09-20T05:43:13.453Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2018-09-20T05:43:13.453Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=118
[task 2018-09-20T05:43:13.454Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.SelectionActionDelegateTest
[task 2018-09-20T05:43:13.454Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=.
[task 2018-09-20T05:43:13.454Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=187
[task 2018-09-20T05:43:13.455Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=collapseToEnd[#input]
[task 2018-09-20T05:43:13.455Z] 05:43:13     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: 0
[task 2018-09-20T05:43:13.456Z] 05:43:13     INFO -  TEST-PASS | org.mozilla.geckoview.test.SelectionActionDelegateTest.collapseToEnd[#input] | took 3208ms

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

Backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/82ac63880ff38d905df3693ae0c3f604ed5d3307

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Bogdan, could you uplift both of these commits to Beta again? Thanks!

https://hg.mozilla.org/mozilla-central/rev/14ca7a3794fa
https://hg.mozilla.org/mozilla-central/rev/ef88efc93b44

Comment 24

[Chris Peterson [:cpeterson]]

status-geckoview62=affedcted because Focus product management would like to get this bug fixed in Focus 7.0 (before October 2).

Comment 25

[Andreea Pavel [:apavel]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #23)
> Bogdan, could you uplift both of these commits to Beta again? Thanks!
> 
> https://hg.mozilla.org/mozilla-central/rev/14ca7a3794fa
> https://hg.mozilla.org/mozilla-central/rev/ef88efc93b44

i get the following when trying to uplift: 


mozilla@ubuntu:~/mozilla-unified$ hg graft -er 14ca7a3794fa::ef88efc93b44
skipping ungraftable merge revision 497639
skipping ungraftable merge revision 497668
skipping ungraftable merge revision 497698
skipping ungraftable merge revision 497823
skipping revision 497691:d073607e0ffa (already grafted to 497903:10bb8728016a)
skipping revision 497850:52871b889095 (already grafted to 497904:9e7e697907a4)
skipping revision 497877:ef88efc93b44 (already grafted to 497911:f6a4fa55f724)
skipping revision 497857:10f0ecb6ed63 (already grafted to 497913:9cfdcb17b563)
skipping revision 497673:8320750a187e (already grafted to 497931:f139ab75d0c9)
grafting 497629:14ca7a3794fa "Bug 1475325 - Hide selection action in some scenar                                                                                                                                                             ios; r=droeh"
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessio                                                                                                                                                             n.java
grafting 497640:59759503b889 "Bug 1483533 - Delay texture delete for DirectMapTe                                                                                                                                                             xtureSource r=jrmuizel"
merging gfx/layers/composite/TextureHost.cpp
grafting 497641:131a80cf6661 "Bug 1491289 - re-vendor libprio to pick up fixes f                                                                                                                                                             or using system NSS r=glandium"
grafting 497642:e4990387dd8c "Bug 1491639 - rename function IS_TABLE_CELL r=dbaron"
merging dom/html/nsGenericHTMLElement.cpp
merging layout/base/nsCSSFrameConstructor.cpp
merging layout/generic/ReflowInput.cpp
merging layout/mathml/nsMathMLmtableFrame.cpp
merging layout/tables/nsTableFrame.cpp
warning: conflicts while merging layout/generic/ReflowInput.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
mozilla@ubuntu:~/mozilla-unified$

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

OK I can take care of it. Thanks!

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-beta/rev/297509ef3d02a63316ca5f3b6e20029b542ba996
https://hg.mozilla.org/releases/mozilla-beta/rev/d21a43596d555ea618da75910aa3c1cb06158725

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-release/rev/f626192f69c331235305223cafc108ac0d1f03df
https://hg.mozilla.org/releases/mozilla-release/rev/61db99a4972c43cf440169f5c3e7cc9e696267a3

Comment 29

[Chris Peterson [:cpeterson]]

Moving toolbar bugs to the new GeckoView::Toolbar component.