|
|
DescriptionDon't show touch handles when selection hidden
If the selection is hidden then clear the touch handles.
Also changed IME test back to using radio input as it was prior to
crrev.com/2616623002. Tapping on a radio button is better to test focus change
because it doesn't create a selection. This means that the clearing of the
handle has to be done by the focus change and not because a new selection was
made.
BUG=714525
Review-Url: https://codereview.chromium.org/2847663002
Cr-Commit-Position: refs/heads/master@{#472371}
Committed: https://chromium.googlesource.com/chromium/src/+/671e6334b0583671f907a52c23a1258b5e1d8770
Patch Set 1 #
Total comments: 3
Patch Set 2 : Incorporated hugoh's CL #
Total comments: 1
Patch Set 3 : added old test back #
Messages
Total messages: 34 (22 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 ========== Initial commit BUG= ========== to ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ==========
Description was changed from ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ========== to ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ==========
Description was changed from ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ========== to ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ==========
Description was changed from ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ========== to ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org, hugoh@opera.com, yosin@chromium.org
PTAL
https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2067: frame.Selection().LayoutSelectionForceHide()) Thanks a lot! I am rewriting the "hide selection" code right now in https://codereview.chromium.org/2841093002 . In PS2 I expose a "HasFocus()" that I think you can use instead of introducing LayoutSelectionForceHide().
https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2067: frame.Selection().LayoutSelectionForceHide()) On 2017/04/27 at 08:53:50, hugoh_UTC2 wrote: > Thanks a lot! I am rewriting the "hide selection" code right now in https://codereview.chromium.org/2841093002 . In PS2 I expose a "HasFocus()" that I think you can use instead of introducing LayoutSelectionForceHide(). Yes, let's use |FrameSelection::HasFocus()| hugoh@ is introducing. |LayoutSelectionForceHide()| doesn't represent selection state (has focus or not focus) correctly. At this time, we show selection for contenteditable regardless focus.
https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2067: frame.Selection().LayoutSelectionForceHide()) On 2017/04/27 09:55:11, yosin_UTC9 wrote: > On 2017/04/27 at 08:53:50, hugoh_UTC2 wrote: > > Thanks a lot! I am rewriting the "hide selection" code right now in > https://codereview.chromium.org/2841093002 . In PS2 I expose a "HasFocus()" that > I think you can use instead of introducing LayoutSelectionForceHide(). > > Yes, let's use |FrameSelection::HasFocus()| hugoh@ is introducing. > > |LayoutSelectionForceHide()| doesn't represent selection state (has focus or not > focus) correctly. > At this time, we show selection for contenteditable regardless focus. FrameSelection::SelectionHasFocus() and FrameSelection::IsHidden() have landed and they can hopefully unblock this CL. BTW, when using them, doesn't !visible_selection.IsNone() become redundant?
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.
On 2017/05/15 at 15:33:58, hugoh wrote: > https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2847663002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:2067: frame.Selection().LayoutSelectionForceHide()) > On 2017/04/27 09:55:11, yosin_UTC9 wrote: > > On 2017/04/27 at 08:53:50, hugoh_UTC2 wrote: > > > Thanks a lot! I am rewriting the "hide selection" code right now in > > https://codereview.chromium.org/2841093002 . In PS2 I expose a "HasFocus()" that > > I think you can use instead of introducing LayoutSelectionForceHide(). > > > > Yes, let's use |FrameSelection::HasFocus()| hugoh@ is introducing. > > > > |LayoutSelectionForceHide()| doesn't represent selection state (has focus or not > > focus) correctly. > > At this time, we show selection for contenteditable regardless focus. > > FrameSelection::SelectionHasFocus() and FrameSelection::IsHidden() have landed and they can hopefully unblock this CL. > > BTW, when using them, doesn't !visible_selection.IsNone() become redundant? VisibleSelection::IsNone() implies FrameSelection::IsHidden() so it is redundant. I've updated the CL to include your new functions.
non-owner lgtm https://codereview.chromium.org/2847663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2847663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2072: if (!frame.Selection().IsHandleVisible() || frame.Selection().IsHidden()) Thanks for changing order. |FS::IsHidden()| is not cheap function.
amaralp@chromium.org changed reviewers: + bokan@chromium.org
Adding OWNERS aelias@ for ImeTest.java bokan@ for FrameView.cpp
Can you split off the radio input into a new test instead? I think both cases are interesting to test (the radio button is not necessarily superset of plain text). lgtm otherwise
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...
rs lgtm based on other reviewers
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/16 18:03:08, aelias wrote: > Can you split off the radio input into a new test instead? I think both cases > are interesting to test (the radio button is not necessarily superset of plain > text). > > lgtm otherwise Done.
The CQ bit was checked by amaralp@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2847663002/#ps40001 (title: "added old test back")
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": 40001, "attempt_start_ts": 1494992039359690, "parent_rev": "0cd996b5bea03c783b7d10062a669da39e2191da", "commit_rev": "671e6334b0583671f907a52c23a1258b5e1d8770"}
Message was sent while issue was closed.
Description was changed from ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 ========== to ========== Don't show touch handles when selection hidden If the selection is hidden then clear the touch handles. Also changed IME test back to using radio input as it was prior to crrev.com/2616623002. Tapping on a radio button is better to test focus change because it doesn't create a selection. This means that the clearing of the handle has to be done by the focus change and not because a new selection was made. BUG=714525 Review-Url: https://codereview.chromium.org/2847663002 Cr-Commit-Position: refs/heads/master@{#472371} Committed: https://chromium.googlesource.com/chromium/src/+/671e6334b0583671f907a52c23a1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/671e6334b0583671f907a52c23a1... |