Do not send redundant selectionchange-events (decouple focus)

Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input> text-field, a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#464698}
Committed: https://chromium.googlesource.com/chromium/src/+/0f65d25a4097959d977dbb1077717323438240a6

[hugoh_UTC2, 2017-01-04 09:14:11]

hugoh@opera.com changed reviewers:
+ bokan@chromium.org

[hugoh_UTC2, 2017-01-04 09:14:11]

@bokan Could you PTAL at this change?

[bokan, 2017-01-04 12:59:52]

Description was changed from

==========
Do not send redundant ViewHostMsg_TextInputStateChanged-message

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an input field (unnecessary!).
 (2) when focus enters another input field.

Solution:
When we know that focus enters a new input-field, we skip the
first call (1) to RenderFrameImpl::didChangeSelection.

BUG=Performance + Fixes a UI glitch in downstream product.
TEST=1. In content_shell, select some text in an <input>-field,
        click another <input>-field (move focus).
        Notice: selection cleared.
     2. run-webkit-tests LayoutTests/editing/selection
     3. content_browsertests --gtest_filter=RenderView*
==========

to

==========
Do not send redundant ViewHostMsg_TextInputStateChanged-message

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an input field (unnecessary!).
 (2) when focus enters another input field.

Solution:
When we know that focus enters a new input-field, we skip the
first call (1) to RenderFrameImpl::didChangeSelection.

BUG=Performance + Fixes a UI glitch in downstream product.
TEST=1. In content_shell, select some text in an <input>-field,
        click another <input>-field (move focus).
        Notice: selection cleared.
     2. run-webkit-tests LayoutTests/editing/selection
     3. content_browsertests --gtest_filter=RenderView*
==========

[bokan, 2017-01-04 12:59:52]

bokan@chromium.org changed reviewers:
- bokan@chromium.org

[bokan, 2017-01-04 13:01:31]

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

[bokan, 2017-01-04 13:01:40]

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

[bokan, 2017-01-04 13:05:14]

I'm not super familiar with focus/editing so +yosin@ to look over this change
and I'll happily provide an OWNER stamp after.

One comment though, this change will need an automated test. I don't know this
code enough to tell how this early return will prevent the TestInputStateChanged
message but presumably some condition could be checked solely within Blink so a
Blink unit test would probably suffice.

Also, please file a bug on crbug.com and link it's # in the BUG= field.

[yosin_UTC9, 2017-01-05 05:35:32]

yosin@chromium.org changed reviewers:
+ kochi@chromium.org

[yosin_UTC9, 2017-01-05 05:35:33]

As bokan said, please file bug and add test for verifying this change to avoid
regression in future.

+kochi@, who is an owner of FocusController.

https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
(newFocusedElement && newFocusedElement->isTextControl())
Should we also bail out for content editable?

Rather than adding more condition, I think it is better to get rid of this
function, clearSelecitonIfNeeded()
and ask FrameSelection to clear selection if new focus element can't have
selection.

[hugoh_UTC2, 2017-01-05 14:14:48]

Description was changed from

==========
Do not send redundant ViewHostMsg_TextInputStateChanged-message

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an input field (unnecessary!).
 (2) when focus enters another input field.

Solution:
When we know that focus enters a new input-field, we skip the
first call (1) to RenderFrameImpl::didChangeSelection.

BUG=Performance + Fixes a UI glitch in downstream product.
TEST=1. In content_shell, select some text in an <input>-field,
        click another <input>-field (move focus).
        Notice: selection cleared.
     2. run-webkit-tests LayoutTests/editing/selection
     3. content_browsertests --gtest_filter=RenderView*
==========

to

==========
Do not send redundant ViewHostMsg_TextInputStateChanged-message

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an input field (unnecessary!).
 (2) when focus enters another input field.

Solution:
When we know that focus enters a new input-field, we skip the
first call (1) to RenderFrameImpl::didChangeSelection.

BUG=678601
TEST=1. In content_shell, select some text in an <input>-field,
        click another <input>-field (move focus).
        Notice: selection cleared.
     2. run-webkit-tests LayoutTests/editing/selection
     3. content_browsertests --gtest_filter=RenderView*
==========

[hugoh_UTC2, 2017-01-05 14:47:33]

I've created crbug.com/678601 . I'm not sure how/where to test this
automatically.

[bokan, 2017-01-05 14:49:21]

On 2017/01/05 14:47:33, hugoh wrote:
> I've created crbug.com/678601 . I'm not sure how/where to test this
> automatically.

I don't know myself and Yosin might know better, but either look through
WebFrameTest.cpp/WebViewTest.cpp to look for similar tests or look through blame
at patches in a similar area and see where they added tests.

[hugoh_UTC2, 2017-01-06 17:31:44]

https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
(newFocusedElement && newFocusedElement->isTextControl())
On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> Should we also bail out for content editable?
> 
Yes, you're right. When focus jumps from <input> to <p contenteditable> I see
that Blink calls into didChangeSelection twice. I added another condition for
this and a unit test that also checks this. 

> Rather than adding more condition, I think it is better to get rid of this
> function, clearSelecitonIfNeeded()
> and ask FrameSelection to clear selection if new focus element can't have
> selection.
You mean clear the selection if the new focused element _can_ have a selection?
Or, does it matter? Don't we always want to clear the selection when focus
moves?

[yosin_UTC9, 2017-01-10 07:30:47]

On 2017/01/06 at 17:31:44, hugoh wrote:
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
(newFocusedElement && newFocusedElement->isTextControl())
> On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > Should we also bail out for content editable?
> > 
> Yes, you're right. When focus jumps from <input> to <p contenteditable> I see
that Blink calls into didChangeSelection twice. I added another condition for
this and a unit test that also checks this. 
> 
> > Rather than adding more condition, I think it is better to get rid of this
> > function, clearSelecitonIfNeeded()
> > and ask FrameSelection to clear selection if new focus element can't have
> > selection.
> You mean clear the selection if the new focused element _can_ have a
selection? Or, does it matter? Don't we always want to clear the selection when
focus moves?

No, we don't need to clear selection here. Clearing selection should be done in
moving selection. This is Blink/WebKit specific behavior . We should align to
Firefox and Edge[1] behavior, not clear selection, since I think it is
reasonable. Note: the spec[2] doesn't mention about selection behavior on
focus()/blur().

