Don't show touch handles when selection hidden

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/+/671e6334b0583671f907a52c23a1258b5e1d8770

[amaralp, 2017-04-27 02:51:47]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 02:52:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 04:45:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 04:45:07]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-04-27 08:26:36]

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
==========

[amaralp, 2017-04-27 08:26:48]

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, 2017-04-27 08:27:18]

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, 2017-04-27 08:27:31]

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, 2017-04-27 08:29:03]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org, hugoh@opera.com, yosin@chromium.org

[amaralp, 2017-04-27 08:29:03]

PTAL

[hugoh_UTC2, 2017-04-27 08:53:50]

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().

[yosin_UTC9, 2017-04-27 09:55:11]

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.

[hugoh_UTC2, 2017-05-15 15:33:58]

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?

[amaralp, 2017-05-15 18:26:00]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:26:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 20:41:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 20:41:41]

Dry run: This issue passed the CQ dry run.

[amaralp, 2017-05-15 23:22:48]

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.

[yosin_UTC9, 2017-05-16 01:13:09]

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, 2017-05-16 17:27:56]

amaralp@chromium.org changed reviewers:
+ bokan@chromium.org

[amaralp, 2017-05-16 17:27:57]

Adding OWNERS

aelias@ for ImeTest.java
bokan@ for FrameView.cpp

[aelias_OOO_until_Jul13, 2017-05-16 18:03:08]

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

[amaralp, 2017-05-16 20:11:31]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 20:12:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-05-16 20:23:27]

rs lgtm based on other reviewers

[commit-bot: I haz the power, 2017-05-16 23:27:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 23:27:59]

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_...)

[amaralp1, 2017-05-17 03:32:02]

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.

[amaralp1, 2017-05-17 03:33:59]

The CQ bit was checked by amaralp@google.com

[amaralp1, 2017-05-17 03:33:59]

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")

[commit-bot: I haz the power, 2017-05-17 03:34:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 07:35:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494992039359690,
"parent_rev": "0cd996b5bea03c783b7d10062a669da39e2191da", "commit_rev":
"671e6334b0583671f907a52c23a1258b5e1d8770"}

[commit-bot: I haz the power, 2017-05-17 07:35:18]

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...
==========

[commit-bot: I haz the power, 2017-05-17 07:35:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/671e6334b0583671f907a52c23a1...