Contextual Search observer notifies Show with no Hide when Smart Selection is active |
|||
Issue descriptionOne 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.
,
Aug 27
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.
,
Aug 28
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.
,
Aug 28
Thanks for looking into this!
,
Aug 29
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.
,
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
,
Aug 29
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?
,
Aug 29
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.
,
Aug 29
> I'll update it... SGTM: I am OOO for a few days starting tomorrow. We don't plan to fix this CSObserver notification.
,
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
,
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
,
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
,
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 |
|||
Comment 1 by donnd@google.com
, Aug 27Status: Started (was: Available)