[1] http://crbug.com/679635 Setting focus should not clear selection
[2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus

[hugoh_UTC2, 2017-01-12 02:35:45]

On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> On 2017/01/06 at 17:31:44, hugoh wrote:
> >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> (newFocusedElement && newFocusedElement->isTextControl())
> > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > Should we also bail out for content editable?
> > > 
> > Yes, you're right. When focus jumps from <input> to <p contenteditable> I
see
> that Blink calls into didChangeSelection twice. I added another condition for
> this and a unit test that also checks this. 
> > 
> > > Rather than adding more condition, I think it is better to get rid of this
> > > function, clearSelecitonIfNeeded()
> > > and ask FrameSelection to clear selection if new focus element can't have
> > > selection.
> > You mean clear the selection if the new focused element _can_ have a
> selection? Or, does it matter? Don't we always want to clear the selection
when
> focus moves?
> 
> No, we don't need to clear selection here. Clearing selection should be done
in
> moving selection. This is Blink/WebKit specific behavior . We should align to
> Firefox and Edge[1] behavior, not clear selection, since I think it is
> reasonable. Note: the spec[2] doesn't mention about selection behavior on
> focus()/blur().
> 
> [1] http://crbug.com/679635 Setting focus should not clear selection
> [2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
So in this CL we simply remove clearSelectionIfNeeded() and the calls to it?
PTAL at Patch Set #3.

[yosin_UTC9, 2017-01-12 04:07:30]

On 2017/01/12 at 02:35:45, hugoh wrote:
> On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > On 2017/01/06 at 17:31:44, hugoh wrote:
> > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > (newFocusedElement && newFocusedElement->isTextControl())
> > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > Should we also bail out for content editable?
> > > > 
> > > Yes, you're right. When focus jumps from <input> to <p contenteditable> I
see
> > that Blink calls into didChangeSelection twice. I added another condition
for
> > this and a unit test that also checks this. 
> > > 
> > > > Rather than adding more condition, I think it is better to get rid of
this
> > > > function, clearSelecitonIfNeeded()
> > > > and ask FrameSelection to clear selection if new focus element can't
have
> > > > selection.
> > > You mean clear the selection if the new focused element _can_ have a
> > selection? Or, does it matter? Don't we always want to clear the selection
when
> > focus moves?
> > 
> > No, we don't need to clear selection here. Clearing selection should be done
in
> > moving selection. This is Blink/WebKit specific behavior . We should align
to
> > Firefox and Edge[1] behavior, not clear selection, since I think it is
> > reasonable. Note: the spec[2] doesn't mention about selection behavior on
> > focus()/blur().
> > 
> > [1] http://crbug.com/679635 Setting focus should not clear selection
> > [2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> So in this CL we simply remove clearSelectionIfNeeded() and the calls to it?
PTAL at Patch Set #3.

lgtm

Could you wait to commit until this patch land [1]?
Without [1], Blink insert a character into non-focused INPUT element.
clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
[1] prevents this for INPUT/TEXTAREA and content editable, as generic solution.
[1] should be landed by this Friday Japan time.

Thanks for your understanding and cooperation. m(_ _)m

[1] http://crrev.co2628763003 Keyboard event should not insert a character at
selection if selection doesn't have focus

[hugoh_UTC2, 2017-01-12 05:46:07]

On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> On 2017/01/12 at 02:35:45, hugoh wrote:
> > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > Should we also bail out for content editable?
> > > > > 
> > > > Yes, you're right. When focus jumps from <input> to <p contenteditable>
I
> see
> > > that Blink calls into didChangeSelection twice. I added another condition
> for
> > > this and a unit test that also checks this. 
> > > > 
> > > > > Rather than adding more condition, I think it is better to get rid of
> this
> > > > > function, clearSelecitonIfNeeded()
> > > > > and ask FrameSelection to clear selection if new focus element can't
> have
> > > > > selection.
> > > > You mean clear the selection if the new focused element _can_ have a
> > > selection? Or, does it matter? Don't we always want to clear the selection
> when
> > > focus moves?
> > > 
> > > No, we don't need to clear selection here. Clearing selection should be
done
> in
> > > moving selection. This is Blink/WebKit specific behavior . We should align
> to
> > > Firefox and Edge[1] behavior, not clear selection, since I think it is
> > > reasonable. Note: the spec[2] doesn't mention about selection behavior on
> > > focus()/blur().
> > > 
> > > [1] http://crbug.com/679635 Setting focus should not clear selection
> > > [2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > So in this CL we simply remove clearSelectionIfNeeded() and the calls to it?
> PTAL at Patch Set #3.
> 
> lgtm
> 
> Could you wait to commit until this patch land [1]?
> Without [1], Blink insert a character into non-focused INPUT element.
> clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> [1] prevents this for INPUT/TEXTAREA and content editable, as generic
solution.
> [1] should be landed by this Friday Japan time.
> 
> Thanks for your understanding and cooperation. m(_ _)m
> 
> [1] http://crrev.co2628763003 Keyboard event should not insert a character at
> selection if selection doesn't have focus

Thank you for digging into this! I will wait for [1] to land. Meanwhile, could
you rubber stamp, bokan@?

[bokan, 2017-01-12 14:07:15]

rs lgtm

[hugoh_UTC2, 2017-01-16 01:13:22]

The CQ bit was checked by hugoh@opera.com

[commit-bot: I haz the power, 2017-01-16 01:13:28]

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-01-16 02:20:51]

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

[commit-bot: I haz the power, 2017-01-16 02:20:52]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hugoh_UTC2, 2017-01-17 01:43:52]

On 2017/01/12 05:46:07, hugoh wrote:
> On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> > On 2017/01/12 at 02:35:45, hugoh wrote:
> > > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > > >
> > > >
> >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> > > > > 
> > > > >
> > > >
> >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > > Should we also bail out for content editable?
> > > > > > 
> > > > > Yes, you're right. When focus jumps from <input> to <p
contenteditable>
> I
> > see
> > > > that Blink calls into didChangeSelection twice. I added another
condition
> > for
> > > > this and a unit test that also checks this. 
> > > > > 
> > > > > > Rather than adding more condition, I think it is better to get rid
of
> > this
> > > > > > function, clearSelecitonIfNeeded()
> > > > > > and ask FrameSelection to clear selection if new focus element can't
> > have
> > > > > > selection.
> > > > > You mean clear the selection if the new focused element _can_ have a
> > > > selection? Or, does it matter? Don't we always want to clear the
selection
> > when
> > > > focus moves?
> > > > 
> > > > No, we don't need to clear selection here. Clearing selection should be
> done
> > in
> > > > moving selection. This is Blink/WebKit specific behavior . We should
align
> > to
> > > > Firefox and Edge[1] behavior, not clear selection, since I think it is
> > > > reasonable. Note: the spec[2] doesn't mention about selection behavior
on
> > > > focus()/blur().
> > > > 
> > > > [1] http://crbug.com/679635 Setting focus should not clear selection
> > > > [2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > > So in this CL we simply remove clearSelectionIfNeeded() and the calls to
it?
> > PTAL at Patch Set #3.
> > 
> > lgtm
> > 
> > Could you wait to commit until this patch land [1]?
> > Without [1], Blink insert a character into non-focused INPUT element.
> > clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> > [1] prevents this for INPUT/TEXTAREA and content editable, as generic
> solution.
> > [1] should be landed by this Friday Japan time.
> > 
> > Thanks for your understanding and cooperation. m(_ _)m
> > 
> > [1] http://crrev.co2628763003 Keyboard event should not insert a character
at
> > selection if selection doesn't have focus
> 
> Thank you for digging into this! I will wait for [1] to land. Meanwhile, could
> you rubber stamp, bokan@?

According to the bots, the new test of [1], keyboard_event_without_focus.html,
failed. Do you see why, yosin@ ?

[yosin_UTC9, 2017-01-17 02:05:39]

On 2017/01/17 at 01:43:52, hugoh wrote:
> On 2017/01/12 05:46:07, hugoh wrote:
> > On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> > > On 2017/01/12 at 02:35:45, hugoh wrote:
> > > > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > > > >
> > > > >
> > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > File third_party/WebKit/Source/core/page/FocusController.cpp
(right):
> > > > > > 
> > > > > >
> > > > >
> > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > > > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > > > Should we also bail out for content editable?
> > > > > > > 
> > > > > > Yes, you're right. When focus jumps from <input> to <p
contenteditable>
> > I
> > > see
> > > > > that Blink calls into didChangeSelection twice. I added another
condition
> > > for
> > > > > this and a unit test that also checks this. 
> > > > > > 
> > > > > > > Rather than adding more condition, I think it is better to get rid
of
> > > this
> > > > > > > function, clearSelecitonIfNeeded()
> > > > > > > and ask FrameSelection to clear selection if new focus element
can't
> > > have
> > > > > > > selection.
> > > > > > You mean clear the selection if the new focused element _can_ have a
> > > > > selection? Or, does it matter? Don't we always want to clear the
selection
> > > when
> > > > > focus moves?
> > > > > 
> > > > > No, we don't need to clear selection here. Clearing selection should
be
> > done
> > > in
> > > > > moving selection. This is Blink/WebKit specific behavior . We should
align
> > > to
> > > > > Firefox and Edge[1] behavior, not clear selection, since I think it is
> > > > > reasonable. Note: the spec[2] doesn't mention about selection behavior
on
> > > > > focus()/blur().
> > > > > 
> > > > > [1] http://crbug.com/679635 Setting focus should not clear selection
> > > > > [2] https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > > > So in this CL we simply remove clearSelectionIfNeeded() and the calls to
it?
> > > PTAL at Patch Set #3.
> > > 
> > > lgtm
> > > 
> > > Could you wait to commit until this patch land [1]?
> > > Without [1], Blink insert a character into non-focused INPUT element.
> > > clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> > > [1] prevents this for INPUT/TEXTAREA and content editable, as generic
> > solution.
> > > [1] should be landed by this Friday Japan time.
> > > 
> > > Thanks for your understanding and cooperation. m(_ _)m
> > > 
> > > [1] http://crrev.co2628763003 Keyboard event should not insert a character
at
> > > selection if selection doesn't have focus
> > 
> > Thank you for digging into this! I will wait for [1] to land. Meanwhile,
could
> > you rubber stamp, bokan@?
> 
> According to the bots, the new test of [1], keyboard_event_without_focus.html,
failed. Do you see why, yosin@ ?

TL;DR: Please update expectation to actual result == having caret/selection
where "|" indicate caret/selection.

Here is the result of keyboard_event_without_focus.html:
	 expected <input value="x">x</input><input type="checkbox">,
	 but got  |<input value="x">x</input><input type="checkbox">,

Before this patch, selection is cleared at focus change, but this patch leaves
selection as is. So, selection is
still in text field; note the test put "|" before text field as
window.getSelection() result.

Other test failures are expected changes, so we can simply update test
expectations.

One sample:

---
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
+++
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-actual.txt
@@ -34,7 +34,6 @@
 Received mouseout on target
 PASS tapHandled is false
 Received selectionchange on #document anchor=#text[0]
-Received selectionchange on #document anchor=#text[0]
 PASS isFocused(target) is false
 
 PASS successfullyParsed is true

This is good indication that this patch gets rid of redundant "selectionchange"
event. (^_^)b

[hugoh_UTC2, 2017-01-17 03:44:08]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 03:44:15]

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-01-17 05:10:10]

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

[commit-bot: I haz the power, 2017-01-17 05:10:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hugoh_UTC2, 2017-01-17 07:36:10]

On 2017/01/17 02:05:39, Yosi_UTC9 wrote:
> On 2017/01/17 at 01:43:52, hugoh wrote:
> > On 2017/01/12 05:46:07, hugoh wrote:
> > > On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> > > > On 2017/01/12 at 02:35:45, hugoh wrote:
> > > > > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > > > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > > > > >
> > > > > >
> > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > File third_party/WebKit/Source/core/page/FocusController.cpp
> (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > > > > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > > > > Should we also bail out for content editable?
> > > > > > > > 
> > > > > > > Yes, you're right. When focus jumps from <input> to <p
> contenteditable>
> > > I
> > > > see
> > > > > > that Blink calls into didChangeSelection twice. I added another
> condition
> > > > for
> > > > > > this and a unit test that also checks this. 
> > > > > > > 
> > > > > > > > Rather than adding more condition, I think it is better to get
rid
> of
> > > > this
> > > > > > > > function, clearSelecitonIfNeeded()
> > > > > > > > and ask FrameSelection to clear selection if new focus element
> can't
> > > > have
> > > > > > > > selection.
> > > > > > > You mean clear the selection if the new focused element _can_ have
a
> > > > > > selection? Or, does it matter? Don't we always want to clear the
> selection
> > > > when
> > > > > > focus moves?
> > > > > > 
> > > > > > No, we don't need to clear selection here. Clearing selection should
> be
> > > done
> > > > in
> > > > > > moving selection. This is Blink/WebKit specific behavior . We should
> align
> > > > to
> > > > > > Firefox and Edge[1] behavior, not clear selection, since I think it
is
> > > > > > reasonable. Note: the spec[2] doesn't mention about selection
behavior
> on
> > > > > > focus()/blur().
> > > > > > 
> > > > > > [1] http://crbug.com/679635 Setting focus should not clear selection
> > > > > > [2]
https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > > > > So in this CL we simply remove clearSelectionIfNeeded() and the calls
to
> it?
> > > > PTAL at Patch Set #3.
> > > > 
> > > > lgtm
> > > > 
> > > > Could you wait to commit until this patch land [1]?
> > > > Without [1], Blink insert a character into non-focused INPUT element.
> > > > clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> > > > [1] prevents this for INPUT/TEXTAREA and content editable, as generic
> > > solution.
> > > > [1] should be landed by this Friday Japan time.
> > > > 
> > > > Thanks for your understanding and cooperation. m(_ _)m
> > > > 
> > > > [1] http://crrev.co2628763003 Keyboard event should not insert a
character
> at
> > > > selection if selection doesn't have focus
> > > 
> > > Thank you for digging into this! I will wait for [1] to land. Meanwhile,
> could
> > > you rubber stamp, bokan@?
> > 
> > According to the bots, the new test of [1],
keyboard_event_without_focus.html,
> failed. Do you see why, yosin@ ?
> 
> TL;DR: Please update expectation to actual result == having caret/selection
> where "|" indicate caret/selection.
> 
> Here is the result of keyboard_event_without_focus.html:
> 	 expected <input value="x">x</input><input type="checkbox">,
> 	 but got  |<input value="x">x</input><input type="checkbox">,
> 
> Before this patch, selection is cleared at focus change, but this patch leaves
> selection as is. So, selection is
> still in text field; note the test put "|" before text field as
> window.getSelection() result.
> 
> Other test failures are expected changes, so we can simply update test
> expectations.
> 
> One sample:
> 
> ---
>
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
> +++
>
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-actual.txt
> @@ -34,7 +34,6 @@
>  Received mouseout on target
>  PASS tapHandled is false
>  Received selectionchange on #document anchor=#text[0]
> -Received selectionchange on #document anchor=#text[0]
>  PASS isFocused(target) is false
>  
>  PASS successfullyParsed is true
> 
> This is good indication that this patch gets rid of redundant
"selectionchange"
> event. (^_^)b

Ok! I'm not sure about focus-and-display-none-and-redisplay.html. Now it fails
because it is possible to type into the <textarea> that has style.display =
'none'. A blur-event comes when the <textarea> hides, but the selection remains
inside it.

[yosin_UTC9, 2017-01-18 04:20:06]

On 2017/01/17 at 07:36:10, hugoh wrote:
> On 2017/01/17 02:05:39, Yosi_UTC9 wrote:
> > On 2017/01/17 at 01:43:52, hugoh wrote:
> > > On 2017/01/12 05:46:07, hugoh wrote:
> > > > On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> > > > > On 2017/01/12 at 02:35:45, hugoh wrote:
> > > > > > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > > > > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > > File third_party/WebKit/Source/core/page/FocusController.cpp
> > (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> >
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079: if
> > > > > > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > > > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > > > > > Should we also bail out for content editable?
> > > > > > > > > 
> > > > > > > > Yes, you're right. When focus jumps from <input> to <p
> > contenteditable>
> > > > I
> > > > > see
> > > > > > > that Blink calls into didChangeSelection twice. I added another
> > condition
> > > > > for
> > > > > > > this and a unit test that also checks this. 
> > > > > > > > 
> > > > > > > > > Rather than adding more condition, I think it is better to get
rid
> > of
> > > > > this
> > > > > > > > > function, clearSelecitonIfNeeded()
> > > > > > > > > and ask FrameSelection to clear selection if new focus element
> > can't
> > > > > have
> > > > > > > > > selection.
> > > > > > > > You mean clear the selection if the new focused element _can_
have a
> > > > > > > selection? Or, does it matter? Don't we always want to clear the
> > selection
> > > > > when
> > > > > > > focus moves?
> > > > > > > 
> > > > > > > No, we don't need to clear selection here. Clearing selection
should
> > be
> > > > done
> > > > > in
> > > > > > > moving selection. This is Blink/WebKit specific behavior . We
should
> > align
> > > > > to
> > > > > > > Firefox and Edge[1] behavior, not clear selection, since I think
it is
> > > > > > > reasonable. Note: the spec[2] doesn't mention about selection
behavior
> > on
> > > > > > > focus()/blur().
> > > > > > > 
> > > > > > > [1] http://crbug.com/679635 Setting focus should not clear
selection
> > > > > > > [2]
https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > > > > > So in this CL we simply remove clearSelectionIfNeeded() and the
calls to
> > it?
> > > > > PTAL at Patch Set #3.
> > > > > 
> > > > > lgtm
> > > > > 
> > > > > Could you wait to commit until this patch land [1]?
> > > > > Without [1], Blink insert a character into non-focused INPUT element.
> > > > > clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> > > > > [1] prevents this for INPUT/TEXTAREA and content editable, as generic
> > > > solution.
> > > > > [1] should be landed by this Friday Japan time.
> > > > > 
> > > > > Thanks for your understanding and cooperation. m(_ _)m
> > > > > 
> > > > > [1] http://crrev.co2628763003 Keyboard event should not insert a
character
> > at
> > > > > selection if selection doesn't have focus
> > > > 
> > > > Thank you for digging into this! I will wait for [1] to land. Meanwhile,
> > could
> > > > you rubber stamp, bokan@?
> > > 
> > > According to the bots, the new test of [1],
keyboard_event_without_focus.html,
> > failed. Do you see why, yosin@ ?
> > 
> > TL;DR: Please update expectation to actual result == having caret/selection
> > where "|" indicate caret/selection.
> > 
> > Here is the result of keyboard_event_without_focus.html:
> > 	 expected <input value="x">x</input><input type="checkbox">,
> > 	 but got  |<input value="x">x</input><input type="checkbox">,
> > 
> > Before this patch, selection is cleared at focus change, but this patch
leaves
> > selection as is. So, selection is
> > still in text field; note the test put "|" before text field as
> > window.getSelection() result.
> > 
> > Other test failures are expected changes, so we can simply update test
> > expectations.
> > 
> > One sample:
> > 
> > ---
> >
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
> > +++
> >
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-actual.txt
> > @@ -34,7 +34,6 @@
> >  Received mouseout on target
> >  PASS tapHandled is false
> >  Received selectionchange on #document anchor=#text[0]
> > -Received selectionchange on #document anchor=#text[0]
> >  PASS isFocused(target) is false
> >  
> >  PASS successfullyParsed is true
> > 
> > This is good indication that this patch gets rid of redundant
"selectionchange"
> > event. (^_^)b
> 
> Ok! I'm not sure about focus-and-display-none-and-redisplay.html. Now it fails
because it is possible to type into the <textarea> that has style.display =
'none'. A blur-event comes when the <textarea> hides, but the selection remains
inside it.

New behavior of "focus-and-display-none-and-redisplay.html" is expected and
correct. Since, selection is still in TEXTAREA.
Firefox and Edge behave as same as current Blink because both of them ignore
execCommand for TEXTAREA[1]

[1] http://crbug.com/558919 execCommand should not work on INPUT/TEXTAREA

[hugoh_UTC2, 2017-01-18 05:44:56]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 05:45:13]

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-01-18 06:49:32]

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

[commit-bot: I haz the power, 2017-01-18 06:49:32]

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

[hugoh_UTC2, 2017-01-18 06:55:53]

On 2017/01/18 04:20:06, Yosi_UTC9 wrote:
> On 2017/01/17 at 07:36:10, hugoh wrote:
> > On 2017/01/17 02:05:39, Yosi_UTC9 wrote:
> > > On 2017/01/17 at 01:43:52, hugoh wrote:
> > > > On 2017/01/12 05:46:07, hugoh wrote:
> > > > > On 2017/01/12 04:07:30, Yosi_UTC9 wrote:
> > > > > > On 2017/01/12 at 02:35:45, hugoh wrote:
> > > > > > > On 2017/01/10 07:30:47, Yosi_UTC9 wrote:
> > > > > > > > On 2017/01/06 at 17:31:44, hugoh wrote:
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > > > File third_party/WebKit/Source/core/page/FocusController.cpp
> > > (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2616623002/diff/1/third_party/WebKit/Source/c...
> > > > > > > > > third_party/WebKit/Source/core/page/FocusController.cpp:1079:
if
> > > > > > > > (newFocusedElement && newFocusedElement->isTextControl())
> > > > > > > > > On 2017/01/05 05:35:33, Yosi_UTC9 wrote:
> > > > > > > > > > Should we also bail out for content editable?
> > > > > > > > > > 
> > > > > > > > > Yes, you're right. When focus jumps from <input> to <p
> > > contenteditable>
> > > > > I
> > > > > > see
> > > > > > > > that Blink calls into didChangeSelection twice. I added another
> > > condition
> > > > > > for
> > > > > > > > this and a unit test that also checks this. 
> > > > > > > > > 
> > > > > > > > > > Rather than adding more condition, I think it is better to
get
> rid
> > > of
> > > > > > this
> > > > > > > > > > function, clearSelecitonIfNeeded()
> > > > > > > > > > and ask FrameSelection to clear selection if new focus
element
> > > can't
> > > > > > have
> > > > > > > > > > selection.
> > > > > > > > > You mean clear the selection if the new focused element _can_
> have a
> > > > > > > > selection? Or, does it matter? Don't we always want to clear the
> > > selection
> > > > > > when
> > > > > > > > focus moves?
> > > > > > > > 
> > > > > > > > No, we don't need to clear selection here. Clearing selection
> should
> > > be
> > > > > done
> > > > > > in
> > > > > > > > moving selection. This is Blink/WebKit specific behavior . We
> should
> > > align
> > > > > > to
> > > > > > > > Firefox and Edge[1] behavior, not clear selection, since I think
> it is
> > > > > > > > reasonable. Note: the spec[2] doesn't mention about selection
> behavior
> > > on
> > > > > > > > focus()/blur().
> > > > > > > > 
> > > > > > > > [1] http://crbug.com/679635 Setting focus should not clear
> selection
> > > > > > > > [2]
> https://html.spec.whatwg.org/multipage/interaction.html#dom-focus
> > > > > > > So in this CL we simply remove clearSelectionIfNeeded() and the
> calls to
> > > it?
> > > > > > PTAL at Patch Set #3.
> > > > > > 
> > > > > > lgtm
> > > > > > 
> > > > > > Could you wait to commit until this patch land [1]?
> > > > > > Without [1], Blink insert a character into non-focused INPUT
element.
> > > > > > clearSelectionIfNeeded() prevents this situation for INPUT/TEXTAREA.
> > > > > > [1] prevents this for INPUT/TEXTAREA and content editable, as
generic
> > > > > solution.
> > > > > > [1] should be landed by this Friday Japan time.
> > > > > > 
> > > > > > Thanks for your understanding and cooperation. m(_ _)m
> > > > > > 
> > > > > > [1] http://crrev.co2628763003 Keyboard event should not insert a
> character
> > > at
> > > > > > selection if selection doesn't have focus
> > > > > 
> > > > > Thank you for digging into this! I will wait for [1] to land.
Meanwhile,
> > > could
> > > > > you rubber stamp, bokan@?
> > > > 
> > > > According to the bots, the new test of [1],
> keyboard_event_without_focus.html,
> > > failed. Do you see why, yosin@ ?
> > > 
> > > TL;DR: Please update expectation to actual result == having
caret/selection
> > > where "|" indicate caret/selection.
> > > 
> > > Here is the result of keyboard_event_without_focus.html:
> > > 	 expected <input value="x">x</input><input type="checkbox">,
> > > 	 but got  |<input value="x">x</input><input type="checkbox">,
> > > 
> > > Before this patch, selection is cleared at focus change, but this patch
> leaves
> > > selection as is. So, selection is
> > > still in text field; note the test put "|" before text field as
> > > window.getSelection() result.
> > > 
> > > Other test failures are expected changes, so we can simply update test
> > > expectations.
> > > 
> > > One sample:
> > > 
> > > ---
> > >
>
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
> > > +++
> > >
>
/b/rr/tmp0uAg6L/w/layout-test-results/fast/events/touch/gesture/focus-selectionchange-on-tap-actual.txt
> > > @@ -34,7 +34,6 @@
> > >  Received mouseout on target
> > >  PASS tapHandled is false
> > >  Received selectionchange on #document anchor=#text[0]
> > > -Received selectionchange on #document anchor=#text[0]
> > >  PASS isFocused(target) is false
> > >  
> > >  PASS successfullyParsed is true
> > > 
> > > This is good indication that this patch gets rid of redundant
> "selectionchange"
> > > event. (^_^)b
> > 
> > Ok! I'm not sure about focus-and-display-none-and-redisplay.html. Now it
fails
> because it is possible to type into the <textarea> that has style.display =
> 'none'. A blur-event comes when the <textarea> hides, but the selection
remains
> inside it.
> 
> New behavior of "focus-and-display-none-and-redisplay.html" is expected and
> correct. Since, selection is still in TEXTAREA.
> Firefox and Edge behave as same as current Blink because both of them ignore
> execCommand for TEXTAREA[1]
> 
> [1] http://crbug.com/558919 execCommand should not work on INPUT/TEXTAREA

Ok. I've updated focus-and-display-none-and-redisplay.html, PTAL.

$ python third_party/WebKit/Tools/Scripts/run-webkit-tests
fast/spatial-navigation
Regressions: Unexpected text-only failures (2)
  fast/spatial-navigation/snav-input.html [ Failure ]
  fast/spatial-navigation/snav-textarea.html [ Failure ]

I tested spatial navigation manually. I see two unwanted changes with this CL:
0. ./content_shell --enable-spatial-navigation -u "data:text/html,<table border>
\
<tr><td></td><td><a href>A</a></td><td></td></tr> \
<tr><td><a href>B</a></td><td><input autofocus value=text></td><td><a
href>C</a></td></tr> \
<tr><td></td><td><a href>D</a></td><td></td></tr></table>"

1. Use arrow-key right to go to C.
2. Tap arrow-key left to go back to text.
Now: tex|t
Before this CL: [text] (OK! text is completely selected)

3. Use arrow-key left to go to B.
4. Tap arrow-key down.
Now: cursor jumps to text (because selection is not cleared?).
Before: cursor jumps to D (OK! because D is south of B)

[hugoh_UTC2, 2017-01-18 09:20:03]

https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:670:
assertWaitForSelectActionBarStatus(true);
I flipped this boolean just to make the test pass. But I'm not sure it is
correct. Even when the selection stays uncleared, we should hide the action bar,
when we click outside the <input>? How do we otherwise get rid of the action
bar?

[yosin_UTC9, 2017-01-19 09:32:31]

On 2017/01/18 at 09:20:03, hugoh wrote:
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):
> 
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:670:
assertWaitForSelectActionBarStatus(true);
> I flipped this boolean just to make the test pass. But I'm not sure it is
correct. Even when the selection stays uncleared, we should hide the action bar,
when we click outside the <input>? How do we otherwise get rid of the action
bar?

changwan@ and yanbin@, any clue for ImeTest.java?

[Changwan Ryu, 2017-01-19 11:10:08]

On 2017/01/18 09:20:03, hugoh wrote:
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> (right):
> 
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:670:
> assertWaitForSelectActionBarStatus(true);
> I flipped this boolean just to make the test pass. But I'm not sure it is
> correct. Even when the selection stays uncleared, we should hide the action
bar,
> when we click outside the <input>? How do we otherwise get rid of the action
> bar?

I don't think it's correct.
But before getting into action bar, is it ok to leave the selection uncleared
when radio input is focused?
I think it's very confusing user experience - both selection highlight and the
dot in radio button are indicative of focus within document so far.

[hugoh_UTC2, 2017-01-20 08:49:09]

On 2017/01/19 11:10:08, Changwan Ryu wrote:
> On 2017/01/18 09:20:03, hugoh wrote:
> >
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> > File
> >
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> >
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:670:
> > assertWaitForSelectActionBarStatus(true);
> > I flipped this boolean just to make the test pass. But I'm not sure it is
> > correct. Even when the selection stays uncleared, we should hide the action
> bar,
> > when we click outside the <input>? How do we otherwise get rid of the action
> > bar?
> 
> I don't think it's correct.
> But before getting into action bar, is it ok to leave the selection uncleared
> when radio input is focused?
> I think it's very confusing user experience - both selection highlight and the
> dot in radio button are indicative of focus within document so far.

I agree. So there seems like we have (at least) two cases where
selection.clear() is still needed. One for above (A) ImeTest.java and one for
(B) spatial navigation... I guess we could add two extra calls to
selection.clear() but from where...? I need to study focus+selection+spatnav
more to really understand this. Could we go back to Patch Set 2 (my initial fix
for the bug) and see if (A) + (B) are green with that change?

[yosin_UTC9, 2017-01-20 09:00:24]

On 2017/01/20 at 08:49:09, hugoh wrote:
> On 2017/01/19 11:10:08, Changwan Ryu wrote:
> > On 2017/01/18 09:20:03, hugoh wrote:
> > >
> >
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> > > File
> > >
> >
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> > > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2616623002/diff/80001/content/public/android/...
> > >
> >
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:670:
> > > assertWaitForSelectActionBarStatus(true);
> > > I flipped this boolean just to make the test pass. But I'm not sure it is
> > > correct. Even when the selection stays uncleared, we should hide the
action
> > bar,
> > > when we click outside the <input>? How do we otherwise get rid of the
action
> > > bar?
> > 
> > I don't think it's correct.
> > But before getting into action bar, is it ok to leave the selection
uncleared
> > when radio input is focused?
> > I think it's very confusing user experience - both selection highlight and
the
> > dot in radio button are indicative of focus within document so far.
> 
> I agree. So there seems like we have (at least) two cases where
selection.clear() is still needed. One for above (A) ImeTest.java and one for
(B) spatial navigation... I guess we could add two extra calls to
selection.clear() but from where...? I need to study focus+selection+spatnav
more to really understand this. Could we go back to Patch Set 2 (my initial fix
for the bug) and see if (A) + (B) are green with that change?

I think we should go Patch Set 6+, since clearing selection isn't web
compatible.

> > I think it's very confusing user experience - both selection highlight and
the
> > dot in radio button are indicative of focus within document so far.
If selection doesn't have focus, it is painted with gray instead of blue
(platform dependent though).
It doesn't confuse user.

For action bar and spatial navigation, they should subscribe focus change and
they need to do appropriate thing.

[aelias_OOO_until_Jul13, 2017-01-24 00:51:40]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-01-24 00:51:41]

https://codereview.chromium.org/2616623002/diff/100001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2616623002/diff/100001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:668:
DOMUtils.clickNode(this, mContentViewCore, "input_radio");
I can't repro any obvious change to action bar behavior when trying out locally,
and radio button scenario doesn't really matter, so I think the test is
unnecessarily specific.  I suggest changing to "plain_text" and also adding an
additional <br/> above the "plain_text" element in input_forms.html (because
touch adjustment fudge factor would cause such taps to hit nearby input box
otherwise); I tested locally the test passes with that.

[Changwan Ryu, 2017-01-24 03:47:16]

