Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2836843006:
Only start action mode if there is a selection (Closed)
DescriptionOnly start action mode if there is a selection
It currently doesn't make sense to have an action mode without a selection. The main
way to close the action mode is through |SELECTION_HANDLES_CLEARED| and that is not
possible in this situation since there was no selection to begin with.
BUG=715268
Review-Url: https://codereview.chromium.org/2836843006
Cr-Commit-Position: refs/heads/master@{#467186}
Committed: https://chromium.googlesource.com/chromium/src/+/ae9712079d2b34e4a543a8f04de581a3b7bfd383
Patch Set 1 #
Total comments: 1
Messages
Total messages: 16 (9 generated)
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Only start action mode if there is a selection BUG=714090 ========== to ========== Only start action mode if there is a selection It currently doesn't make sense to have an action mode without a selection. The main way to close the action mode is through |SELECTION_HANDLES_CLEARED| and that is not possible in this situation since there was no selection to begin with. BUG=715268 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org, timav@chromium.org
PTAL
https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:229: if (!isActionModeSupported() || !mHasSelection) return; Do we ever create this ActinMode for insertion? At least createActionMenu() calls isInsertion() (indirectly). For the editable situation the context menu would make sense even if there is no text yet.
On 2017/04/25 at 21:34:50, timav wrote: > https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... > File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): > > https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:229: if (!isActionModeSupported() || !mHasSelection) return; > Do we ever create this ActinMode for insertion? At least createActionMenu() calls isInsertion() (indirectly). For the editable situation the context menu would make sense even if there is no text yet. We do in the situation I described in the linked bug. The problem with having the action mode when there is no selection is that we use the selection for positioning and clearing. Normally the action mode is cleared on |SELECTION_HANDLES_CLEARED|. I think that the if statement with |isInsertion()| could be removed. What situation are you suggesting?
On 2017/04/25 21:50:50, amaralp wrote: > On 2017/04/25 at 21:34:50, timav wrote: > > > https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... > > File > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java > (right): > > > > > https://codereview.chromium.org/2836843006/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:229: > if (!isActionModeSupported() || !mHasSelection) return; > > Do we ever create this ActinMode for insertion? At least createActionMenu() > calls isInsertion() (indirectly). For the editable situation the context menu > would make sense even if there is no text yet. > > We do in the situation I described in the linked bug. The problem with having > the action mode when there is no selection is that we use the selection for > positioning and clearing. Normally the action mode is cleared on > |SELECTION_HANDLES_CLEARED|. I think that the if statement with |isInsertion()| > could be removed. What situation are you suggesting? We discussed offline and it seems the action mode for selection should never have isInsertion() == true. lgtm
lgtm
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1493167719946780, "parent_rev":
"cd6760de8afec6c089b53689e78bf39ae7a08868", "commit_rev":
"ae9712079d2b34e4a543a8f04de581a3b7bfd383"}
Message was sent while issue was closed.
Description was changed from ========== Only start action mode if there is a selection It currently doesn't make sense to have an action mode without a selection. The main way to close the action mode is through |SELECTION_HANDLES_CLEARED| and that is not possible in this situation since there was no selection to begin with. BUG=715268 ========== to ========== Only start action mode if there is a selection It currently doesn't make sense to have an action mode without a selection. The main way to close the action mode is through |SELECTION_HANDLES_CLEARED| and that is not possible in this situation since there was no selection to begin with. BUG=715268 Review-Url: https://codereview.chromium.org/2836843006 Cr-Commit-Position: refs/heads/master@{#467186} Committed: https://chromium.googlesource.com/chromium/src/+/ae9712079d2b34e4a543a8f04de5... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/ae9712079d2b34e4a543a8f04de5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
