New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 878006 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
(OOO slow)
Closed: Aug 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Contextual Search observer notifies Show with no Hide when Smart Selection is active

Project Member Reported by mdjones@chromium.org, Aug 27

Issue description

One of two things is happening:

1. Long-pressing text triggers the ContextualSearchObserver#onShowContextualSearch without showing the UI.

2. Long-pressing text shows and immediately hides the UI without calling the onHideContextualSearch event.

In practice this breaks UI (like Duet) that show or hide their UI to allow Contextual Search to be shown.
 
Owner: donnd@chromium.org
Status: Started (was: Available)
I'll take a look at what happens when we suppress our UI due to Smart Selection integration.
Looks like #1 is happening. We should never show without a corresponding hide. 

I'll investigate further to see if there's a quick fix.  
Cc: twelling...@chromium.org
Labels: -Pri-2 Pri-3
Summary: Contextual Search observer notifies Show with no Hide when Smart Selection is active (was: Contextual Search observer misfiring events)
I don't see a clean way to fix this problem, but don't think it's very important since Matt is taking another approach to addressing issue 877942.

For the record, here's what's going on:

1) The contract for the observer is a little unclear, and doesn't quite fit the requirements.  The contract says notification will happen for a TTS Show or Hide, but the icing system would really like to track the selection rather than TTS.  This causes a problem when Smart Selection is active because TTS is suppressed, and that leads to an inconsistent notification.

2) The code sends Hide when TTS is hidden, but needs to send Show events when the selection changes.  Moving to a model that just tracks the selection would probably be best, and we should rename the observer interface for that.

3) For now I'll just update the interface to document that multiple Show messages can happen, and the flaw in the existing test that allows this bug to persist.
Thanks for looking into this!
For the record: snapshot of possible fix to just notify when CS is shown or hidden is here: https://chromium-review.googlesource.com/c/chromium/src/+/1192324/2
A better approach would be to track the selection and rename the interface.
For now we're just documenting current behavior.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c0d6aacaad083171f0c294d2ae3a844d2655bba

commit 6c0d6aacaad083171f0c294d2ae3a844d2655bba
Author: Donn Denman <donnd@chromium.org>
Date: Wed Aug 29 20:28:09 2018

[TTS] ContextualSearchObserver w/ Smart Selection.

The ContextualSearchObserver code sometimes sends a Show without any
Hide.  This is because we're sending the Show notification when the
selection changes, instead of when the panel is shown.  This means that
features that suppress showing the panel, like Smart Selection, may
interfere with this notification.

This CL just clarifies when the messages are sent and notes a problem
with test coverage (linking to this bug).

BUG= 878006 

Change-Id: I7f9b618b84f1633620490fd15f385796f947dfba
Reviewed-on: https://chromium-review.googlesource.com/1192324
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587276}
[modify] https://crrev.com/6c0d6aacaad083171f0c294d2ae3a844d2655bba/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchObserver.java
[modify] https://crrev.com/6c0d6aacaad083171f0c294d2ae3a844d2655bba/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Matt and Theresa, Does it make sense to update the chromium//src/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabBottomBarDelegate.java
so it uses the new OverlayPanelObserver Matt wrote instead of the ContextualSearchObserver due to this bug?
I think it probably does, though I believe the issue is less severe in that case. I'll update it if you haven't started.
Status: WontFix (was: Started)
> I'll update it...
SGTM: I am OOO for a few days starting tomorrow.

We don't plan to fix this CSObserver notification.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ecef27c668fb1d46abdae3000453dd673fa0b0b2

commit ecef27c668fb1d46abdae3000453dd673fa0b0b2
Author: Matthew Jones <mdjones@chromium.org>
Date: Thu Aug 30 23:25:42 2018

Move ContextualSearchObserver for ContextReporter

This patch delegates the task of reporting context to GSA to
Contextual Search. This is being done in preparation to make the
ContextualSearchObserver package protected since its use is extremely
feature specific.

Bug:  878006 
Change-Id: I340a6196210a4709cebef03e477e241be1633bb4
Reviewed-on: https://chromium-review.googlesource.com/1198003
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587838}
[modify] https://crrev.com/ecef27c668fb1d46abdae3000453dd673fa0b0b2/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
[modify] https://crrev.com/ecef27c668fb1d46abdae3000453dd673fa0b0b2/chrome/android/java/src/org/chromium/chrome/browser/gsa/ContextReporter.java

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab633154eaef930fc3bec2e2402cb1e415979e8f

commit ab633154eaef930fc3bec2e2402cb1e415979e8f
Author: Matthew Jones <mdjones@chromium.org>
Date: Fri Aug 31 18:01:27 2018

Replace ContextualSearchObserve use in CCT

ContextualSearchObserver provides external events unreliably. This
patch replaces its usage with OverlayPanelManagerObserver which is an
observer of all overlay panel UI (not just the single feature).

Bug:  878006 
Change-Id: I1e96fc3f4a98d3b55713fe1caa583b3bcb81cfe0
Reviewed-on: https://chromium-review.googlesource.com/1197723
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588088}
[modify] https://crrev.com/ab633154eaef930fc3bec2e2402cb1e415979e8f/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/ab633154eaef930fc3bec2e2402cb1e415979e8f/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabBottomBarDelegate.java

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/841f93c93e7b41d8ae054d1c2d0b136fa38ac77f

commit 841f93c93e7b41d8ae054d1c2d0b136fa38ac77f
Author: Matthew Jones <mdjones@chromium.org>
Date: Fri Aug 31 20:25:32 2018

Replace bottom sheet's use of ContextualSearchObserver

Contextual search's observer is unreliable for show/hide events of
the feature. This patch replaces the use of that observer with an
observer that watches any overlay panel and its visibility.

Bug:  878006 
Change-Id: I271b61504ca99ceebe308f7a65bdb4c06ee64cc7
Reviewed-on: https://chromium-review.googlesource.com/1197166
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588147}
[modify] https://crrev.com/841f93c93e7b41d8ae054d1c2d0b136fa38ac77f/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/841f93c93e7b41d8ae054d1c2d0b136fa38ac77f/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java
[modify] https://crrev.com/841f93c93e7b41d8ae054d1c2d0b136fa38ac77f/chrome/android/javatests/src/org/chromium/chrome/browser/widget/ScrimTest.java
[modify] https://crrev.com/841f93c93e7b41d8ae054d1c2d0b136fa38ac77f/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a0153c9282f1b921ace7f08a6d000ca2a59b0ef

commit 9a0153c9282f1b921ace7f08a6d000ca2a59b0ef
Author: Matthew Jones <mdjones@chromium.org>
Date: Fri Aug 31 21:13:34 2018

ContextualSearchObserver is now package protected

This patch makes ContextualSearchObserver package protected since its
use is specific to the feature itself.

Bug:  878006 
Change-Id: If2e2a660199baf4600caf28729e7b2af3f41d8a1
Reviewed-on: https://chromium-review.googlesource.com/1198005
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588169}
[modify] https://crrev.com/9a0153c9282f1b921ace7f08a6d000ca2a59b0ef/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
[modify] https://crrev.com/9a0153c9282f1b921ace7f08a6d000ca2a59b0ef/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchObserver.java

Sign in to add a comment