On 2017/01/24 00:51:41, aelias wrote:
>
https://codereview.chromium.org/2616623002/diff/100001/content/public/android...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> (right):
> 
>
https://codereview.chromium.org/2616623002/diff/100001/content/public/android...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:668:
> DOMUtils.clickNode(this, mContentViewCore, "input_radio");
> I can't repro any obvious change to action bar behavior when trying out
locally,
> and radio button scenario doesn't really matter, so I think the test is
> unnecessarily specific.  I suggest changing to "plain_text" and also adding an
> additional <br/> above the "plain_text" element in input_forms.html (because
> touch adjustment fudge factor would cause such taps to hit nearby input box
> otherwise); I tested locally the test passes with that.

One problem I found when patching and testing this CL locally:

Long-press selection then selection handles get dismissed after <1 second.
This might be related to what I'm trying to fix at
https://codereview.chromium.org/2646963002/, but haven't looked into too much.

Also selection highlight does not get grayed out when radio_input takes the
focus, but I guess that's not high priority.

input_radio is not an important scenario, but it may still help to have a test
for focus loss scenario.
How about replacing the line with JavaScript blur()?

[aelias_OOO_until_Jul13, 2017-01-24 04:35:17]

> input_radio is not an important scenario, but it may still help to have a test
for focus loss scenario.
> How about replacing the line with JavaScript blur()?

What happens on JavaScript blur doesn't matter much from a user point of view
either.  I'd rather we go with tapping plain_text which is the most common
scenario we want to keep working.

[hugoh_UTC2, 2017-02-07 05:46:54]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 05:47:12]

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-02-07 06:43:32]

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

[commit-bot: I haz the power, 2017-02-07 06:43:33]

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

[hugoh_UTC2, 2017-02-09 07:52:31]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 07:52:45]

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-02-09 08:54:09]

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

[commit-bot: I haz the power, 2017-02-09 08:54:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-02-09 09:00:09]

Description was changed from

==========
Do not send redundant ViewHostMsg_TextInputStateChanged-message

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an input field (unnecessary!).
 (2) when focus enters another input field.

Solution:
When we know that focus enters a new input-field, we skip the
first call (1) to RenderFrameImpl::didChangeSelection.

BUG=678601
TEST=1. In content_shell, select some text in an <input>-field,
        click another <input>-field (move focus).
        Notice: selection cleared.
     2. run-webkit-tests LayoutTests/editing/selection
     3. content_browsertests --gtest_filter=RenderView*
==========

to

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the first call to selection.clear().

Automated tests are updated to test for one event, not two.
A newtest in WebFrameTest.cpp tests WebFrameClient::
didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-02-09 09:00:37]

Description was changed from

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the first call to selection.clear().

Automated tests are updated to test for one event, not two.
A newtest in WebFrameTest.cpp tests WebFrameClient::
didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the first call to selection.clear().

Automated tests are updated to test for one event, not two.
A new test in WebFrameTest.cpp tests WebFrameClient::
didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-02-09 09:44:26]

aelias@, I updated the Android test according to your suggestion.

yosin@, to fix the spatnav tests I "clear selection if new focus element can't
have selection" that you suggested earlier. Is this logic OK? See Element.cpp. 

amaralp@, FYI, your newly added EventHandlerTest.ClearHandleAfterTap test
fails... To me it feels like the test code fails to click outside the
<textarea>. When I try <textarea> in content_shell, a click outside of it does
reset the selection, at least visually.

[yosin_UTC9, 2017-02-09 13:01:48]

On 2017/02/09 at 09:44:26, hugoh wrote:
> yosin@, to fix the spatnav tests I "clear selection if new focus element can't
have selection" that you suggested earlier. Is this logic OK? See Element.cpp. 

It seems failure of "sav-{input,textarea}.html" is caused by
|executeMove{Down,Left,Right,Up}()| returns false and default event handle move
focus to farther.

Here is call stack:
Editor:handleKeyboardEvent:
handleEditingKeboardEvent:
executeMove{Down,Left,Right,Up}
FrameSelection::modify:
SelecitonModifier::modify

The suspicious code is:

SelecitonModifier::modify()
631:  if (isSpatialNavigationEnabled(frame())) {
632:    if (!wasRange && alter == FrameSelection::AlterationMove &&
633:        position.deepEquivalent() == originalStartPosition.deepEquivalent())
634:      return false;
635:  }

Before this patch, I guess |originalStartPosition| is null, but from this patch
|position.deepEquivalent() == originalStartPosition.deepEquivalent()|.

FrameSelection::modify()
665:  if (!modified) {
666:    if (userTriggered == NotUserTriggered)
667:      return false;
668:    // If spatial navigation enabled, focus navigator will move focus to
669:    // another element. See snav-input.html and snav-textarea.html
670:    if (isSpatialNavigationEnabled(m_frame))
671:      return false;

Arrow{Up,Down,Left,Right} is handle by  FrameSelection::modify() which also
spatial navigation mode, I think we should not check spatial mode here though.
via executeMove{Up,Down,Left,Right}() in "EditorCommand.cpp".

BTW, I think we handle spatial navigation specific behavior in
executeMove{Up,Down,Left,Right}() rather than FrameSelection::modify() and
SelectionModifier::modify().

Once we change executeMove{Up,Down,Left,Right}() to return true, == not moving
focus, by checking whether selection has focus or not.

[yosin_UTC9, 2017-02-09 13:02:17]

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2685: LocalFrame* frame =
document().frame();
We can early return here if |!frame|.
Let's hoist |if (!frame)| at L2687 here.

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2713: frame->selection().clear();
Focus and selection are individual identity. We don't want to clear focus here.
Think about caret browsing is an example although chrome doesn't support caret
browsing anymore.

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4243: void
didChangeSelection(bool isSelectionEmpty) { m_callCount++; }
nit: It is OK to use post-increment, but I prefer pre-increment as same as
iterator increment pattern.

[yosin_UTC9, 2017-02-09 13:16:57]

On 2017/02/09 at 13:02:17, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:2685: LocalFrame* frame =
document().frame();
> We can early return here if |!frame|.
> Let's hoist |if (!frame)| at L2687 here.
> 
>
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Element.cpp:2713:
frame->selection().clear();
> Focus and selection are individual identity. We don't want to clear focus
here.
> Think about caret browsing is an example although chrome doesn't support caret
> browsing anymore.
> 
>
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4243: void
didChangeSelection(bool isSelectionEmpty) { m_callCount++; }
> nit: It is OK to use post-increment, but I prefer pre-increment as same as
iterator increment pattern.

Original behavior of spv-input.html is
1 Caret is at beginning of start "|text"
2 Down to move "8" and clear selection
3 Up to start => select all "^text|"
4 Up to move caret at beginning of start
5 Up to move caret to "2"

Where "^" and "|" represent selection anchor and focus node/offset.

New behavior is
1 Caret is at beginning of start "|text"
2 Down to move "8"
3 Up to start and caret is beginning of start "|text"
4 Up to move caret to "2"

For me new behavior is seems to be natural, e.g. set caret at "te|xt"
So, I think it is OK to adjust test expectations of "sav-input.html" and
"sav-textarea.html".
Please ask UX team at Opera, I remember this is introduced by Opera.

[yosin_UTC9, 2017-02-09 15:14:27]

On 2017/02/09 at 13:16:57, yosin_UTC9 wrote:
> On 2017/02/09 at 13:02:17, yosin_UTC9 wrote:
> >
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > 
> >
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.cpp:2685: LocalFrame* frame =
document().frame();
> > We can early return here if |!frame|.
> > Let's hoist |if (!frame)| at L2687 here.
> > 
> >
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Element.cpp:2713:
frame->selection().clear();
> > Focus and selection are individual identity. We don't want to clear focus
here.
> > Think about caret browsing is an example although chrome doesn't support
caret
> > browsing anymore.
> > 
> >
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):
> > 
> >
https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4243: void
didChangeSelection(bool isSelectionEmpty) { m_callCount++; }
> > nit: It is OK to use post-increment, but I prefer pre-increment as same as
iterator increment pattern.
> 
> Original behavior of spv-input.html is
> 1 Caret is at beginning of start "|text"
> 2 Down to move "8" and clear selection
> 3 Up to start => select all "^text|"
> 4 Up to move caret at beginning of start
> 5 Up to move caret to "2"
> 
> Where "^" and "|" represent selection anchor and focus node/offset.
> 
> New behavior is
> 1 Caret is at beginning of start "|text"
> 2 Down to move "8"
> 3 Up to start and caret is beginning of start "|text"
> 4 Up to move caret to "2"
> 
> For me new behavior is seems to be natural, e.g. set caret at "te|xt"
> So, I think it is OK to adjust test expectations of "sav-input.html" and
"sav-textarea.html".
> Please ask UX team at Opera, I remember this is introduced by Opera.

BTW, what value do we get at HTMLInputElement::updateFocusAppearance() in
"sav-input.html"?
Is the value |SelectionBehaviorOnFocus::Restore|? The answer may be yes since
caret is restored the last value.

Default behavior for arrow key in spatial navigation mode is implemented in:

KeyboardEventManager::defaultArrowEventHandler()

345  WebFocusType type = focusDirectionForKey(event);
346  if (type != WebFocusTypeNone && isSpatialNavigationEnabled(m_frame) &&
347      !m_frame->document()->inDesignMode()) {
348    if (page->focusController().advanceFocus(type)) {
349      event->setDefaultHandled();
350      return;
351    }
352  }

FocusController::advanceFocus() calls Element::focus() with
|SelectionBehaviorOnFocus::Reset| via 
FocusController::advanceFocusDirectionallyInContainer().

This is called when executeMove{Down,Left,Right,Up}() returns false.

Another approach is make "Move{Down,Left,Right,Up}" not to work if selection
doesn't have focus.
It can be down change following entry of kEditorCommands[] in
"EditorCommand.cpp":

      {WebEditingCommandType::MoveDown, executeMoveDown,
       supportedFromMenuOrKeyBinding, enabledInEditableText, stateNone,
......................................^^^^^^^^^^^^^^^^^^^^^^ replace to new
function enabledInFocusedEditableText
       valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled},

[aelias_OOO_until_Jul13, 2017-02-09 23:43:55]

On 2017/02/09 at 09:44:26, hugoh wrote:
> amaralp@, FYI, your newly added EventHandlerTest.ClearHandleAfterTap test
fails... To me it feels like the test code fails to click outside the
<textarea>. When I try <textarea> in content_shell, a click outside of it does
reset the selection, at least visually.

That explanation doesn't make sense, because if so the test would be red even
with your patch not applied.  My guess is that a real-world browser causes more
events than just the tap event in this unit test, one of which clears selection.
 Please figure out what's going on and update the test to be more realistic if
needed.

[hugoh_UTC2, 2017-02-10 05:46:07]

On 2017/02/09 15:14:27, yosin_UTC9 wrote:
> So, I think it is OK to adjust test expectations of "sav-input.html" and
> "sav-textarea.html".
> Please ask UX team at Opera, I remember this is introduced by Opera.
I think Opera wants to keep current behavior. Our internal tests might fail
otherwise. Spatnav's advanceFocusDirectionallyInContainer() sends
SelectionBehaviorOnFocus::*Reset* and sav-input.html tests this behavior. Also
tab and shift+tab keys, handled by advanceFocusInDocumentOrder(), does
element->focus(FocusParams(SelectionBehaviorOnFocus::*Reset* ...)).

> BTW, what value do we get at HTMLInputElement::updateFocusAppearance() in
> "sav-input.html"?
> Is the value |SelectionBehaviorOnFocus::Restore|? The answer may be yes since
> caret is restored the last value.
When using left arrow key to spatnav to the <input>, we see tex|t and
HTMLInputElement::updateFocusAppearance() gets SelectionBehaviorOnFocus::*None*
because it is set by FocusController::setFocusedElement(Element*, Frame*).

However, advanceFocusDirectionallyInContainer() does not get called because the
"old" (=uncleared) selection gets to handle the arrow key:
1   blink::HTMLInputElement::updateFocusAppearance // Gets ...None
2   blink::Document::setFocusedElement
3   blink::FocusController::setFocusedElement // Focus changes too early.
4   blink::FrameSelection::setFocusedNodeIfNeeded
5   void blink::FrameSelection::setSelectionAlgorithm
6   blink::FrameSelection::setSelection
7   blink::FrameSelection::setSelection
8   blink::FrameSelection::modify // Oops! *
9   blink::executeMoveLeft
10  blink::Editor::Command::execute
11  blink::Editor::Command::execute
12  blink::Editor::handleEditingKeyboardEvent
13  blink::Editor::handleKeyboardEvent
14  blink::KeyboardEventManager::defaultKeyboardEventHandler
15  blink::EventHandler::defaultKeyboardEventHandler
16  blink::Node::defaultEventHandler
17  blink::HTMLElement::defaultEventHandler
18  blink::HTMLAnchorElement::defaultEventHandler
19  blink::EventDispatcher::dispatchEventPostProcess
20  blink::EventDispatcher::dispatch

* We wanna avoid this entry to FrameSelection and thus FocusController. During
spatnav, only advanceFocusDirectionallyInContainer() should do the job of moving
focus.

> Another approach is make "Move{Down,Left,Right,Up}" not to work if selection
> doesn't have focus.
I agree. This seems like a good way to ensure that
advanceFocusDirectionallyInContainer() runs.

[hugoh_UTC2, 2017-02-10 09:38:06]

On 2017/02/10 05:46:07, hugoh wrote:
> On 2017/02/09 15:14:27, yosin_UTC9 wrote:
> > So, I think it is OK to adjust test expectations of "sav-input.html" and
> > "sav-textarea.html".
> > Please ask UX team at Opera, I remember this is introduced by Opera.
> I think Opera wants to keep current behavior. Our internal tests might fail
> otherwise. Spatnav's advanceFocusDirectionallyInContainer() sends
> SelectionBehaviorOnFocus::*Reset* and sav-input.html tests this behavior. Also
> tab and shift+tab keys, handled by advanceFocusInDocumentOrder(), does
> element->focus(FocusParams(SelectionBehaviorOnFocus::*Reset* ...)).
> 
> > BTW, what value do we get at HTMLInputElement::updateFocusAppearance() in
> > "sav-input.html"?
> > Is the value |SelectionBehaviorOnFocus::Restore|? The answer may be yes
since
> > caret is restored the last value.
> When using left arrow key to spatnav to the <input>, we see tex|t and
> HTMLInputElement::updateFocusAppearance() gets
SelectionBehaviorOnFocus::*None*
> because it is set by FocusController::setFocusedElement(Element*, Frame*).
> 
> However, advanceFocusDirectionallyInContainer() does not get called because
the
> "old" (=uncleared) selection gets to handle the arrow key:
> 1   blink::HTMLInputElement::updateFocusAppearance // Gets ...None
> 2   blink::Document::setFocusedElement
> 3   blink::FocusController::setFocusedElement // Focus changes too early.
> 4   blink::FrameSelection::setFocusedNodeIfNeeded
> 5   void blink::FrameSelection::setSelectionAlgorithm
> 6   blink::FrameSelection::setSelection
> 7   blink::FrameSelection::setSelection
> 8   blink::FrameSelection::modify // Oops! *
> 9   blink::executeMoveLeft
> 10  blink::Editor::Command::execute
> 11  blink::Editor::Command::execute
> 12  blink::Editor::handleEditingKeyboardEvent
> 13  blink::Editor::handleKeyboardEvent
> 14  blink::KeyboardEventManager::defaultKeyboardEventHandler
> 15  blink::EventHandler::defaultKeyboardEventHandler
> 16  blink::Node::defaultEventHandler
> 17  blink::HTMLElement::defaultEventHandler
> 18  blink::HTMLAnchorElement::defaultEventHandler
> 19  blink::EventDispatcher::dispatchEventPostProcess
> 20  blink::EventDispatcher::dispatch
> 
> * We wanna avoid this entry to FrameSelection and thus FocusController. During
> spatnav, only advanceFocusDirectionallyInContainer() should do the job of
moving
> focus.
> 
> > Another approach is make "Move{Down,Left,Right,Up}" not to work if selection
> > doesn't have focus.
> I agree. This seems like a good way to ensure that
> advanceFocusDirectionallyInContainer() runs.

@yosin, please confirm this concept:

static bool enabledInFocusedEditableText(LocalFrame& frame,
                                  Event* event,
                                  EditorCommandSource dummy) {
  if (!enabledInEditableText(frame, event, dummy))
    return false;
  Node* nodeWithSelection = frame.selection().start().anchorNode();
  Node* nodeWithFocus = frame.document()->focusedElement();
  return nodeWithSelection->isSameNode(nodeWithFocus); // * 
}

* Not working. I need to figure out a safe way to compare Nodes (or Elements).
Maybe we need to add a FrameSelection::selectedElement() ?

[yosin_UTC9, 2017-02-10 11:49:45]

On 2017/02/10 at 09:38:06, hugoh wrote:
> On 2017/02/10 05:46:07, hugoh wrote:
> > On 2017/02/09 15:14:27, yosin_UTC9 wrote:
> > > So, I think it is OK to adjust test expectations of "sav-input.html" and
> > > "sav-textarea.html".
> > > Please ask UX team at Opera, I remember this is introduced by Opera.
> > I think Opera wants to keep current behavior. Our internal tests might fail
> > otherwise. Spatnav's advanceFocusDirectionallyInContainer() sends
> > SelectionBehaviorOnFocus::*Reset* and sav-input.html tests this behavior.
Also
> > tab and shift+tab keys, handled by advanceFocusInDocumentOrder(), does
> > element->focus(FocusParams(SelectionBehaviorOnFocus::*Reset* ...)).
> > 
> > > BTW, what value do we get at HTMLInputElement::updateFocusAppearance() in
> > > "sav-input.html"?
> > > Is the value |SelectionBehaviorOnFocus::Restore|? The answer may be yes
since
> > > caret is restored the last value.
> > When using left arrow key to spatnav to the <input>, we see tex|t and
> > HTMLInputElement::updateFocusAppearance() gets
SelectionBehaviorOnFocus::*None*
> > because it is set by FocusController::setFocusedElement(Element*, Frame*).
> > 
> > However, advanceFocusDirectionallyInContainer() does not get called because
the
> > "old" (=uncleared) selection gets to handle the arrow key:
> > 1   blink::HTMLInputElement::updateFocusAppearance // Gets ...None
> > 2   blink::Document::setFocusedElement
> > 3   blink::FocusController::setFocusedElement // Focus changes too early.
> > 4   blink::FrameSelection::setFocusedNodeIfNeeded
> > 5   void blink::FrameSelection::setSelectionAlgorithm
> > 6   blink::FrameSelection::setSelection
> > 7   blink::FrameSelection::setSelection
> > 8   blink::FrameSelection::modify // Oops! *
> > 9   blink::executeMoveLeft
> > 10  blink::Editor::Command::execute
> > 11  blink::Editor::Command::execute
> > 12  blink::Editor::handleEditingKeyboardEvent
> > 13  blink::Editor::handleKeyboardEvent
> > 14  blink::KeyboardEventManager::defaultKeyboardEventHandler
> > 15  blink::EventHandler::defaultKeyboardEventHandler
> > 16  blink::Node::defaultEventHandler
> > 17  blink::HTMLElement::defaultEventHandler
> > 18  blink::HTMLAnchorElement::defaultEventHandler
> > 19  blink::EventDispatcher::dispatchEventPostProcess
> > 20  blink::EventDispatcher::dispatch
> > 
> > * We wanna avoid this entry to FrameSelection and thus FocusController.
During
> > spatnav, only advanceFocusDirectionallyInContainer() should do the job of
moving
> > focus.
> > 
> > > Another approach is make "Move{Down,Left,Right,Up}" not to work if
selection
> > > doesn't have focus.
> > I agree. This seems like a good way to ensure that
> > advanceFocusDirectionallyInContainer() runs.
> 
> @yosin, please confirm this concept:
> 
> static bool enabledInFocusedEditableText(LocalFrame& frame,
>                                   Event* event,
>                                   EditorCommandSource dummy) {
>   if (!enabledInEditableText(frame, event, dummy))
>     return false;
>   Node* nodeWithSelection = frame.selection().start().anchorNode();
>   Node* nodeWithFocus = frame.document()->focusedElement();
>   return nodeWithSelection->isSameNode(nodeWithFocus); // * 
> }
> 
> * Not working. I need to figure out a safe way to compare Nodes (or Elements).
Maybe we need to add a FrameSelection::selectedElement() ?

