New issue
Advanced search Search tips

Issue 753579 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 599555



Sign in to add a comment

[TTS] Tapping a form button acts like tapping on previously selected text

Project Member Reported by donnd@google.com, Aug 8 2017

Issue description

Tapping on a button in a form can trigger Contextual Search on some previously selected section of text in that form.  This is very strange -- the tap is clearly on a button element, but only taps on text nodes should trigger CS.  Discovered when looking at crbug.com/751408.
 

Comment 1 by donnd@google.com, Aug 8 2017

Blocking: 599555

Comment 2 by donnd@google.com, Aug 8 2017

I suspect this bug is a regression due to https://codereview.chromium.org/2907963002 which makes surrounding text work for input elements.  Since Buttons are input elements I'm guessing that they are now triggering Contextual Search and probably smart select inappropriately -- I would think only text input elements would trigger, not button-type input elements. 

Here's an example test page that shows the behavior.  https://dump-truck.appspot.com/usecase-address_and_cc_on_same_page/address_and_cc.html

Comment 3 by donnd@google.com, Aug 9 2017

Cc: -ti...@chromium.org donnd@chromium.org
Owner: ti...@chromium.org
Looks like the tap on the button is technically a tap on non-editable text -- the text content of the button itself!  We should verify that this new behavior is due to CL https://codereview.chromium.org/2907963002 and if so roll it back or develop a fix that prevents text within button elements from triggering CS because this ends up causing a lot of problems including issue 599555 which is a privacy vulnerability.  See https://chromium-review.googlesource.com/608618 for details on how I determined this -- a check could be put in GestureManager.cpp for the check that the element isn't an input element.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 21 2017

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

commit afd38807e684a7b571e82b57977f636b6aa21933
Author: Tima Vaisburd <timav@chromium.org>
Date: Mon Aug 21 18:01:02 2017

[Smart Text] Disable surrounding text for input elements.

This CL effectively reverts
https://codereview.chromium.org/2907963002, which appears to
causing problems, including leaking of input fields to
Google servers.

Disabling until a better fix is found.

BUG=751408, 753579, 721840

Change-Id: Iecddba6ebed8faf3c266daa232613c57fa5a7464
Reviewed-on: https://chromium-review.googlesource.com/622747
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Tima Vaisburd <timav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495985}
[modify] https://crrev.com/afd38807e684a7b571e82b57977f636b6aa21933/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/afd38807e684a7b571e82b57977f636b6aa21933/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/222b1645bdd0f7461f7dd9013036ba541412bfd4

commit 222b1645bdd0f7461f7dd9013036ba541412bfd4
Author: Tima Vaisburd <timav@chromium.org>
Date: Tue Aug 22 18:49:30 2017

[Smart Text] Disable surrounding text for input elements.

This CL effectively reverts
https://codereview.chromium.org/2907963002, which appears to
causing problems, including leaking of input fields to
Google servers.

Disabling until a better fix is found.

BUG=751408, 753579, 721840
TBR=timav@chromium.org

(cherry picked from commit afd38807e684a7b571e82b57977f636b6aa21933)

Change-Id: Iecddba6ebed8faf3c266daa232613c57fa5a7464
Reviewed-on: https://chromium-review.googlesource.com/622747
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Tima Vaisburd <timav@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#495985}
Reviewed-on: https://chromium-review.googlesource.com/627076
Reviewed-by: Tima Vaisburd <timav@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#765}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/222b1645bdd0f7461f7dd9013036ba541412bfd4/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/222b1645bdd0f7461f7dd9013036ba541412bfd4/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 22 2017

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

commit c48b1926cc970b4e3b11eaced967596cf63fdbde
Author: Tima Vaisburd <timav@chromium.org>
Date: Tue Aug 22 21:44:16 2017

Revert "[Smart Text] Disable surrounding text for input elements."

This reverts commit 222b1645bdd0f7461f7dd9013036ba541412bfd4.

Reason for revert: Breaks smart text selection for GMail editor, http://crbug.com/757945

Original change's description:
> [Smart Text] Disable surrounding text for input elements.
> 
> This CL effectively reverts
> https://codereview.chromium.org/2907963002, which appears to
> causing problems, including leaking of input fields to
> Google servers.
> 
> Disabling until a better fix is found.
> 
> BUG=751408, 753579, 721840
> TBR=timav@chromium.org
> 
> (cherry picked from commit afd38807e684a7b571e82b57977f636b6aa21933)
> 
> Change-Id: Iecddba6ebed8faf3c266daa232613c57fa5a7464
> Reviewed-on: https://chromium-review.googlesource.com/622747
> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> Commit-Queue: Tima Vaisburd <timav@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#495985}
> Reviewed-on: https://chromium-review.googlesource.com/627076
> Reviewed-by: Tima Vaisburd <timav@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3163@{#765}
> Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}

TBR=yosin@chromium.org,timav@chromium.org

Change-Id: Ie9e4da479d3d20a9c48153b9dda2aeda90ef5b28
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 751408, 753579, 721840
Reviewed-on: https://chromium-review.googlesource.com/627420
Reviewed-by: Tima Vaisburd <timav@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#782}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/c48b1926cc970b4e3b11eaced967596cf63fdbde/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/c48b1926cc970b4e3b11eaced967596cf63fdbde/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 23 2017

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

commit 18fdc89d1fc690834f27e06112ac5f74d317c4c0
Author: Tima Vaisburd <timav@chromium.org>
Date: Wed Aug 23 07:36:52 2017

Revert "[Smart Text] Disable surrounding text for input elements."

This reverts commit afd38807e684a7b571e82b57977f636b6aa21933.

Reason for revert: breaks smart text selection for GMail editor, http://crbug.com/757945

Original change's description:
> [Smart Text] Disable surrounding text for input elements.
> 
> This CL effectively reverts
> https://codereview.chromium.org/2907963002, which appears to
> causing problems, including leaking of input fields to
> Google servers.
> 
> Disabling until a better fix is found.
> 
> BUG=751408, 753579, 721840
> 
> Change-Id: Iecddba6ebed8faf3c266daa232613c57fa5a7464
> Reviewed-on: https://chromium-review.googlesource.com/622747
> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> Commit-Queue: Tima Vaisburd <timav@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#495985}

TBR=yosin@chromium.org,donnd@chromium.org,timav@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 751408, 753579, 721840
Change-Id: Ifc8d987a25fe1b98a206dbd6e572436444545490
Reviewed-on: https://chromium-review.googlesource.com/627576
Reviewed-by: Tima Vaisburd <timav@chromium.org>
Reviewed-by: Donn Denman <donnd@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496615}
[modify] https://crrev.com/18fdc89d1fc690834f27e06112ac5f74d317c4c0/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/18fdc89d1fc690834f27e06112ac5f74d317c4c0/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp

Project Member

Comment 8 by sheriffbot@chromium.org, Sep 4 2017

Labels: Hotlist-Recharge-BouncingOwner
Owner: ----
Status: Untriaged (was: Assigned)
The assigned owner "timav@chromium.org" is not able to receive e-mails, please re-triage.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by donnd@google.com, Sep 5 2017

Cc: -donnd@chromium.org ctzsm@chromium.org
Owner: donnd@chromium.org
Status: Available (was: Untriaged)
Status: Assigned (was: Available)

Sign in to add a comment