New issue
Advanced search Search tips

Issue 765455 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Insertion selection content rect should also include the touch handle

Project Member Reported by amaralp@chromium.org, Sep 14 2017

Issue description

Chrome Version: All
OS: Android

The content rect that is used for positioning the floating selection
menu didn't include the touch handle when the selection was not a
range selection. This could lead to the floating menu occluding the
handle. This CL adds the touch handle to the content rect.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 14 2017

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

commit 1d980f6e63a287e63284b8fc56b69a78a8c37312
Author: Pedro Amaral <amaralp@chromium.org>
Date: Thu Sep 14 23:04:39 2017

Insertion selection content rect should also include the touch handle

The content rect that is used for positioning the floating selection
menu didn't include the touch handle when the selection was not a
range selection. This could lead to the floating menu occluding the
handle. This CL adds the touch handle to the content rect.

Bug:  765455 
Change-Id: Ie3f60f5fba7edd223cd3d91e208b483a9eacd2b4
Reviewed-on: https://chromium-review.googlesource.com/667820
Commit-Queue: Pedro Amaral <amaralp@chromium.org>
Reviewed-by: Alexandre Elias <aelias@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502084}
[modify] https://crrev.com/1d980f6e63a287e63284b8fc56b69a78a8c37312/content/browser/renderer_host/render_widget_host_view_android.cc

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 27 2017

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

commit ae740f9436314440bd18ab946a52a7d29107c5d2
Author: Pedro Amaral <amaralp@chromium.org>
Date: Fri Oct 27 19:02:39 2017

Revert "Insertion selection content rect should also include the touch handle"

This reverts commit 1d980f6e63a287e63284b8fc56b69a78a8c37312.

Reason for revert: Apps that relied on WebView had some of their functionality break. See crbug.com/771891 and crbug.com/766459.

Original change's description:
> Insertion selection content rect should also include the touch handle
> 
> The content rect that is used for positioning the floating selection
> menu didn't include the touch handle when the selection was not a
> range selection. This could lead to the floating menu occluding the
> handle. This CL adds the touch handle to the content rect.
> 
> Bug:  765455 
> Change-Id: Ie3f60f5fba7edd223cd3d91e208b483a9eacd2b4
> Reviewed-on: https://chromium-review.googlesource.com/667820
> Commit-Queue: Pedro Amaral <amaralp@chromium.org>
> Reviewed-by: Alexandre Elias <aelias@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#502084}

TBR=aelias@chromium.org,amaralp@chromium.org

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

Bug:  765455 
Change-Id: I47eea73915bbee88effc84e908f468f28fdc51c1
Reviewed-on: https://chromium-review.googlesource.com/741688
Reviewed-by: Pedro Amaral <amaralp@chromium.org>
Commit-Queue: Pedro Amaral <amaralp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512259}
[modify] https://crrev.com/ae740f9436314440bd18ab946a52a7d29107c5d2/content/browser/renderer_host/render_widget_host_view_android.cc

Labels: Merge-Request-63
Requesting to merge revert to M63.
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 27 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

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

Comment 6 by cma...@chromium.org, Oct 27 2017

This fix does not seem critical enough to be merged in M63. Can this just be in M64?
This revert would fix crbug.com/771891 and crbug.com/766459 which are both marked as RBS. The linked bugs aren't critical so we could also remove their RBS labels. I'm fine with either decision.

Comment 8 by cma...@chromium.org, Oct 30 2017

Labels: -Pri-3 Pri-1
 amaralp@ is this revert safe? Could it potentially introduce new issues?
I think it's safe. It shouldn't introduce new issues.
Labels: -Hotlist-Merge-Review -Merge-Review-63 Merge-Approved-63
Approving this merge into 63 branch 3239 so that 2 RBS can be fixed.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 30 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dc82198565ad5a5337df0e08cdcde651009705f

commit 3dc82198565ad5a5337df0e08cdcde651009705f
Author: Pedro Amaral <amaralp@chromium.org>
Date: Mon Oct 30 23:26:38 2017

Revert "Insertion selection content rect should also include the touch handle"

This reverts commit 1d980f6e63a287e63284b8fc56b69a78a8c37312.

Reason for revert: Apps that relied on WebView had some of their functionality break. See crbug.com/771891 and crbug.com/766459.

Original change's description:
> Insertion selection content rect should also include the touch handle
>
> The content rect that is used for positioning the floating selection
> menu didn't include the touch handle when the selection was not a
> range selection. This could lead to the floating menu occluding the
> handle. This CL adds the touch handle to the content rect.
>
> Bug:  765455 
> Change-Id: Ie3f60f5fba7edd223cd3d91e208b483a9eacd2b4
> Reviewed-on: https://chromium-review.googlesource.com/667820
> Commit-Queue: Pedro Amaral <amaralp@chromium.org>
> Reviewed-by: Alexandre Elias <aelias@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#502084}

TBR=aelias@chromium.org, amaralp@chromium.org


(cherry picked from commit ae740f9436314440bd18ab946a52a7d29107c5d2)

Bug:  765455 
Change-Id: I47eea73915bbee88effc84e908f468f28fdc51c1
Reviewed-on: https://chromium-review.googlesource.com/741688
Reviewed-by: Pedro Amaral <amaralp@chromium.org>
Commit-Queue: Pedro Amaral <amaralp@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#512259}
Reviewed-on: https://chromium-review.googlesource.com/744994
Cr-Commit-Position: refs/branch-heads/3239@{#306}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/3dc82198565ad5a5337df0e08cdcde651009705f/content/browser/renderer_host/render_widget_host_view_android.cc

Sign in to add a comment