Yes, this is what I expected.
Please try as below found in Editor::handleEditingKeyboardEvent()

  if (!focusedElement->containsIncludingHostElements(
          *m_frame->selection().start().computeContainerNode())) {
    // We should not insert text at selection start if selection doesn't have
    // focus. See http://crbug.com/89026
    return false;
  }

[hugoh_UTC2, 2017-02-11 03:42:22]

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2616623002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2713: frame->selection().clear();
On 2017/02/09 13:02:17, yosin_UTC9 wrote:
> Focus and selection are individual identity. We don't want to clear focus
here.
> Think about caret browsing is an example although chrome doesn't support caret
> browsing anymore.
True, here we would clear() too often. Let's put this logic in
advanceFocusInDocumentOrder() instead? When tabbing from <input> to <button> the
selected text of <input> should be cleared. Otherwise, we change the current
behavior of tab/shift+tab.

[hugoh_UTC2, 2017-02-13 07:18:19]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 07:18:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-02-13 07:35:54]

On 2017/02/09 23:43:55, aelias wrote:
> On 2017/02/09 at 09:44:26, hugoh wrote:
> > amaralp@, FYI, your newly added EventHandlerTest.ClearHandleAfterTap test
> fails... To me it feels like the test code fails to click outside the
> <textarea>. When I try <textarea> in content_shell, a click outside of it does
> reset the selection, at least visually.
> 
> That explanation doesn't make sense, because if so the test would be red even
> with your patch not applied.  My guess is that a real-world browser causes
more
> events than just the tap event in this unit test, one of which clears
selection.
>  Please figure out what's going on and update the test to be more realistic if
> needed.

True. When ClearHandleAfterTap() uses handleGestureEvent() to tap outside the 
textarea it expects clearSelectionIfNeeded() to mark
HandleVisibility::NotVisible.

#2  blink::FrameSelection::setSelectionAlgorithm<>()
#3  blink::FrameSelection::setSelection()
#4  blink::FrameSelection::setSelection()
#5  blink::FrameSelection::clear()
#6  blink::clearSelectionIfNeeded()
#7  blink::FocusController::setFocusedElement()
#8  blink::MouseEventManager::handleMouseFocus()
#9  blink::GestureManager::handleGestureTap()
#10 blink::GestureManager::handleGestureEventInFrame()
#11 blink::EventHandler::handleGestureEvent()
#12 blink::EventHandler::handleGestureEvent()
#13 blink::EventHandlerTest_ClearHandleAfterTap_Test::TestBody()

From now on, when we click on <body>, we don't wanna clear the selection - we
just wanna mark it NotVisible. I guess this must still be done by
FocusController::setFocusedElement(). I will look into it.

@yosin, I uploaded new Patch Set 9 where tabbing and spatial navigation tests
works again.

[yosin_UTC9, 2017-02-13 08:09:00]

https://codereview.chromium.org/2616623002/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2616623002/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1258:
waitForEventLogs("selectionchange");
(^_^)b

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
I'm OK this if Firefox and Edge do so.
kochi@, what do you think?

Do Firefox and Edge clear selection? Do they fire "selectionchange" event?

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1334: FrameSelection&
selection = focusedFrame()->selection();
I'm OK this if Firefox and Edge do so.
kochi@, what do you think?

Do Firefox and Edge clear selection? Do they fire "selectionchange" event?

[commit-bot: I haz the power, 2017-02-13 08:13:57]

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

[commit-bot: I haz the power, 2017-02-13 08:14:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-02-13 09:18:52]

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
On 2017/02/13 08:09:00, yosin_UTC9 wrote:
> I'm OK this if Firefox and Edge do so.
> kochi@, what do you think?
> 
> Do Firefox and Edge clear selection? 
Visually yes! That is why we cannot remove selection.clear() completely.

> Do they fire "selectionchange" event?
Firefox fires two identical (?) selectionchange events when focus jumps. But the
spec only mentions one event:

"When the selection is dissociated with its range, associated with a new range
or the associated range's boundary point is mutated either by the user or the
content script, the user agent must queue a task to fire an event with the name
selectionchange."

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1334: FrameSelection&
selection = focusedFrame()->selection();
On 2017/02/13 08:09:00, yosin_UTC9 wrote:
> I'm OK this if Firefox and Edge do so.
> kochi@, what do you think?
> 
> Do Firefox and Edge clear selection? Do they fire "selectionchange" event?

advanceFocusDirectionallyInContainer() only runs for
--enable-spatial-navigation. Neither Firefox nor Edge (?) implement spatial
navigation natively. But I believe that spatial navigation should behave as "tab
navigation" (both are user triggered focus jumps).

[hugoh_UTC2, 2017-02-13 09:55:52]

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
On 2017/02/13 09:18:52, hugoh wrote:
> On 2017/02/13 08:09:00, yosin_UTC9 wrote:
> > I'm OK this if Firefox and Edge do so.
> > kochi@, what do you think?
> > 
> > Do Firefox and Edge clear selection? 
> Visually yes! That is why we cannot remove selection.clear() completely.
Maybe a selection.hide() would be better here? Firefox only seem to hide the
selection, it doesn't send a selectionchange event when tabbing from <input> to
<button>.

[yosin_UTC9, 2017-02-13 10:18:55]

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
On 2017/02/13 at 09:18:52, hugoh wrote:
> On 2017/02/13 08:09:00, yosin_UTC9 wrote:
> > I'm OK this if Firefox and Edge do so.
> > kochi@, what do you think?
> > 
> > Do Firefox and Edge clear selection? 
> Visually yes! That is why we cannot remove selection.clear() completely.
If we don't paint selection if it doesn't have focus, it can also be visually
equal.

> > Do they fire "selectionchange" event?
> Firefox fires two identical (?) selectionchange events when focus jumps. But
the spec only mentions one event:
> 
> "When the selection is dissociated with its range, associated with a new range
or the associated range's boundary point is mutated either by the user or the
content script, the user agent must queue a task to fire an event with the name
selectionchange."

Firing two "selectionchange" events comes from |selection.clear()| and setting
selection to another INPUT element.
This means Blink,(and Firefox) still dispatch redundant "selectionchange" for
TAB navigation (and spatial navigation) case.

How about not painting selection if it doesn't have focus?

It can be done in PendingSelection::commit():

129  if (!selection.isRange()) {
130    layoutView.clearSelection();
131    return;
132  }

We can change L129 to

if (!selection.isRange() && !doseSelectionHaveFocus(selection))


doeSelectionHaveFocus() is similar to enabledInFocusedEditableText()

[hugoh_UTC2, 2017-02-14 10:18:44]

On 2017/02/13 10:18:55, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
> selection = focusedFrame()->selection();
> On 2017/02/13 at 09:18:52, hugoh wrote:
> > On 2017/02/13 08:09:00, yosin_UTC9 wrote:
> > > I'm OK this if Firefox and Edge do so.
> > > kochi@, what do you think?
> > > 
> > > Do Firefox and Edge clear selection? 
> > Visually yes! That is why we cannot remove selection.clear() completely.
> If we don't paint selection if it doesn't have focus, it can also be visually
> equal.
> 
> > > Do they fire "selectionchange" event?
> > Firefox fires two identical (?) selectionchange events when focus jumps. But
> the spec only mentions one event:
> > 
> > "When the selection is dissociated with its range, associated with a new
range
> or the associated range's boundary point is mutated either by the user or the
> content script, the user agent must queue a task to fire an event with the
name
> selectionchange."
> 
> Firing two "selectionchange" events comes from |selection.clear()| and setting
> selection to another INPUT element.
> This means Blink,(and Firefox) still dispatch redundant "selectionchange" for
> TAB navigation (and spatial navigation) case.
> 
> How about not painting selection if it doesn't have focus?
> 
> It can be done in PendingSelection::commit():
> 
> 129  if (!selection.isRange()) {
> 130    layoutView.clearSelection();
> 131    return;
> 132  }
> 
> We can change L129 to
> 
> if (!selection.isRange() && !doseSelectionHaveFocus(selection))
> 
> 
> doeSelectionHaveFocus() is similar to enabledInFocusedEditableText()

Good idea. I supposed you meant ||, not && ? That visually cleared the selection
during tab navigation (we really should add an automatic test case for tab
navigation). But it cleared it too often: it broke 36 LayoutTests/editing tests
as well as TextSelectionRepaintTest.RepaintSelectionOnFocus and
LocalFrameTest.dragImageForSelectionUsesPageScaleFactor. 

This is the code I tried (without selection.clear() in FocusController). We need
to find something better. Doesn't containsIncludingHostElements() return true
too often? If <body> is the focused element and a child element has the
selection, <body><p>[Selected]</p>, containsIncludingHostElements() will return
true. Maybe that is the problem.

+static bool selectionHasFocus(const LayoutView& layoutView,
+                              const VisibleSelectionInFlatTree& selection) {
+  const Node* containerNode = selection.start().computeContainerNode();
+  const LocalFrame* frame = layoutView.document().frame();
+  const Document* document = frame->document();
+  const Element* focusedElement = document->focusedElement();
+
+  return focusedElement && containerNode &&
+      focusedElement->containsIncludingHostElements(*containerNode);
+}

-  if (!selection.isRange()) {
+  if (!selectionHasFocus(layoutView, selection) || !selection.isRange()) {
     layoutView.clearSelection();
     return;
   }

I'm happy if this CL only fixes crbug.com/678601 and crbug.com/679635 . We could
put removal of the last two selection.clear() in FocusController as TODO. That
would reduce the impact and risk of this CL :)

[hugoh_UTC2, 2017-02-14 10:32:50]

Inline answer.

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
On 2017/02/13 10:18:55, yosin_UTC9 wrote:
> How about not painting selection if it doesn't have focus?
> 
> It can be done in PendingSelection::commit():
> 
> 129  if (!selection.isRange()) {
> 130    layoutView.clearSelection();
> 131    return;
> 132  }
> 
> We can change L129 to
> 
> if (!selection.isRange() && !doseSelectionHaveFocus(selection))
> 
> 
> doeSelectionHaveFocus() is similar to enabledInFocusedEditableText()

Good idea. I supposed you meant ||, not && ? That visually cleared the selection
during tab navigation (we really should add an automatic test case for tab
navigation). But it cleared it too often: it broke 36 LayoutTests/editing tests
as well as TextSelectionRepaintTest.RepaintSelectionOnFocus and
LocalFrameTest.dragImageForSelectionUsesPageScaleFactor. 

This is the code I tried (without selection.clear() in FocusController). We need
to find something better. Doesn't containsIncludingHostElements() return true
too often? If <body> is the focused element and a child element has the
selection, <body><p>[Selected]</p>, containsIncludingHostElements() will return
true. Maybe that is the problem.

+static bool selectionHasFocus(const LayoutView& layoutView,
+                              const VisibleSelectionInFlatTree& selection) {
+  const Node* containerNode = selection.start().computeContainerNode();
+  const LocalFrame* frame = layoutView.document().frame();
+  const Document* document = frame->document();
+  const Element* focusedElement = document->focusedElement();
+
+  return focusedElement && containerNode &&
+      focusedElement->containsIncludingHostElements(*containerNode);
+}

-  if (!selection.isRange()) {
+  if (!selectionHasFocus(layoutView, selection) || !selection.isRange()) {
     layoutView.clearSelection();
     return;
   }

I'm happy if this CL only fixes crbug.com/678601 and crbug.com/679635 . We could
put removal of the last two selection.clear() in FocusController as TODO. That
would reduce the impact and risk of this CL :)

[yosin_UTC9, 2017-02-14 12:27:10]

On 2017/02/14 at 10:32:50, hugoh wrote:
> Inline answer.
> 
>
https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/2616623002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/page/FocusController.cpp:1048: FrameSelection&
selection = focusedFrame()->selection();
> On 2017/02/13 10:18:55, yosin_UTC9 wrote:
> > How about not painting selection if it doesn't have focus?
> > 
> > It can be done in PendingSelection::commit():
> > 
> > 129  if (!selection.isRange()) {
> > 130    layoutView.clearSelection();
> > 131    return;
> > 132  }
> > 
> > We can change L129 to
> > 
> > if (!selection.isRange() && !doseSelectionHaveFocus(selection))
> > 
> > 
> > doeSelectionHaveFocus() is similar to enabledInFocusedEditableText()
> 
> Good idea. I supposed you meant ||, not && ? That visually cleared the
selection
Yes, you're right. I mean &&.

> This is the code I tried (without selection.clear() in FocusController). We
need
> to find something better. Doesn't containsIncludingHostElements() return true
> too often? If <body> is the focused element and a child element has the
> selection, <body><p>[Selected]</p>, containsIncludingHostElements() will
return
> true. Maybe that is the problem.

I guess we should consider editing boundary and shodow host.

> I'm happy if this CL only fixes crbug.com/678601 and crbug.com/679635 . We
could
> put removal of the last two selection.clear() in FocusController as TODO. That
> would reduce the impact and risk of this CL :)
Agree. Un-painting unfocus selection should be handled in another patch.
Please file the bug about extra "selectionchange" event firing on TAB key and
spatial navigation and put bug number in TODO.

Thanks!

[hugoh_UTC2, 2017-02-14 13:56:03]

On 2017/02/14 12:27:10, yosin_UTC9 wrote:
> > This is the code I tried (without selection.clear() in FocusController). We
> need
> > to find something better. Doesn't containsIncludingHostElements() return
true
> > too often? If <body> is the focused element and a child element has the
> > selection, <body><p>[Selected]</p>, containsIncludingHostElements() will
> return
> > true. Maybe that is the problem.
> 
> I guess we should consider editing boundary and shodow host.

Instead of containsIncludingHostElements(), maybe it would work to use something
like associatedElementOf() in EditingUtilities.cpp?

static bool selectionHasFocus(const LayoutView& layoutView,
                              const VisibleSelection& selection) {
  const LocalFrame* frame = layoutView.document().frame();
  const Document* document = frame->document();
  const Element* focusedElement = document->focusedElement();
  if (!focusedElement)
    return false;
  const Element* selectedElement = associatedElementOf(selection.start());
  return selectedElement == focusedElement;
}

[yosin_UTC9, 2017-02-14 14:19:22]

On 2017/02/14 at 13:56:03, hugoh wrote:
> On 2017/02/14 12:27:10, yosin_UTC9 wrote:
> > > This is the code I tried (without selection.clear() in FocusController).
We
> > need
> > > to find something better. Doesn't containsIncludingHostElements() return
true
> > > too often? If <body> is the focused element and a child element has the
> > > selection, <body><p>[Selected]</p>, containsIncludingHostElements() will
> > return
> > > true. Maybe that is the problem.
> > 
> > I guess we should consider editing boundary and shodow host.
> 
> Instead of containsIncludingHostElements(), maybe it would work to use
something like associatedElementOf() in EditingUtilities.cpp?
> 
> static bool selectionHasFocus(const LayoutView& layoutView,
>                               const VisibleSelection& selection) {
>   const LocalFrame* frame = layoutView.document().frame();
>   const Document* document = frame->document();
>   const Element* focusedElement = document->focusedElement();
>   if (!focusedElement)
>     return false;
>   const Element* selectedElement = associatedElementOf(selection.start());
>   return selectedElement == focusedElement;
> }

I don't think so. associatedElementOf() essentially returns Element of
position.anchorNode()->parent()
(It is actually wrong, associatedElementOf() should handle BeforeAnchor,
AfterAnchor)

Example: where "|" represents selection#focusNode/focusOffset, "^" represents
selection#anchorNode/anchorOffset
<div id="outer">
<input>
 #shadow-root
   <div id="inner-editor" contenteditable>f^oob|ar</div>
Assume "foo" and "bar" are different Text nodes

- When <div id="outer" has focus: not paint selection
- When <input> has focus: paint selection
- When <div id="inner-editor"> has focus: paint selection
- When text node "foo" has focus: paint selection
- When text node "bar" has focus: paint selection

So, we need to check both selection.base() and selection.extent() positions.

Example#2:
<body>
 <p>foo^bar|baz</p>

When <body> has focus: paint

Example#3:
<body>
 <p contenteditable>foo^bar|baz</p>

When <body> has focus: not paint; due by editing boundary
Note: selection.base() and selection.extend() can't cross editing boundary
(until we support user-selection:contain).

Example#4:
<div contenteditable>
 <span contenteditable=false>fo^ob|ar</span>
</div>

When <div cE> has focus: paint; selection's editable root <div cE> has focus

How test failed with selectionHasFocus()? Are these test failure pass if we also
check selection.end()?

[hugoh_UTC2, 2017-02-15 01:19:24]

> Agree. Un-painting unfocused selection should be handled in another patch.
Unfortunately, I don't think mouse-click-plugin-clears-selection.html will pass
if we don't "unpaint" the selection. But getting the selection unpainted
properly seems hard. Week 8 and 9 I will be out of office so I hoped to close
this CL within this week :) Would it be OK if this CL only fixes
crbug.com/678601 (Patch Set 2)? Making selection independent of focus is not
needed to fix 678601.

> How test failed with selectionHasFocus()? Are these test failure pass if we
also check selection.end()?
Yes, tests still fail because the selection is gone (I do
layoutView.clearSelection() too often).

static bool selectionHasFocus(const LayoutView& layoutView,
                              const VisibleSelectionInFlatTree& selection) {
  const Node* containerStart = selection.start().computeContainerNode();
  const Node* containerEnd = selection.end().computeContainerNode();
  const LocalFrame* frame = layoutView.document().frame();
  const Document* document = frame->document();
  const Element* focusedElement = document->focusedElement();

  return focusedElement && containerStart && containerEnd &&
         (focusedElement->containsIncludingHostElements(*containerStart) ||
         
focusedElement->containsIncludingHostElements(*containerEnd));/*added*/
}

  if (!selectionHasFocus(layoutView, selection) || !selection.isRange()) {
    // Do not display selection when selection doesn't have focus.
    layoutView.clearSelection();
    return;
  }

Regressions: Unexpected image-only failures (36)
  editing/execCommand/findString-2.html [ Failure ]
  editing/execCommand/findString.html [ Failure ]
  editing/pasteboard/drag-image-to-contenteditable-in-iframe.html [ Failure ]
  editing/selection/14971.html [ Failure ]
  editing/selection/4402375.html [ Failure ]
  editing/selection/5232159.html [ Failure ]
  editing/selection/5240265.html [ Failure ]
  editing/selection/addRange.html [ Failure ]
  editing/selection/display-table-text.html [ Failure ]
 
editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-ltr-mixed.html
[ Failure ]
 
editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-rtl-mixed.html
[ Failure ]
  editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-rtl.html [
Failure ]
  editing/selection/dont-select-text-overflow-ellipsis-when-wrapping.html [
Failure ]
  editing/selection/drag-to-contenteditable-iframe.html [ Failure ]
  editing/selection/extend-inside-transforms-backward.html [ Failure ]
  editing/selection/extend-inside-transforms-forward.html [ Failure ]
  editing/selection/inline-closest-leaf-child.html [ Failure ]
  editing/selection/line-wrap-1.html [ Failure ]
  editing/selection/line-wrap-2.html [ Failure ]
  editing/selection/linux_selection_color.html [ Failure ]
  editing/selection/mixed-editability-2.html [ Failure ]
  editing/selection/node-removal-2.html [ Failure ]
  editing/selection/paragraph-granularity.html [ Failure ]
  editing/selection/range-between-block-and-inline.html [ Failure ]
  editing/selection/select-across-readonly-input-2.html [ Failure ]
  editing/selection/select-across-readonly-input-3.html [ Failure ]
  editing/selection/select-text-overflow-ellipsis-mixed-in-ltr-2.html [ Failure
]
  editing/selection/select-text-overflow-ellipsis-mixed-in-ltr.html [ Failure ]
  editing/selection/select-text-overflow-ellipsis-mixed-in-rtl-2.html [ Failure
]
  editing/selection/select-text-overflow-ellipsis-mixed-in-rtl.html [ Failure ]
  editing/selection/select-text-overflow-ellipsis.html [ Failure ]
  editing/selection/selection-actions.html [ Failure ]
  editing/selection/selection-background.html [ Failure ]
  editing/selection/selection-button-text.html [ Failure ]
  editing/selection/transformed-selection-rects.html [ Failure ]
  editing/selection/word-granularity.html [ Failure ]

[yosin_UTC9, 2017-02-15 01:41:22]

On 2017/02/15 at 01:19:24, hugoh wrote:
> > Agree. Un-painting unfocused selection should be handled in another patch.
> Unfortunately, I don't think mouse-click-plugin-clears-selection.html will
pass if we don't "unpaint" the selection. But getting the selection unpainted
properly seems hard. Week 8 and 9 I will be out of office so I hoped to close
this CL within this week :) Would it be OK if this CL only fixes
crbug.com/678601 (Patch Set 2)? Making selection independent of focus is not
needed to fix 678601.
> 
> > How test failed with selectionHasFocus()? Are these test failure pass if we
also check selection.end()?
> Yes, tests still fail because the selection is gone (I do
layoutView.clearSelection() too often).
> 
> static bool selectionHasFocus(const LayoutView& layoutView,
>                               const VisibleSelectionInFlatTree& selection) {
>   const Node* containerStart = selection.start().computeContainerNode();
>   const Node* containerEnd = selection.end().computeContainerNode();
>   const LocalFrame* frame = layoutView.document().frame();
>   const Document* document = frame->document();
>   const Element* focusedElement = document->focusedElement();
> 
>   return focusedElement && containerStart && containerEnd &&
>          (focusedElement->containsIncludingHostElements(*containerStart) ||
>          
focusedElement->containsIncludingHostElements(*containerEnd));/*added*/
> }
> 
>   if (!selectionHasFocus(layoutView, selection) || !selection.isRange()) {
>     // Do not display selection when selection doesn't have focus.
>     layoutView.clearSelection();
>     return;
>   }
> 
> Regressions: Unexpected image-only failures (36)
>   editing/execCommand/findString-2.html [ Failure ]
>   editing/execCommand/findString.html [ Failure ]
>   editing/pasteboard/drag-image-to-contenteditable-in-iframe.html [ Failure ]
>   editing/selection/14971.html [ Failure ]
>   editing/selection/4402375.html [ Failure ]
>   editing/selection/5232159.html [ Failure ]
>   editing/selection/5240265.html [ Failure ]
>   editing/selection/addRange.html [ Failure ]
>   editing/selection/display-table-text.html [ Failure ]
>  
editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-ltr-mixed.html
[ Failure ]
>  
editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-rtl-mixed.html
[ Failure ]
>   editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-rtl.html
[ Failure ]
>   editing/selection/dont-select-text-overflow-ellipsis-when-wrapping.html [
Failure ]
>   editing/selection/drag-to-contenteditable-iframe.html [ Failure ]
>   editing/selection/extend-inside-transforms-backward.html [ Failure ]
>   editing/selection/extend-inside-transforms-forward.html [ Failure ]
>   editing/selection/inline-closest-leaf-child.html [ Failure ]
>   editing/selection/line-wrap-1.html [ Failure ]
>   editing/selection/line-wrap-2.html [ Failure ]
>   editing/selection/linux_selection_color.html [ Failure ]
>   editing/selection/mixed-editability-2.html [ Failure ]
>   editing/selection/node-removal-2.html [ Failure ]
>   editing/selection/paragraph-granularity.html [ Failure ]
>   editing/selection/range-between-block-and-inline.html [ Failure ]
>   editing/selection/select-across-readonly-input-2.html [ Failure ]
>   editing/selection/select-across-readonly-input-3.html [ Failure ]
>   editing/selection/select-text-overflow-ellipsis-mixed-in-ltr-2.html [
Failure ]
>   editing/selection/select-text-overflow-ellipsis-mixed-in-ltr.html [ Failure
]
>   editing/selection/select-text-overflow-ellipsis-mixed-in-rtl-2.html [
Failure ]
>   editing/selection/select-text-overflow-ellipsis-mixed-in-rtl.html [ Failure
]
>   editing/selection/select-text-overflow-ellipsis.html [ Failure ]
>   editing/selection/selection-actions.html [ Failure ]
>   editing/selection/selection-background.html [ Failure ]
>   editing/selection/selection-button-text.html [ Failure ]
>   editing/selection/transformed-selection-rects.html [ Failure ]
>   editing/selection/word-granularity.html [ Failure ]

Thanks for investigation. PendingSelection::commit() doesn't check focus if
selection has not been
change since the last paint. But, we need to mark checking whether selection has
focus or not
after focus change == Make FrameSelection observe focus change.

Anyway, let's postpone to fix TAB/SNAV issue, unpaint no focus slection, this in
another patch.

[hugoh_UTC2, 2017-02-15 02:01:40]

On 2017/02/15 01:41:22, yosin_UTC9 wrote:
> Anyway, let's postpone to fix TAB/SNAV issue, unpaint no focus slection, this
in
> another patch.
You mean it is OK that this CL only fixes crbug.com/678601 (=Patch Set 2)?

[yosin_UTC9, 2017-02-15 02:30:37]

On 2017/02/15 at 02:01:40, hugoh wrote:
> On 2017/02/15 01:41:22, yosin_UTC9 wrote:
> > Anyway, let's postpone to fix TAB/SNAV issue, unpaint no focus slection,
this in
> > another patch.
> You mean it is OK that this CL only fixes crbug.com/678601 (=Patch Set 2)?

Yes. Because of this patch mitigate the situation == don't fire an extra
"selectionchange"
except for TAB and Spatial navigation cases.

And fix for TAB and SNAV requires changes in FrameSelection and it is isolated
from
this patch.

Other reviewers, what do you think?

[hugoh_UTC2, 2017-02-15 03:00:42]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:02:53]

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-02-15 03:53:38]

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

[commit-bot: I haz the power, 2017-02-15 03:53:39]

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

[hugoh_UTC2, 2017-02-15 05:22:19]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 05:23:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-02-15 05:25:09]

Description was changed from

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the first call to selection.clear().

Automated tests are updated to test for one event, not two.
A new test in WebFrameTest.cpp tests WebFrameClient::
didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the call to selection.clear().

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests WebFrameClient::didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-02-15 06:36:33]

@yosin, I updated the unit test. PTAL. Now it will not break when later CLs
fixes http://crbug.com/679635 .

[yosin_UTC9, 2017-02-15 07:16:24]

On 2017/02/15 at 06:36:33, hugoh wrote:
> @yosin, I updated the unit test. PTAL. Now it will not break when later CLs
fixes http://crbug.com/679635 .

I prefer PS#9 way instead of updating |clearSelectionIfNeeded()|. So far, we are
agreed this way and clear selection for TAB and SNAV.

BTW, could you tell me your location or time zone? I would like to know timezone
difference to optimize response time.

[hugoh_UTC2, 2017-02-15 07:52:46]

On 2017/02/15 07:16:24, yosin_UTC9 wrote:
> On 2017/02/15 at 06:36:33, hugoh wrote:
> > @yosin, I updated the unit test. PTAL. Now it will not break when later CLs
> fixes http://crbug.com/679635 .
> 
> I prefer PS#9 way instead of updating |clearSelectionIfNeeded()|. So far, we
are
> agreed this way and clear selection for TAB and SNAV.

The problem with Patch Set 9 is still EventHandlerTest.ClearHandleAfterTap and
mouse-click-plugin-clears-selection.html.
mouse-click-plugin-clears-selection.html requires an unpainted (=hidden)
selection, so more logical changes (FrameSelection observing focus...)? I
understand we don't want to update |clearSelectionIfNeeded()| but I prefer to
land a smaller CL in case of regressions.

> BTW, could you tell me your location or time zone? I would like to know
timezone
> difference to optimize response time.

I'm in Tokyo at the moment, sorry for working at confusing hours :) I will be
here for a few weeks, so we could try to meet at some point. (Week 8 and 9 I'm
on holiday).

[yosin_UTC9, 2017-02-15 08:23:36]

On 2017/02/15 at 07:52:46, hugoh wrote:
> On 2017/02/15 07:16:24, yosin_UTC9 wrote:
> > On 2017/02/15 at 06:36:33, hugoh wrote:
> > > @yosin, I updated the unit test. PTAL. Now it will not break when later
CLs
> > fixes http://crbug.com/679635 .
> > 
> > I prefer PS#9 way instead of updating |clearSelectionIfNeeded()|. So far, we
are
> > agreed this way and clear selection for TAB and SNAV.
> 
> The problem with Patch Set 9 is still EventHandlerTest.ClearHandleAfterTap and
mouse-click-plugin-clears-selection.html.
mouse-click-plugin-clears-selection.html requires an unpainted (=hidden)
selection, so more logical changes (FrameSelection observing focus...)? I
understand we don't want to update |clearSelectionIfNeeded()| but I prefer to
land a smaller CL in case of regressions.

Since this patch set doesn't have editing/, so I delegate decision to kochi@, he
owns FocusController.


> > BTW, could you tell me your location or time zone? I would like to know
timezone
> > difference to optimize response time.
> 
> I'm in Tokyo at the moment, sorry for working at confusing hours :) I will be
here for a few weeks, so we could try to meet at some point. (Week 8 and 9 I'm
on holiday).
Good to hear. I send invitation email.
Since you're in Tokyo, you make me free from working in home. I wrote review
comments in home around mid night. ;-)

[yosin_UTC9, 2017-02-15 08:23:52]

yosin@chromium.org changed reviewers:
- yosin@chromium.org

[hugoh_UTC2, 2017-02-15 08:24:30]

On 2017/02/15 07:52:46, hugoh wrote:
> On 2017/02/15 07:16:24, yosin_UTC9 wrote:
> > On 2017/02/15 at 06:36:33, hugoh wrote:
> > > @yosin, I updated the unit test. PTAL. Now it will not break when later
CLs
> > fixes http://crbug.com/679635 .
> > 
> > I prefer PS#9 way instead of updating |clearSelectionIfNeeded()|. So far, we
> are
> > agreed this way and clear selection for TAB and SNAV.
> 
> The problem with Patch Set 9 is still EventHandlerTest.ClearHandleAfterTap and
> mouse-click-plugin-clears-selection.html.
> mouse-click-plugin-clears-selection.html requires an unpainted (=hidden)
> selection, so more logical changes (FrameSelection observing focus...)? I
> understand we don't want to update |clearSelectionIfNeeded()| but I prefer to
> land a smaller CL in case of regressions.
> 
> > BTW, could you tell me your location or time zone? I would like to know
> timezone
> > difference to optimize response time.
> 
> I'm in Tokyo at the moment, sorry for working at confusing hours :) I will be
> here for a few weeks, so we could try to meet at some point. (Week 8 and 9 I'm
> on holiday).

I just found another problem with Patch Set 9. It sends two selectionchange (one
is redundant) when tabkey makes focus jump from <input> to <p
contenteditable="true">blabla</p>. I saw this because it doesn't pass the new
WebFrame test of Patch Set 12 :(

[yosin_UTC9, 2017-02-15 12:02:04]

On 2017/02/15 at 08:24:30, hugoh wrote:
> On 2017/02/15 07:52:46, hugoh wrote:
> > On 2017/02/15 07:16:24, yosin_UTC9 wrote:
> > > On 2017/02/15 at 06:36:33, hugoh wrote:
> > > > @yosin, I updated the unit test. PTAL. Now it will not break when later
CLs
> > > fixes http://crbug.com/679635 .
> > > 
> > > I prefer PS#9 way instead of updating |clearSelectionIfNeeded()|. So far,
we
> > are
> > > agreed this way and clear selection for TAB and SNAV.
> > 
> > The problem with Patch Set 9 is still EventHandlerTest.ClearHandleAfterTap
and
> > mouse-click-plugin-clears-selection.html.
> > mouse-click-plugin-clears-selection.html requires an unpainted (=hidden)
> > selection, so more logical changes (FrameSelection observing focus...)? I
> > understand we don't want to update |clearSelectionIfNeeded()| but I prefer
to
> > land a smaller CL in case of regressions.
> > 
> > > BTW, could you tell me your location or time zone? I would like to know
> > timezone
> > > difference to optimize response time.
> > 
> > I'm in Tokyo at the moment, sorry for working at confusing hours :) I will
be
> > here for a few weeks, so we could try to meet at some point. (Week 8 and 9
I'm
> > on holiday).
> 
> I just found another problem with Patch Set 9. It sends two selectionchange
(one is redundant) when tabkey makes focus jump from <input> to <p
contenteditable="true">blabla</p>. I saw this because it doesn't pass the new
WebFrame test of Patch Set 12 :(

Since we clear selection for TAB navigation.

[hugoh_UTC2, 2017-02-16 10:33:00]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-02-16 10:33:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2616623002/#ps240001
(title: "To pass tests, keep selection.clear when clicking <body>")

[hugoh_UTC2, 2017-02-16 10:33:02]

The CQ bit was unchecked by hugoh@opera.com

[hugoh_UTC2, 2017-02-16 10:33:16]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 10:33:46]

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-02-16 10:37:04]

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

[commit-bot: I haz the power, 2017-02-16 10:37:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hugoh_UTC2, 2017-02-17 03:09:21]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 03:10:15]

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-02-17 05:33:02]

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

[commit-bot: I haz the power, 2017-02-17 05:33:04]

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

[hugoh_UTC2, 2017-02-17 10:29:54]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 10:30:06]

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-02-17 11:16:53]

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

[commit-bot: I haz the power, 2017-02-17 11:16:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-06 00:10:28]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 00:10:44]

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-03-06 00:12:40]

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

[commit-bot: I haz the power, 2017-03-06 00:12:41]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hugoh_UTC2, 2017-03-06 03:52:56]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 03:53:08]

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-03-06 05:19:31]

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

[commit-bot: I haz the power, 2017-03-06 05:19:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-06 07:14:26]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 07:14:39]

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-03-06 08:33:22]

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

[commit-bot: I haz the power, 2017-03-06 08:33:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-06 09:09:48]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 09:09:59]

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-03-06 13:00:54]

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

[commit-bot: I haz the power, 2017-03-06 13:00:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hugoh_UTC2, 2017-03-07 01:32:01]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:33:37]

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-03-07 03:34:05]

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

[commit-bot: I haz the power, 2017-03-07 03:34:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hugoh_UTC2, 2017-03-07 06:02:56]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 06:03:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-07 06:57:51]

The CQ bit was unchecked by hugoh@opera.com

[hugoh_UTC2, 2017-03-07 07:00:58]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 07:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-07 08:41:40]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 08:42:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-07 09:13:36]

Description was changed from

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Also the web page gets two selectionchange events.

Solution:
This happened because FocusController::setFocusedElement() cleared
the current selection even when the new "focus element" was about
to set the new selection. Now, when we see that focus goes to an
editable element (an element that will reset the selection), we
skip the call to selection.clear().

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests WebFrameClient::didChangeSelection.

BUG=678601
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events
(the first one is immediately invalid so no need to react on it).

Solution:
This happened because FocusController::setFocusedElement() cleared
the selection too often. Now, when JavaScript moves focus we don't
clear the selection. Also, when the user moves focus between
<input>-fields using tab-key navigation or mouse, selection moves
once: we send one selectionchange event, not two, for each jump.

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-03-07 09:55:16]

Description was changed from

==========
Do not send redundant selectionchange-event

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events
(the first one is immediately invalid so no need to react on it).

Solution:
This happened because FocusController::setFocusedElement() cleared
the selection too often. Now, when JavaScript moves focus we don't
clear the selection. Also, when the user moves focus between
<input>-fields using tab-key navigation or mouse, selection moves
once: we send one selectionchange event, not two, for each jump.

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events
(the first one is immediately invalid so no need to react on it).

Solution:
This happened because FocusController::setFocusedElement() cleared
the selection too often. Now, when JavaScript moves focus we don't
clear the selection. Also, when the user moves focus between
<input>-fields using tab-key navigation or mouse, selection moves
once: we send one selectionchange event, not two, for each jump.

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests tab-key navigation.

Follow-ups will remove the remaining redundant clears:
Part 2: crbug.com/699015 (mouse clicks on <body>).
Part 3: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[commit-bot: I haz the power, 2017-03-07 10:32:19]

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

[commit-bot: I haz the power, 2017-03-07 10:32:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hugoh_UTC2, 2017-03-09 07:45:31]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 07:45:51]

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-03-09 09:06:36]

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

[commit-bot: I haz the power, 2017-03-09 09:06:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-09 11:49:09]

Description was changed from

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events
(the first one is immediately invalid so no need to react on it).

Solution:
This happened because FocusController::setFocusedElement() cleared
the selection too often. Now, when JavaScript moves focus we don't
clear the selection. Also, when the user moves focus between
<input>-fields using tab-key navigation or mouse, selection moves
once: we send one selectionchange event, not two, for each jump.

Automated tests are updated to test for one event, not two. A new
test in WebFrameTest.cpp tests tab-key navigation.

Follow-ups will remove the remaining redundant clears:
Part 2: crbug.com/699015 (mouse clicks on <body>).
Part 3: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid no so point reacting on it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to any element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the selection (we just hide it). This
means, selection moves once: we send one selectionchange event,
not two, for each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. Since the selection is no longer cleared (it is hidden)
   update layout tests with new "hidden" selection's caret
   position.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-03-09 11:58:16]

Description was changed from

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid no so point reacting on it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to any element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the selection (we just hide it). This
means, selection moves once: we send one selectionchange event,
not two, for each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. Since the selection is no longer cleared (it is hidden)
   update layout tests with new "hidden" selection's caret
   position.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid no so point reacting on it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to any element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. Layout trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-03-10 01:36:35]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 01:37:10]

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-03-10 02:54:22]

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

[commit-bot: I haz the power, 2017-03-10 02:54:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-10 03:59:50]

Description was changed from

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is called
twice so two ViewHostMsg_TextInputStateChanged-messages are sent
to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid no so point reacting on it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to any element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. Layout trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-03-10 04:37:14]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 04:37:23]

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-03-10 05:52:25]

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

[commit-bot: I haz the power, 2017-03-10 05:52:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-10 10:10:37]

Conclusions from offline talk with @yosin:
* We need hiding ("unpainting") now. Otherwise element.focus() from JavaScript
leaves an unfocused selection visible in blue. 
* FrameSelection already observes focus change via its
FrameSelection::didChangeFocus().

@amaralp, I see that ClearHandleAfterTap fails because, for tap,
SelectionController::handleMousePressEventSingleClick() returns early: it does
not call into updateSelectionForMouseDownDispatchingSelectStart().

#0  SelectionController::handleMousePressEventSingleClick # returns because
!innerNode
#1  MouseEventManager::handleMousePressEvent 
#2  GestureManager::handleGestureTap 
#3  GestureManager::handleGestureEventInFrame 
#4  EventHandler::handleGestureEvent 
#5  EventHandler::handleGestureEvent
#6  EventHandlerTest_ClearHandleAfterTap_Test::TestBody

That is why the complete selection is unaffected by the tap.

I think it makes sense to not send a selectstart-event on tap. A mouse click can
be the start of a new selection (thus we send selectionstart), but a quick tap
cannot start a new selection (so we do not send selectionstart). Right? Does
this mean it is OK to remove ClearHandleAfterTap()? :)

https://codereview.chromium.org/2616623002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/PendingSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/PendingSelection.cpp:118: focus =
layoutView.document().documentElement();
I use document().documentElement(), not document.body() so non HTML-documents
(here SVG documents) also can have selected text.

[aelias_OOO_until_Jul13, 2017-03-10 20:07:57]

On 2017/03/10 at 10:10:37, hugoh wrote:
> Conclusions from offline talk with @yosin:
> * We need hiding ("unpainting") now. Otherwise element.focus() from JavaScript
leaves an unfocused selection visible in blue. 
> [...]
> I think it makes sense to not send a selectstart-event on tap. A mouse click
can be the start of a new selection (thus we send selectionstart), but a quick
tap cannot start a new selection (so we do not send selectionstart). Right? Does
this mean it is OK to remove ClearHandleAfterTap()? :)

Well, the ClearHandleAfterTap test taps outside the textarea, causing an
unfocus.  Regardless of those fine details, that unfocus should cause the handle
to cleared.  So it doesn't seem like the test should be removed.  And it sounds
like this test failure is related to the "unpainting" issue you mentioned above.
 It sounds like the "unfocused selection visible in blue" probably also has a
handle associated with it.  So hopefully after you fix the focus logic, the test
will go green naturally?

[hugoh_UTC2, 2017-03-12 09:41:55]

On 2017/03/10 20:07:57, aelias wrote:
> On 2017/03/10 at 10:10:37, hugoh wrote:
> > Conclusions from offline talk with @yosin:
> > * We need hiding ("unpainting") now. Otherwise element.focus() from
JavaScript
> leaves an unfocused selection visible in blue. 
> > [...]
> > I think it makes sense to not send a selectstart-event on tap. A mouse click
> can be the start of a new selection (thus we send selectionstart), but a quick
> tap cannot start a new selection (so we do not send selectionstart). Right?
Does
> this mean it is OK to remove ClearHandleAfterTap()? :)
> 
> Well, the ClearHandleAfterTap test taps outside the textarea, causing an
> unfocus.  Regardless of those fine details, that unfocus should cause the
handle
> to cleared.  So it doesn't seem like the test should be removed.  And it
sounds
> like this test failure is related to the "unpainting" issue you mentioned
above.
>  It sounds like the "unfocused selection visible in blue" probably also has a
> handle associated with it.  So hopefully after you fix the focus logic, the
test
> will go green naturally?

I've done the "unpainting" in PendingSelection.cpp already. PendingSelection is
owned by FrameSelection and communication goes from FS to PS. Now FS is unaware
of the paint-state. I like that. We could let the "is handle visible"-check also
include "and selection is focused". But do the users* of
FrameSelection::isHandleVisible() really need to know that? To the outside
world, the handle (= blue selection) is visible or hidden based on focused
element. So for end users, it looks fine.

* InputMethodController, SelectionController, FrameView + tests

[hugoh_UTC2, 2017-03-13 05:03:17]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 05:03:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-13 05:08:55]

On 2017/03/12 09:41:55, hugoh wrote:
> On 2017/03/10 20:07:57, aelias wrote:
> > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > Conclusions from offline talk with @yosin:
> > > * We need hiding ("unpainting") now. Otherwise element.focus() from
> JavaScript
> > leaves an unfocused selection visible in blue. 
> > > [...]
> > > I think it makes sense to not send a selectstart-event on tap. A mouse
click
> > can be the start of a new selection (thus we send selectionstart), but a
quick
> > tap cannot start a new selection (so we do not send selectionstart). Right?
> Does
> > this mean it is OK to remove ClearHandleAfterTap()? :)
> > 
> > Well, the ClearHandleAfterTap test taps outside the textarea, causing an
> > unfocus.  Regardless of those fine details, that unfocus should cause the
> handle
> > to cleared.  So it doesn't seem like the test should be removed.  And it
> sounds
> > like this test failure is related to the "unpainting" issue you mentioned
> above.
> >  It sounds like the "unfocused selection visible in blue" probably also has
a
> > handle associated with it.  So hopefully after you fix the focus logic, the
> test
> > will go green naturally?
> 
> I've done the "unpainting" in PendingSelection.cpp already. PendingSelection
is
> owned by FrameSelection and communication goes from FS to PS. Now FS is
unaware
> of the paint-state. I like that. We could let the "is handle visible"-check
also
> include "and selection is focused". But do the users* of
> FrameSelection::isHandleVisible() really need to know that? To the outside
> world, the handle (= blue selection) is visible or hidden based on focused
> element. So for end users, it looks fine.
> 
> * InputMethodController, SelectionController, FrameView + tests

Maybe we should rename FrameSelection::isHandleVisible() to
FrameSelection::hasHandle() ? Handle = anything (possibly) visible to the user =
caret or selection. In Patch Set 20 +, just because the selection has a handle,
that doesn't necessarily mean that a handle is painted.

[commit-bot: I haz the power, 2017-03-13 05:57:25]

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

[commit-bot: I haz the power, 2017-03-13 05:57:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-03-13 09:18:38]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 09:18:52]

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-03-13 10:30:02]

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

[commit-bot: I haz the power, 2017-03-13 10:30:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2017-03-14 02:43:24]

On 2017/03/13 at 05:08:55, hugoh wrote:
> On 2017/03/12 09:41:55, hugoh wrote:
> > On 2017/03/10 20:07:57, aelias wrote:
> > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > Conclusions from offline talk with @yosin:
> > > > * We need hiding ("unpainting") now. Otherwise element.focus() from
> > JavaScript
> > > leaves an unfocused selection visible in blue. 
> > > > [...]
> > > > I think it makes sense to not send a selectstart-event on tap. A mouse
click
> > > can be the start of a new selection (thus we send selectionstart), but a
quick
> > > tap cannot start a new selection (so we do not send selectionstart).
Right?
> > Does
> > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > 
> > > Well, the ClearHandleAfterTap test taps outside the textarea, causing an
> > > unfocus.  Regardless of those fine details, that unfocus should cause the
> > handle
> > > to cleared.  So it doesn't seem like the test should be removed.  And it
> > sounds
> > > like this test failure is related to the "unpainting" issue you mentioned
> > above.
> > >  It sounds like the "unfocused selection visible in blue" probably also
has a
> > > handle associated with it.  So hopefully after you fix the focus logic,
the
> > test
> > > will go green naturally?
> > 
> > I've done the "unpainting" in PendingSelection.cpp already. PendingSelection
is
> > owned by FrameSelection and communication goes from FS to PS. Now FS is
unaware
> > of the paint-state. I like that. We could let the "is handle visible"-check
also
> > include "and selection is focused". But do the users* of
> > FrameSelection::isHandleVisible() really need to know that? To the outside
> > world, the handle (= blue selection) is visible or hidden based on focused
> > element. So for end users, it looks fine.
> > 
> > * InputMethodController, SelectionController, FrameView + tests
> 
> Maybe we should rename FrameSelection::isHandleVisible() to
FrameSelection::hasHandle() ? Handle = anything (possibly) visible to the user =
caret or selection. In Patch Set 20 +, just because the selection has a handle,
that doesn't necessarily mean that a handle is painted.

I'm not opposed to renaming to hasHandle(). Also I have a patch
https://codereview.chromium.org/2748483002/ that should make ClearHandleAfterTap
work with your CL.

[hugoh_UTC2, 2017-03-14 03:05:49]

On 2017/03/14 02:43:24, amaralp wrote:
> On 2017/03/13 at 05:08:55, hugoh wrote:
> > On 2017/03/12 09:41:55, hugoh wrote:
> > > On 2017/03/10 20:07:57, aelias wrote:
> > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > Conclusions from offline talk with @yosin:
> > > > > * We need hiding ("unpainting") now. Otherwise element.focus() from
> > > JavaScript
> > > > leaves an unfocused selection visible in blue. 
> > > > > [...]
> > > > > I think it makes sense to not send a selectstart-event on tap. A mouse
> click
> > > > can be the start of a new selection (thus we send selectionstart), but a
> quick
> > > > tap cannot start a new selection (so we do not send selectionstart).
> Right?
> > > Does
> > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > 
> > > > Well, the ClearHandleAfterTap test taps outside the textarea, causing an
> > > > unfocus.  Regardless of those fine details, that unfocus should cause
the
> > > handle
> > > > to cleared.  So it doesn't seem like the test should be removed.  And it
> > > sounds
> > > > like this test failure is related to the "unpainting" issue you
mentioned
> > > above.
> > > >  It sounds like the "unfocused selection visible in blue" probably also
> has a
> > > > handle associated with it.  So hopefully after you fix the focus logic,
> the
> > > test
> > > > will go green naturally?
> > > 
> > > I've done the "unpainting" in PendingSelection.cpp already.
PendingSelection
> is
> > > owned by FrameSelection and communication goes from FS to PS. Now FS is
> unaware
> > > of the paint-state. I like that. We could let the "is handle
visible"-check
> also
> > > include "and selection is focused". But do the users* of
> > > FrameSelection::isHandleVisible() really need to know that? To the outside
> > > world, the handle (= blue selection) is visible or hidden based on focused
> > > element. So for end users, it looks fine.
> > > 
> > > * InputMethodController, SelectionController, FrameView + tests
> > 
> > Maybe we should rename FrameSelection::isHandleVisible() to
> FrameSelection::hasHandle() ? Handle = anything (possibly) visible to the user
=
> caret or selection. In Patch Set 20 +, just because the selection has a
handle,
> that doesn't necessarily mean that a handle is painted.
> 
> I'm not opposed to renaming to hasHandle().
amaralp@, thanks for your feedback! Using this CL, the tap of
ClearHandleAfterTap does not affect the selection (it still has a handle).
- ASSERT_FALSE(selection().hasHandle());
+ ASSERT_TRUE(selection().hasHandle());
Do you think that is OK? (Or do you want hasHandle() to also include "and
selection is focused"?)

> Also I have a patch
> https://codereview.chromium.org/2748483002/ that should make
ClearHandleAfterTap
> work with your CL.
amaralp@, thank you very much for looking into why the test got !innerNode.

[hugoh_UTC2, 2017-03-14 03:15:41]

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

[hugoh_UTC2, 2017-03-14 13:17:21]

On 2017/03/14 03:05:49, hugoh wrote:
> On 2017/03/14 02:43:24, amaralp wrote:
> > On 2017/03/13 at 05:08:55, hugoh wrote:
> > > On 2017/03/12 09:41:55, hugoh wrote:
> > > > On 2017/03/10 20:07:57, aelias wrote:
> > > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > > Conclusions from offline talk with @yosin:
> > > > > > * We need hiding ("unpainting") now. Otherwise element.focus() from
> > > > JavaScript
> > > > > leaves an unfocused selection visible in blue. 
> > > > > > [...]
> > > > > > I think it makes sense to not send a selectstart-event on tap. A
mouse
> > click
> > > > > can be the start of a new selection (thus we send selectionstart), but
a
> > quick
> > > > > tap cannot start a new selection (so we do not send selectionstart).
> > Right?
> > > > Does
> > > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > > 
> > > > > Well, the ClearHandleAfterTap test taps outside the textarea, causing
an
> > > > > unfocus.  Regardless of those fine details, that unfocus should cause
> the
> > > > handle
> > > > > to cleared.  So it doesn't seem like the test should be removed.  And
it
> > > > sounds
> > > > > like this test failure is related to the "unpainting" issue you
> mentioned
> > > > above.
> > > > >  It sounds like the "unfocused selection visible in blue" probably
also
> > has a
> > > > > handle associated with it.  So hopefully after you fix the focus
logic,
> > the
> > > > test
> > > > > will go green naturally?
> > > > 
> > > > I've done the "unpainting" in PendingSelection.cpp already.
> PendingSelection
> > is
> > > > owned by FrameSelection and communication goes from FS to PS. Now FS is
> > unaware
> > > > of the paint-state. I like that. We could let the "is handle
> visible"-check
> > also
> > > > include "and selection is focused". But do the users* of
> > > > FrameSelection::isHandleVisible() really need to know that? To the
outside
> > > > world, the handle (= blue selection) is visible or hidden based on
focused
> > > > element. So for end users, it looks fine.
> > > > 
> > > > * InputMethodController, SelectionController, FrameView + tests
> > > 
> > > Maybe we should rename FrameSelection::isHandleVisible() to
> > FrameSelection::hasHandle() ? Handle = anything (possibly) visible to the
user
> =
> > caret or selection. In Patch Set 20 +, just because the selection has a
> handle,
> > that doesn't necessarily mean that a handle is painted.
> > 
> > I'm not opposed to renaming to hasHandle().
> amaralp@, thanks for your feedback! Using this CL, the tap of
> ClearHandleAfterTap does not affect the selection (it still has a handle).
> - ASSERT_FALSE(selection().hasHandle());
> + ASSERT_TRUE(selection().hasHandle());
> Do you think that is OK? (Or do you want hasHandle() to also include "and
> selection is focused"?)

I think I wrongly assumed that a handle is any visual ("blue") text selection.
I'm sorry! After reading crbug.com/633281 I understand that "handle" means the
kind of "selection-markers" that longpresses on text on Android give. Right? We
should probably add some comments in FrameSelection.h (and SelectionTemplate.h)
that explain what a handle really is :) And can't we replace the word "Handle"
with something like "TouchSelectionMarkers"? ;) ... is it browser-side (not
Blink) that paints these?

[amaralp, 2017-03-14 15:39:49]

On 2017/03/14 at 13:17:21, hugoh wrote:
> On 2017/03/14 03:05:49, hugoh wrote:
> > On 2017/03/14 02:43:24, amaralp wrote:
> > > On 2017/03/13 at 05:08:55, hugoh wrote:
> > > > On 2017/03/12 09:41:55, hugoh wrote:
> > > > > On 2017/03/10 20:07:57, aelias wrote:
> > > > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > > > Conclusions from offline talk with @yosin:
> > > > > > > * We need hiding ("unpainting") now. Otherwise element.focus()
from
> > > > > JavaScript
> > > > > > leaves an unfocused selection visible in blue. 
> > > > > > > [...]
> > > > > > > I think it makes sense to not send a selectstart-event on tap. A
mouse
> > > click
> > > > > > can be the start of a new selection (thus we send selectionstart),
but a
> > > quick
> > > > > > tap cannot start a new selection (so we do not send selectionstart).
> > > Right?
> > > > > Does
> > > > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > > > 
> > > > > > Well, the ClearHandleAfterTap test taps outside the textarea,
causing an
> > > > > > unfocus.  Regardless of those fine details, that unfocus should
cause
> > the
> > > > > handle
> > > > > > to cleared.  So it doesn't seem like the test should be removed. 
And it
> > > > > sounds
> > > > > > like this test failure is related to the "unpainting" issue you
> > mentioned
> > > > > above.
> > > > > >  It sounds like the "unfocused selection visible in blue" probably
also
> > > has a
> > > > > > handle associated with it.  So hopefully after you fix the focus
logic,
> > > the
> > > > > test
> > > > > > will go green naturally?
> > > > > 
> > > > > I've done the "unpainting" in PendingSelection.cpp already.
> > PendingSelection
> > > is
> > > > > owned by FrameSelection and communication goes from FS to PS. Now FS
is
> > > unaware
> > > > > of the paint-state. I like that. We could let the "is handle
> > visible"-check
> > > also
> > > > > include "and selection is focused". But do the users* of
> > > > > FrameSelection::isHandleVisible() really need to know that? To the
outside
> > > > > world, the handle (= blue selection) is visible or hidden based on
focused
> > > > > element. So for end users, it looks fine.
> > > > > 
> > > > > * InputMethodController, SelectionController, FrameView + tests
> > > > 
> > > > Maybe we should rename FrameSelection::isHandleVisible() to
> > > FrameSelection::hasHandle() ? Handle = anything (possibly) visible to the
user
> > =
> > > caret or selection. In Patch Set 20 +, just because the selection has a
> > handle,
> > > that doesn't necessarily mean that a handle is painted.
> > > 
> > > I'm not opposed to renaming to hasHandle().
> > amaralp@, thanks for your feedback! Using this CL, the tap of
> > ClearHandleAfterTap does not affect the selection (it still has a handle).
> > - ASSERT_FALSE(selection().hasHandle());
> > + ASSERT_TRUE(selection().hasHandle());
> > Do you think that is OK? (Or do you want hasHandle() to also include "and
> > selection is focused"?)
> 
> I think I wrongly assumed that a handle is any visual ("blue") text selection.
I'm sorry! After reading crbug.com/633281 I understand that "handle" means the
kind of "selection-markers" that longpresses on text on Android give. Right? We
should probably add some comments in FrameSelection.h (and SelectionTemplate.h)
that explain what a handle really is :) And can't we replace the word "Handle"
with something like "TouchSelectionMarkers"? ;) ... is it browser-side (not
Blink) that paints these?

The "handles" are used for touch selection for Android and anything that uses
Aura (Windows, Linux, and ChromeOS). I think renaming it to TouchSelectionHandle
would be better than TouchSelectionMarker because all of the browser side code
refers to them as handles. The handles are painted browser-side.
Also what are the scenarios where a loss a focus doesn't also result in a new
selection (when would the gray selections happen)? Are there any scenarios where
this could occur using touch?

[hugoh_UTC2, 2017-03-15 01:03:58]

On 2017/03/14 15:39:49, amaralp wrote:
> On 2017/03/14 at 13:17:21, hugoh wrote:
> > On 2017/03/14 03:05:49, hugoh wrote:
> > > On 2017/03/14 02:43:24, amaralp wrote:
> > > > On 2017/03/13 at 05:08:55, hugoh wrote:
> > > > > On 2017/03/12 09:41:55, hugoh wrote:
> > > > > > On 2017/03/10 20:07:57, aelias wrote:
> > > > > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > > > > Conclusions from offline talk with @yosin:
> > > > > > > > * We need hiding ("unpainting") now. Otherwise element.focus()
> from
> > > > > > JavaScript
> > > > > > > leaves an unfocused selection visible in blue. 
> > > > > > > > [...]
> > > > > > > > I think it makes sense to not send a selectstart-event on tap. A
> mouse
> > > > click
> > > > > > > can be the start of a new selection (thus we send selectionstart),
> but a
> > > > quick
> > > > > > > tap cannot start a new selection (so we do not send
selectionstart).
> > > > Right?
> > > > > > Does
> > > > > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > > > > 
> > > > > > > Well, the ClearHandleAfterTap test taps outside the textarea,
> causing an
> > > > > > > unfocus.  Regardless of those fine details, that unfocus should
> cause
> > > the
> > > > > > handle
> > > > > > > to cleared.  So it doesn't seem like the test should be removed. 
> And it
> > > > > > sounds
> > > > > > > like this test failure is related to the "unpainting" issue you
> > > mentioned
> > > > > > above.
> > > > > > >  It sounds like the "unfocused selection visible in blue" probably
> also
> > > > has a
> > > > > > > handle associated with it.  So hopefully after you fix the focus
> logic,
> > > > the
> > > > > > test
> > > > > > > will go green naturally?
> > > > > > 
> > > > > > I've done the "unpainting" in PendingSelection.cpp already.
> > > PendingSelection
> > > > is
> > > > > > owned by FrameSelection and communication goes from FS to PS. Now FS
> is
> > > > unaware
> > > > > > of the paint-state. I like that. We could let the "is handle
> > > visible"-check
> > > > also
> > > > > > include "and selection is focused". But do the users* of
> > > > > > FrameSelection::isHandleVisible() really need to know that? To the
> outside
> > > > > > world, the handle (= blue selection) is visible or hidden based on
> focused
> > > > > > element. So for end users, it looks fine.
> > > > > > 
> > > > > > * InputMethodController, SelectionController, FrameView + tests
> > > > > 
> > > > > Maybe we should rename FrameSelection::isHandleVisible() to
> > > > FrameSelection::hasHandle() ? Handle = anything (possibly) visible to
the
> user
> > > =
> > > > caret or selection. In Patch Set 20 +, just because the selection has a
> > > handle,
> > > > that doesn't necessarily mean that a handle is painted.
> > > > 
> > > > I'm not opposed to renaming to hasHandle().
> > > amaralp@, thanks for your feedback! Using this CL, the tap of
> > > ClearHandleAfterTap does not affect the selection (it still has a handle).
> > > - ASSERT_FALSE(selection().hasHandle());
> > > + ASSERT_TRUE(selection().hasHandle());
> > > Do you think that is OK? (Or do you want hasHandle() to also include "and
> > > selection is focused"?)
> > 
> > I think I wrongly assumed that a handle is any visual ("blue") text
selection.
> I'm sorry! After reading crbug.com/633281 I understand that "handle" means the
> kind of "selection-markers" that longpresses on text on Android give. Right?
We
> should probably add some comments in FrameSelection.h (and
SelectionTemplate.h)
> that explain what a handle really is :) And can't we replace the word "Handle"
> with something like "TouchSelectionMarkers"? ;) ... is it browser-side (not
> Blink) that paints these?
> 
> The "handles" are used for touch selection for Android and anything that uses
> Aura (Windows, Linux, and ChromeOS). I think renaming it to
TouchSelectionHandle
> would be better than TouchSelectionMarker because all of the browser side code
> refers to them as handles. The handles are painted browser-side.
> Also what are the scenarios where a loss a focus doesn't also result in a new
> selection (when would the gray selections happen)? Are there any scenarios
where
> this could occur using touch?

When a selection looses focus, it is just hidden (but it's not cleared anymore).
This CL should not change current appearance. I don't know under which condition
grey selections are shown. yosin@, where does Blink do gray selections?

[hugoh_UTC2, 2017-03-17 02:07:54]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:08:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-17 02:38:55]

On 2017/03/15 01:03:58, hugoh_UTC9 wrote:
> On 2017/03/14 15:39:49, amaralp wrote:
> > On 2017/03/14 at 13:17:21, hugoh wrote:
> > > On 2017/03/14 03:05:49, hugoh wrote:
> > > > On 2017/03/14 02:43:24, amaralp wrote:
> > > > > On 2017/03/13 at 05:08:55, hugoh wrote:
> > > > > > On 2017/03/12 09:41:55, hugoh wrote:
> > > > > > > On 2017/03/10 20:07:57, aelias wrote:
> > > > > > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > > > > > Conclusions from offline talk with @yosin:
> > > > > > > > > * We need hiding ("unpainting") now. Otherwise element.focus()
> > from
> > > > > > > JavaScript
> > > > > > > > leaves an unfocused selection visible in blue. 
> > > > > > > > > [...]
> > > > > > > > > I think it makes sense to not send a selectstart-event on tap.
A
> > mouse
> > > > > click
> > > > > > > > can be the start of a new selection (thus we send
selectionstart),
> > but a
> > > > > quick
> > > > > > > > tap cannot start a new selection (so we do not send
> selectionstart).
> > > > > Right?
> > > > > > > Does
> > > > > > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > > > > > 
> > > > > > > > Well, the ClearHandleAfterTap test taps outside the textarea,
> > causing an
> > > > > > > > unfocus.  Regardless of those fine details, that unfocus should
> > cause
> > > > the
> > > > > > > handle
> > > > > > > > to cleared.  So it doesn't seem like the test should be removed.

> > And it
> > > > > > > sounds
> > > > > > > > like this test failure is related to the "unpainting" issue you
> > > > mentioned
> > > > > > > above.
> > > > > > > >  It sounds like the "unfocused selection visible in blue"
probably
> > also
> > > > > has a
> > > > > > > > handle associated with it.  So hopefully after you fix the focus
> > logic,
> > > > > the
> > > > > > > test
> > > > > > > > will go green naturally?
> > > > > > > 
> > > > > > > I've done the "unpainting" in PendingSelection.cpp already.
> > > > PendingSelection
> > > > > is
> > > > > > > owned by FrameSelection and communication goes from FS to PS. Now
FS
> > is
> > > > > unaware
> > > > > > > of the paint-state. I like that. We could let the "is handle
> > > > visible"-check
> > > > > also
> > > > > > > include "and selection is focused". But do the users* of
> > > > > > > FrameSelection::isHandleVisible() really need to know that? To the
> > outside
> > > > > > > world, the handle (= blue selection) is visible or hidden based on
> > focused
> > > > > > > element. So for end users, it looks fine.
> > > > > > > 
> > > > > > > * InputMethodController, SelectionController, FrameView + tests
> > > > > > 
> > > > > > Maybe we should rename FrameSelection::isHandleVisible() to
> > > > > FrameSelection::hasHandle() ? Handle = anything (possibly) visible to
> the
> > user
> > > > =
> > > > > caret or selection. In Patch Set 20 +, just because the selection has
a
> > > > handle,
> > > > > that doesn't necessarily mean that a handle is painted.
> > > > > 
> > > > > I'm not opposed to renaming to hasHandle().
> > > > amaralp@, thanks for your feedback! Using this CL, the tap of
> > > > ClearHandleAfterTap does not affect the selection (it still has a
handle).
> > > > - ASSERT_FALSE(selection().hasHandle());
> > > > + ASSERT_TRUE(selection().hasHandle());
> > > > Do you think that is OK? (Or do you want hasHandle() to also include
"and
> > > > selection is focused"?)
> > > 
> > > I think I wrongly assumed that a handle is any visual ("blue") text
> selection.
> > I'm sorry! After reading crbug.com/633281 I understand that "handle" means
the
> > kind of "selection-markers" that longpresses on text on Android give. Right?
> We
> > should probably add some comments in FrameSelection.h (and
> SelectionTemplate.h)
> > that explain what a handle really is :) And can't we replace the word
"Handle"
> > with something like "TouchSelectionMarkers"? ;) ... is it browser-side (not
> > Blink) that paints these?
> > 
> > The "handles" are used for touch selection for Android and anything that
uses
> > Aura (Windows, Linux, and ChromeOS). I think renaming it to
> TouchSelectionHandle
> > would be better than TouchSelectionMarker because all of the browser side
code
> > refers to them as handles. The handles are painted browser-side.
> > Also what are the scenarios where a loss a focus doesn't also result in a
new
> > selection (when would the gray selections happen)? Are there any scenarios
> where
> > this could occur using touch?
> 
> When a selection looses focus, it is just hidden (but it's not cleared
anymore).
> This CL should not change current appearance. I don't know under which
condition
> grey selections are shown. yosin@, where does Blink do gray selections?

Now that https://codereview.chromium.org/2748483002/ has landed,
ClearHandleAfterTap should pass so this CL should be ready for landing. yosin@,
did you see my last changes? Any comments?

[commit-bot: I haz the power, 2017-03-17 04:16:41]

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

[commit-bot: I haz the power, 2017-03-17 04:16:43]

Dry run: This issue passed the CQ dry run.

[hugoh_UTC2, 2017-03-17 05:16:09]

Description was changed from

==========
Do not send redundant selectionchange-events (part 1)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
Part 2: crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[yosin_UTC9, 2017-03-17 10:11:34]

On 2017/03/17 at 02:38:55, hugoh wrote:
> On 2017/03/15 01:03:58, hugoh_UTC9 wrote:
> > On 2017/03/14 15:39:49, amaralp wrote:
> > > On 2017/03/14 at 13:17:21, hugoh wrote:
> > > > On 2017/03/14 03:05:49, hugoh wrote:
> > > > > On 2017/03/14 02:43:24, amaralp wrote:
> > > > > > On 2017/03/13 at 05:08:55, hugoh wrote:
> > > > > > > On 2017/03/12 09:41:55, hugoh wrote:
> > > > > > > > On 2017/03/10 20:07:57, aelias wrote:
> > > > > > > > > On 2017/03/10 at 10:10:37, hugoh wrote:
> > > > > > > > > > Conclusions from offline talk with @yosin:
> > > > > > > > > > * We need hiding ("unpainting") now. Otherwise
element.focus()
> > > from
> > > > > > > > JavaScript
> > > > > > > > > leaves an unfocused selection visible in blue. 
> > > > > > > > > > [...]
> > > > > > > > > > I think it makes sense to not send a selectstart-event on
tap. A
> > > mouse
> > > > > > click
> > > > > > > > > can be the start of a new selection (thus we send
selectionstart),
> > > but a
> > > > > > quick
> > > > > > > > > tap cannot start a new selection (so we do not send
> > selectionstart).
> > > > > > Right?
> > > > > > > > Does
> > > > > > > > > this mean it is OK to remove ClearHandleAfterTap()? :)
> > > > > > > > > 
> > > > > > > > > Well, the ClearHandleAfterTap test taps outside the textarea,
> > > causing an
> > > > > > > > > unfocus.  Regardless of those fine details, that unfocus
should
> > > cause
> > > > > the
> > > > > > > > handle
> > > > > > > > > to cleared.  So it doesn't seem like the test should be
removed. 
> > > And it
> > > > > > > > sounds
> > > > > > > > > like this test failure is related to the "unpainting" issue
you
> > > > > mentioned
> > > > > > > > above.
> > > > > > > > >  It sounds like the "unfocused selection visible in blue"
probably
> > > also
> > > > > > has a
> > > > > > > > > handle associated with it.  So hopefully after you fix the
focus
> > > logic,
> > > > > > the
> > > > > > > > test
> > > > > > > > > will go green naturally?
> > > > > > > > 
> > > > > > > > I've done the "unpainting" in PendingSelection.cpp already.
> > > > > PendingSelection
> > > > > > is
> > > > > > > > owned by FrameSelection and communication goes from FS to PS.
Now FS
> > > is
> > > > > > unaware
> > > > > > > > of the paint-state. I like that. We could let the "is handle
> > > > > visible"-check
> > > > > > also
> > > > > > > > include "and selection is focused". But do the users* of
> > > > > > > > FrameSelection::isHandleVisible() really need to know that? To
the
> > > outside
> > > > > > > > world, the handle (= blue selection) is visible or hidden based
on
> > > focused
> > > > > > > > element. So for end users, it looks fine.
> > > > > > > > 
> > > > > > > > * InputMethodController, SelectionController, FrameView + tests
> > > > > > > 
> > > > > > > Maybe we should rename FrameSelection::isHandleVisible() to
> > > > > > FrameSelection::hasHandle() ? Handle = anything (possibly) visible
to
> > the
> > > user
> > > > > =
> > > > > > caret or selection. In Patch Set 20 +, just because the selection
has a
> > > > > handle,
> > > > > > that doesn't necessarily mean that a handle is painted.
> > > > > > 
> > > > > > I'm not opposed to renaming to hasHandle().
> > > > > amaralp@, thanks for your feedback! Using this CL, the tap of
> > > > > ClearHandleAfterTap does not affect the selection (it still has a
handle).
> > > > > - ASSERT_FALSE(selection().hasHandle());
> > > > > + ASSERT_TRUE(selection().hasHandle());
> > > > > Do you think that is OK? (Or do you want hasHandle() to also include
"and
> > > > > selection is focused"?)
> > > > 
> > > > I think I wrongly assumed that a handle is any visual ("blue") text
> > selection.
> > > I'm sorry! After reading crbug.com/633281 I understand that "handle" means
the
> > > kind of "selection-markers" that longpresses on text on Android give.
Right?
> > We
> > > should probably add some comments in FrameSelection.h (and
> > SelectionTemplate.h)
> > > that explain what a handle really is :) And can't we replace the word
"Handle"
> > > with something like "TouchSelectionMarkers"? ;) ... is it browser-side
(not
> > > Blink) that paints these?
> > > 
> > > The "handles" are used for touch selection for Android and anything that
uses
> > > Aura (Windows, Linux, and ChromeOS). I think renaming it to
> > TouchSelectionHandle
> > > would be better than TouchSelectionMarker because all of the browser side
code
> > > refers to them as handles. The handles are painted browser-side.
> > > Also what are the scenarios where a loss a focus doesn't also result in a
new
> > > selection (when would the gray selections happen)? Are there any scenarios
> > where
> > > this could occur using touch?
> > 
> > When a selection looses focus, it is just hidden (but it's not cleared
anymore).
> > This CL should not change current appearance. I don't know under which
condition
> > grey selections are shown. yosin@, where does Blink do gray selections?
> 
> Now that https://codereview.chromium.org/2748483002/ has landed,
ClearHandleAfterTap should pass so this CL should be ready for landing. yosin@,
did you see my last changes? Any comments?

Gray selection comes from:

Color LayoutObject::selectionBackgroundColor() const {
  if (!isSelectable())
    return Color::transparent;

  if (RefPtr<ComputedStyle> pseudoStyle = getUncachedSelectionStyle())
    return resolveColor(*pseudoStyle, CSSPropertyBackgroundColor)
        .blendWithWhite();
  return frame()->selection().isFocusedAndActive()
             ? LayoutTheme::theme().activeSelectionBackgroundColor()
             : LayoutTheme::theme().inactiveSelectionBackgroundColor();
}

FrameSelection::isFocusedAndActive() is the flag.

bool FrameSelection::isFocusedAndActive() const {
  return m_focused && m_frame->page() &&
         m_frame->page()->focusController().isActive();
}

[hugoh_UTC2, 2017-03-18 03:59:05]

Thanks yosin@, could you please reconfirm your approval? (PS23 has many changes
since your last l g t m)

[yosin_UTC9, 2017-03-21 06:49:59]

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:394: if
(!isSelectionInDocument(selection, *frame.document()))
We can assume SelectionInFlatTree in FrameSeleciton always isNone() or
isConnected().
So, we don't need to use isSelectionInDocument().

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:403: const Node*
const nodeWhereSelectionStarts =
To check base/extent in focus element, we don't need to use
|VisibleSelectionInFlatTree|, please use |SelectionInFlatTree| here.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.h (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.h:85: bool
isSelectionInDocument(const VisibleSelectionInFlatTree&, const Document&);
Could you use |SelectionInFlatTree| instead of |VisibleSelectionInFlatTree|?
We are working to reduce usage of |VisibleSelection|.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2274:
{WebEditingCommandType::MoveDown, executeMoveDown,
Should we change of Move{Paragraph,Word,...}{Back,For}ward() too?
It is nice if we can move "EditorCommand.cpp", w/ tests, in another patch.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1044: //
TODO(blink-editing): Remove this clear(), crbug.com/692898.
nit: s/blink-editing/editing-dev/

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4312: //
TODO(blink-editing): Once we've fixed crbug.com/692898 test this too:
nit: s/blink-editing/editing-dev/

[hugoh_UTC2, 2017-03-22 02:50:14]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 02:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-03-22 02:54:47]

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:394: if
(!isSelectionInDocument(selection, *frame.document()))
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> We can assume SelectionInFlatTree in FrameSeleciton always isNone() or
> isConnected().
> So, we don't need to use isSelectionInDocument().

Done.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:403: const Node*
const nodeWhereSelectionStarts =
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> To check base/extent in focus element, we don't need to use
> |VisibleSelectionInFlatTree|, please use |SelectionInFlatTree| here.

FrameSelection has no public getter that returns the |SelectionInFlatTree|...
Isn't computeVisibleSelectionInFlatTree() quick to call since it uses caching?

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.h (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.h:85: bool
isSelectionInDocument(const VisibleSelectionInFlatTree&, const Document&);
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> Could you use |SelectionInFlatTree| instead of |VisibleSelectionInFlatTree|?
> We are working to reduce usage of |VisibleSelection|.

Yes, PendingSelection::commit has a |SelectionInFlatTree| already, the one it
constructs by calling calcVisibleSelection(). I try this in PS24.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2274:
{WebEditingCommandType::MoveDown, executeMoveDown,
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> Should we change of Move{Paragraph,Word,...}{Back,For}ward() too?
> It is nice if we can move "EditorCommand.cpp", w/ tests, in another patch.

I introduced these changes to make LayoutTests/fast/spatial-navigation pass. Now
that we postpone fixing selectionchange of spatnav (see comment in
FocusController::advanceFocusDirectionallyInContainer) the changes in this file
might not be needed until we tackle crbug.com/692898. I try this in PS24.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1044: //
TODO(blink-editing): Remove this clear(), crbug.com/692898.
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> nit: s/blink-editing/editing-dev/

Done.

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4312: //
TODO(blink-editing): Once we've fixed crbug.com/692898 test this too:
On 2017/03/21 06:49:59, yosin_UTC9 wrote:
> nit: s/blink-editing/editing-dev/

Done.

[commit-bot: I haz the power, 2017-03-22 04:52:23]

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

[commit-bot: I haz the power, 2017-03-22 04:52:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[yosin_UTC9, 2017-03-22 05:08:00]

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:377: bool
selectionHasFocus(const LocalFrame& frame,
Let's take Document from selection instead of extra parameter |frame|.

[hugoh_UTC2, 2017-03-22 07:50:33]

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:377: bool
selectionHasFocus(const LocalFrame& frame,
On 2017/03/22 05:07:59, yosin_UTC9 wrote:
> Let's take Document from selection instead of extra parameter |frame|.

SelectionTemplate currently keeps Document private but sure, we can make it
public.

[yosin_UTC9, 2017-03-22 07:53:19]

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:377: bool
selectionHasFocus(const LocalFrame& frame,
On 2017/03/22 at 07:50:33, hugoh_UTC9 wrote:
> On 2017/03/22 05:07:59, yosin_UTC9 wrote:
> > Let's take Document from selection instead of extra parameter |frame|.
> 
> SelectionTemplate currently keeps Document private but sure, we can make it
public.

You can get Document by Position, e.g. selection.base().document()

[hugoh_UTC2, 2017-03-22 08:32:38]

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:377: bool
selectionHasFocus(const LocalFrame& frame,
On 2017/03/22 07:53:19, yosin_UTC9 wrote:
> On 2017/03/22 at 07:50:33, hugoh_UTC9 wrote:
> > On 2017/03/22 05:07:59, yosin_UTC9 wrote:
> > > Let's take Document from selection instead of extra parameter |frame|.
> > 
> > SelectionTemplate currently keeps Document private but sure, we can make it
> public.
> 
> You can get Document by Position, e.g. selection.base().document()

Done. See PS25.

[hugoh_UTC2, 2017-03-22 08:55:16]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 08:55:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-03-22 09:11:35]

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:378: const Document*
document = selection.base().document();
Let's handle document == nullptr case, selection.isNone(), for ease of using
this function.

[hugoh_UTC2, 2017-03-22 09:19:14]

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:378: const Document*
document = selection.base().document();
On 2017/03/22 09:11:35, yosin_UTC9 wrote:
> Let's handle document == nullptr case, selection.isNone(), for ease of using
> this function.

You mean when selection.isNone(), we return false? 

!selection.isNone() => document != nullptr ?

[yosin_UTC9, 2017-03-22 09:26:35]

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:378: const Document*
document = selection.base().document();
On 2017/03/22 at 09:19:14, hugoh_UTC9 wrote:
> On 2017/03/22 09:11:35, yosin_UTC9 wrote:
> > Let's handle document == nullptr case, selection.isNone(), for ease of using
> > this function.
> 
> You mean when selection.isNone(), we return false? 
> 
> !selection.isNone() => document != nullptr ?

Yes, just return false if selection.isNone(),

So, we can write

if (!document)
  return false;

Since there is no selection, it can not have focus.

[hugoh_UTC2, 2017-03-22 09:41:11]

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:378: const Document*
document = selection.base().document();
On 2017/03/22 09:26:35, yosin_UTC9 wrote:
> On 2017/03/22 at 09:19:14, hugoh_UTC9 wrote:
> > On 2017/03/22 09:11:35, yosin_UTC9 wrote:
> > > Let's handle document == nullptr case, selection.isNone(), for ease of
using
> > > this function.
> > 
> > You mean when selection.isNone(), we return false? 
> > 
> > !selection.isNone() => document != nullptr ?
> 
> Yes, just return false if selection.isNone(),
> 
> So, we can write
> 
> if (!document)
>   return false;
> 
> Since there is no selection, it can not have focus.
Done. See PS26.

[yosin_UTC9, 2017-03-23 01:26:20]

lgtm w/ small nits

https://codereview.chromium.org/2616623002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:382: if (!document)
nit: Since selection.isNone() implies selection.base().document() == nullptr,
we don't need to have this if-statement.

[hugoh_UTC2, 2017-03-23 01:38:06]

Thanks! Nit fixed. Do we need to reconfirm approval from @bokan ?

https://codereview.chromium.org/2616623002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2616623002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:382: if (!document)
On 2017/03/23 01:26:20, yosin_UTC9 wrote:
> nit: Since selection.isNone() implies selection.base().document() == nullptr,
> we don't need to have this if-statement.
Done. See PS27.

[yosin_UTC9, 2017-03-23 03:46:23]

On 2017/03/23 at 01:38:06, hugoh wrote:
> Thanks! Nit fixed. Do we need to reconfirm approval from @bokan ?
Since this patch takes long iteration, it is better to get reconfirmation.

[hugoh_UTC2, 2017-03-23 06:12:27]

On 2017/03/23 03:46:23, yosin_UTC9 wrote:
> On 2017/03/23 at 01:38:06, hugoh wrote:
> > Thanks! Nit fixed. Do we need to reconfirm approval from @bokan ?
> Since this patch takes long iteration, it is better to get reconfirmation.

bokan@ PTAL

[hugoh_UTC2, 2017-03-27 08:33:47]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 08:34:02]

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-03-27 08:42:22]

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

[commit-bot: I haz the power, 2017-03-27 08:42:23]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2017-03-27 11:41:51]

rs lgtm

[hugoh_UTC2, 2017-03-27 11:46:48]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-03-27 11:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2616623002/#ps540001
(title: "Rebase and refix nit (compilation error slipped)")

[commit-bot: I haz the power, 2017-03-27 11:47:04]

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-03-27 11:54:55]

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

[commit-bot: I haz the power, 2017-03-27 11:54:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hugoh_UTC2, 2017-03-27 12:03:01]

aelias@, I need your approval on ImeTest.java ;) PTAL.

[aelias_OOO_until_Jul13, 2017-03-27 21:54:49]

Android test lgtm

[hugoh_UTC2, 2017-03-28 01:59:01]

The CQ bit was checked by hugoh@opera.com

[commit-bot: I haz the power, 2017-03-28 01:59:35]

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-03-28 02:09:12]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1490666341355630,
"parent_rev": "30f7588201a275c75d0382ad4d63d4329a2fd9e8", "commit_rev":
"31d55d991baf03aa9bc3ff5d5e67f709eee77959"}

[commit-bot: I haz the power, 2017-03-28 02:10:00]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

[commit-bot: I haz the power, 2017-03-28 02:10:03]

Committed patchset #28 (id:540001) as
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...

[imcheng, 2017-03-28 20:26:49]

A revert of this CL (patchset #28 id:540001) has been created in
https://codereview.chromium.org/2784653002/ by imcheng@chromium.org.

The reason for reverting is: Broke webkit_tests:
https://bugs.chromium.org/p/chromium/issues/detail?id=706119.

[hugoh_UTC2, 2017-03-29 02:37:04]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

[hugoh_UTC2, 2017-03-29 02:38:35]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

to

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly.

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

[hugoh_UTC2, 2017-03-29 02:40:33]

Description was changed from

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly.

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#459984}
Committed:
https://chromium.googlesource.com/chromium/src/+/31d55d991baf03aa9bc3ff5d5e67...
==========

to

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[hugoh_UTC2, 2017-03-29 02:41:11]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-03-29 02:41:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2616623002/#ps560001
(title: "Reland (fix Win LayoutTests)")

[commit-bot: I haz the power, 2017-03-29 02:42:02]

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-03-29 04:43:31]

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

[commit-bot: I haz the power, 2017-03-29 04:43:34]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[hugoh_UTC2, 2017-03-29 05:23:20]

The CQ bit was checked by hugoh@opera.com

[commit-bot: I haz the power, 2017-03-29 05:23:48]

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-03-29 07:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 560001, "attempt_start_ts": 1490765000916810,
"parent_rev": "625d39366a93f55aa9afd1f4ffeac51b0fcaecf8", "commit_rev":
"92bf3c29fccac551acceff254e53d7ea73367158"}

[commit-bot: I haz the power, 2017-03-29 07:20:12]

Description was changed from

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#460314}
Committed:
https://chromium.googlesource.com/chromium/src/+/92bf3c29fccac551acceff254e53...
==========

[commit-bot: I haz the power, 2017-03-29 07:20:14]

Committed patchset #29 (id:560001) as
https://chromium.googlesource.com/chromium/src/+/92bf3c29fccac551acceff254e53...

[aelias_OOO_until_Jul13, 2017-03-31 21:24:10]

A revert of this CL (patchset #29 id:560001) has been created in
https://codereview.chromium.org/2791753002/ by aelias@chromium.org.

The reason for reverting is: Caused http://crbug.com/707156, please reland after
fixing it.

BUG=707156.

[hugoh_UTC2, 2017-04-11 04:47:22]

Description was changed from

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#460314}
Committed:
https://chromium.googlesource.com/chromium/src/+/92bf3c29fccac551acceff254e53...
==========

to

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#460314}
Committed:
https://chromium.googlesource.com/chromium/src/+/92bf3c29fccac551acceff254e53...
==========

[hugoh_UTC2, 2017-04-11 04:47:53]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 04:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-04-11 04:52:07]

Description was changed from

==========
Reland: Do not send redundant selectionchange-events (decouple focus)

Reason for reland:
Update Win7/10 LayoutTests correctly: crbug.com/706119

Background:
Blink tells browser-side when a new <input>-element gets focus.
The information is passed in the
ViewHostMsg_TextInputStateChanged-message.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement() 
always clears the selection when a new element gets focus.

Solution:
When JavaScript moves focus to an element, element.focus(),
and when the user moves focus using tab-key navigation or
mouse, we don't clear the old selection (we hide it). This
means, we only send one selectionchange event, not two, for
each caret jump (as in Firefox).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#460314}
Committed:
https://chromium.googlesource.com/chromium/src/+/92bf3c29fccac551acceff254e53...
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
When you click anywhere on a webpage or hit the tab-key
both focus and FrameSelection moves. A click selects nothing
so the FrameSelection represents an invisible caret. When you
focus an <input>-element ViewHostMsg_TextInputStateChanged-
message is sent to browser-side.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. When a user moves
focus by mouse click or tab-key we move the caret
(aka selection) only once: we send one selectionchange-
event, not two, for each caret jump (as in Firefox). When
JS moves focus, selection remains unchanged.

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

[commit-bot: I haz the power, 2017-04-11 06:39:48]

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

[commit-bot: I haz the power, 2017-04-11 06:39:49]

Dry run: This issue passed the CQ dry run.

[hugoh_UTC2, 2017-04-12 06:25:44]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 06:26:05]

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-12 08:03:00]

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

[commit-bot: I haz the power, 2017-04-12 08:03:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hugoh_UTC2, 2017-04-12 11:08:24]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 11:08:41]

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-12 11:11:14]

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

[commit-bot: I haz the power, 2017-04-12 11:11:17]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[hugoh_UTC2, 2017-04-13 03:44:36]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-04-13 03:53:55]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

Background:
When you click anywhere on a webpage or hit the tab-key
both focus and FrameSelection moves. A click selects nothing
so the FrameSelection represents an invisible caret. When you
focus an <input>-element ViewHostMsg_TextInputStateChanged-
message is sent to browser-side.

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. When a user moves
focus by mouse click or tab-key we move the caret
(aka selection) only once: we send one selectionchange-
event, not two, for each caret jump (as in Firefox). When
JS moves focus, selection remains unchanged.

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

Follow-up will remove the remaining redundant clears:
crbug.com/692898 (tab jumps to non-editable elements).

BUG=678601, 679635, 699015
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired.
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input>-element a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).
==========

[commit-bot: I haz the power, 2017-04-13 05:22:59]

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

[commit-bot: I haz the power, 2017-04-13 05:23:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-04-13 06:16:18]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 06:16:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-04-13 06:37:31]

Turns out we can't just hide the selection when it doesn't "have focus"; we
unintentionally hid the selection when focus went to a DOM node that had no
parent/child-relation to the selection.

I introduced the "paint only if focused" only to fix <embed> in
mouse-click-plugin-clears-selection.html but after testing current Chrome and
Firefox I see that this is also needed for <canvas>, <img> and <video> etc...
For example,
 <input value="hey" id="x">
 <script>document.getElementById("x").select();</script>
 <video>
Clicking on <video> should hide the selection in the <input>-field.

So let's hide the selection when focus (here changed by the mouse click) leaves
a text-field and goes to something that isn't a text-field.

Upon relayout, a hidden selection must be kept hidden and a visible selection
must be kept visible. That is why I introduced PaintState in LayoutSelection.

yosin@, what do you think of this idea? PTAL at PS34. (I've updated this CL's
description to reflect this new approach).

[hugoh_UTC2, 2017-04-13 07:25:47]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 07:26:13]

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-13 08:54:31]

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

[commit-bot: I haz the power, 2017-04-13 08:54:34]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-13 09:31:50]

Approach is good since this is simpler implementation of
ClearSelectionIfNeeded().

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:339: const Element*
focus = GetDocument().FocusedElement();
Since Document::FocusedElement() is simple getter.

We can write

const Element* const focus = GetDocument().FocusedElement() ?
GetDocument().FocusedElement() : GetDOcument.documentElement();
if (!focus) {
  // No focused element means document root has focus.
  return;
}

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:344: return;
We still need to call
frame_caret_->ScheduleVisualUpdateForPaintInvalidationIfNeeded();

We should rest |text_control_focus_| to false.

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:312: bool
text_control_focus_ = false;
How about |text_control_had_focus_| or another good name?
|text_control_focus_| doesn't seem to hold |bool|.

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: if (hint ==
PaintHint::kHide)
How about

if (hint == PaintHint::kKeep)
  return;
force_hide_ = hint == PaintHint::kHide;

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/LayoutSelection.h (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/LayoutSelection.h:36: enum PaintHint {
kHide, kKeep, kPaint };
nit: s/enum/enum class/

[hugoh_UTC2, 2017-04-14 01:34:47]

yosin@ PTAL

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:339: const Element*
focus = GetDocument().FocusedElement();
On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> Since Document::FocusedElement() is simple getter.
> 
> We can write
> 
> const Element* const focus = GetDocument().FocusedElement() ?
> GetDocument().FocusedElement() : GetDOcument.documentElement();
> if (!focus) {
>   // No focused element means document root has focus.
>   return;
> }
Done. (But the comment must go above the ternary operator).

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:344: return;
On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> We still need to call
> frame_caret_->ScheduleVisualUpdateForPaintInvalidationIfNeeded();
> 
Done.

> We should rest |text_control_focus_| to false.
Done. We need this early return to protect us from
LayoutTests/editing/selection/selection-crash.html (I've now added a comment
about that), in all other cases I've tried, focus has been an Element (or
documentElement). So I was thinking this exceptional case should rather "keep
current value". But let's reset it to false for consistency if you prefer.

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:312: bool
text_control_focus_ = false;
On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> How about |text_control_had_focus_| or another good name?
> |text_control_focus_| doesn't seem to hold |bool|.

Done. I renamed it to text_control_focused_.

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: if (hint ==
PaintHint::kHide)
On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> How about
> 
> if (hint == PaintHint::kKeep)
>   return;
> force_hide_ = hint == PaintHint::kHide;

Acknowledged. Semantically the same so this is a matter of taste. Hmm. :) I
think current logic is easier to read (no == operator to calculate).

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/LayoutSelection.h (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/LayoutSelection.h:36: enum PaintHint {
kHide, kKeep, kPaint };
On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> nit: s/enum/enum class/

Done.

[yosin_UTC9, 2017-04-14 02:29:16]

lgtm

Please remember Blink prefers early-return style. ;-)

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2616623002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:117: if (hint ==
PaintHint::kHide)
On 2017/04/14 at 01:34:47, hugoh_UTC9 wrote:
> On 2017/04/13 09:31:50, yosin_UTC9 wrote:
> > How about
> > 
> > if (hint == PaintHint::kKeep)
> >   return;
> > force_hide_ = hint == PaintHint::kHide;
> 
> Acknowledged. Semantically the same so this is a matter of taste. Hmm. :) I
think current logic is easier to read (no == operator to calculate).

Blink prefers early-return style, though.

[hugoh_UTC2, 2017-04-14 02:34:25]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input>-element a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input> text-field, a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).
==========

[hugoh_UTC2, 2017-04-14 03:01:21]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-04-14 03:01:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2616623002/#ps680001
(title: "Added yosin's suggestions")

[commit-bot: I haz the power, 2017-04-14 03:02:13]

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-14 03:05:39]

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

[commit-bot: I haz the power, 2017-04-14 03:05:43]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hugoh_UTC2, 2017-04-14 05:17:00]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-04-14 05:17:01]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
aelias@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2616623002/#ps700001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-14 05:17:17]

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-14 07:10:36]

CQ is committing da patch.

Bot data: {"patchset_id": 700001, "attempt_start_ts": 1492147020793140,
"parent_rev": "188c1056bbfb9c98eca691c12f5641d4204f6066", "commit_rev":
"0f65d25a4097959d977dbb1077717323438240a6"}

[commit-bot: I haz the power, 2017-04-14 07:12:00]

Description was changed from

==========
Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input> text-field, a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).
==========

to

==========
Do not send redundant selectionchange-events (decouple focus)

This CL aims to remove redundant selectionchange-events
that were sent upon change of focus caused by element.focus(),
tab-navigation, spatnav and mouse-clicks.

Goals:
1. Send == one selectionchange-event, not two, for each caret jump.
2. Send <= one selectionchange-event, not two, for each focus jump.

Background:
When you click/tab to an <input> text-field, a
ViewHostMsg_TextInputStateChanged-message is sent to browser-side. 

Problem:
With current logic, RenderFrameImpl::didChangeSelection is
called twice so two ViewHostMsg_TextInputStateChanged-messages
are sent to browser-side:
 (1) when focus leaves an <input>-field (unnecessary!).
 (2) when focus enters another <input>-field.

Worse, also the web page gets two selectionchange events.
The first one is immediately invalid so the webpage should
not react to it.

(1) happens because FocusController::setFocusedElement()
always clears the selection when a new element gets focus.

Solution:
Do not clear selection when focus moves. To keep current visual
behavior when focus moves away from a text-field we need to hide
that field's selection (clicking outside a text-field hides its
selection).

Test updates:
1. Check for one selectionchange event, not two.
2. LayoutTests' trees now expect the "hidden" selection.
3. A new test in WebFrameTest.cpp tests tab-key navigation.

BUG=678601, 679635, 699015, 692898
TEST=In content_shell, select some text in an <input>-field,
     click another <input>-field (move focus).
     Notice: one selectionchange event is fired (as in Firefox).
TEST=In content_shell, select some text in an <input>-field,
     click on an <img>. Notice: selection gets hid and
     zero selectionchange events are fired (as in Firefox).

Review-Url: https://codereview.chromium.org/2616623002
Cr-Commit-Position: refs/heads/master@{#464698}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f65d25a4097959d977dbb107771...
==========

[commit-bot: I haz the power, 2017-04-14 07:12:03]

Committed patchset #36 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/0f65d25a4097959d977dbb107771...