Don't clear selection on focus change except for text form control element

Don't clear selection on focus change except for text form control element

Before this patch, Blink clears selection when focus is changed. This patch
changes to clear selection for text field only. Thus, we keep selection for
content editable.

This patch also updates below tests to follow new behavior introduced by this
patch.

- fast/dom/blur-contenteditable.html
 Render tree dump has a caret.
- fast/layers/scroll-rect-to-visible.html
 Render tree dump has a caret.
- fast/events/selectionchange-user-initiated.html
 One "selectionchange" event from clear selection for content editable has
 been gone.
- /fast/events/selectstart-prevent-selectall.html
 same as above.
- fast/forms/focus-selection-input.html
 One "selectionchange" event from clear selection for caret on LABEL element
 by test step 5 been gone.
- fast/forms/focus-selection-textarea.html
 same as above

BUG=351981
TEST=LayoutTests/editing/selection/keep-selection-after-set-focus.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180228

[yosin_UTC9, 2014-07-29 08:29:39]

Could you review this patch?
Thanks in advance.

Note: {linux,mac,win}_blink_rel bots failures aren't related this patch.

[leviw_travelin_and_unemployed, 2014-07-29 19:56:40]

https://codereview.chromium.org/424973003/diff/20001/LayoutTests/editing/sele...
File LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
(right):

https://codereview.chromium.org/424973003/diff/20001/LayoutTests/editing/sele...
LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html:6:
description('Unfocused TEXTAREA should not accept editing.');
This contradicts the results.

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:680: if (newFocusedNode &&
(newFocusedNode->hasEditableStyle()
Is it actually possible for newFocusedNode to be null here?

[yosin_UTC9, 2014-07-30 00:48:07]

PTAL

https://codereview.chromium.org/424973003/diff/20001/LayoutTests/editing/sele...
File LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
(right):

https://codereview.chromium.org/424973003/diff/20001/LayoutTests/editing/sele...
LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html:6:
description('Unfocused TEXTAREA should not accept editing.');
On 2014/07/29 19:56:40, leviw wrote:
> This contradicts the results.

Done.

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:680: if (newFocusedNode &&
(newFocusedNode->hasEditableStyle()
On 2014/07/29 19:56:40, leviw wrote:
> Is it actually possible for newFocusedNode to be null here?

Yes.  EventHandler::handleMouseFocus() calls
FocusController::setFocusedElement() with nullptr.
Here is excerpt from EventHandler.cpp:

 // We call setFocusedElement even with !element in order to blur
 // current focus element when a link is clicked; this is expected by
 // some sites that rely on onChange handlers running from form
 // fields before the button click is processed.
 if (!page->focusController().setFocusedElement(0, m_frame))

[leviw_travelin_and_unemployed, 2014-07-30 19:19:19]

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:680: if (newFocusedNode &&
(newFocusedNode->hasEditableStyle()
On 2014/07/30 00:48:07, Yosi_UTC9 wrote:
> On 2014/07/29 19:56:40, leviw wrote:
> > Is it actually possible for newFocusedNode to be null here?
> 
> Yes.  EventHandler::handleMouseFocus() calls
> FocusController::setFocusedElement() with nullptr.
> Here is excerpt from EventHandler.cpp:
> 
>  // We call setFocusedElement even with !element in order to blur
>  // current focus element when a link is clicked; this is expected by
>  // some sites that rely on onChange handlers running from form
>  // fields before the button click is processed.
>  if (!page->focusController().setFocusedElement(0, m_frame))

I know we can have no newFocusedNode, I'm just not convinced we'd get this far
in the function before early returning.

Anyhow, lgtm.

https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:681: || (newFocusedNode->isElementNode() &&
isHTMLTextFormControlElement(toElement(newFocusedNode))))) {
How about a named function to document why you're checking hasEditableStyle and
isHTMLTextFormControlElement?

[yosin_UTC9, 2014-07-31 07:50:10]

On 2014/07/30 19:19:19, leviw wrote:
>
https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
> File Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
> Source/core/page/FocusController.cpp:680: if (newFocusedNode &&
> (newFocusedNode->hasEditableStyle()
> On 2014/07/30 00:48:07, Yosi_UTC9 wrote:
> > On 2014/07/29 19:56:40, leviw wrote:
> > > Is it actually possible for newFocusedNode to be null here?
> > 
> > Yes.  EventHandler::handleMouseFocus() calls
> > FocusController::setFocusedElement() with nullptr.
> > Here is excerpt from EventHandler.cpp:
> > 
> >  // We call setFocusedElement even with !element in order to blur
> >  // current focus element when a link is clicked; this is expected by
> >  // some sites that rely on onChange handlers running from form
> >  // fields before the button click is processed.
> >  if (!page->focusController().setFocusedElement(0, m_frame))
> 
> I know we can have no newFocusedNode, I'm just not convinced we'd get this far
> in the function before early returning.
> 
> Anyhow, lgtm.
> 
>
https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
> File Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
> Source/core/page/FocusController.cpp:681: || (newFocusedNode->isElementNode()
&&
> isHTMLTextFormControlElement(toElement(newFocusedNode))))) {
> How about a named function to document why you're checking hasEditableStyle
and
> isHTMLTextFormControlElement?

This is my bad. there is no good reason to do so. We can completed remove
|clearSelectionIfNeeded|.

The reason is passed following tests, although we should adopt them to new
behavior:
- editing/inserting/return-key-in-hidden-field.html
 * this test inserts newline into hidden textarea
 * I'm figuring out this test to be robust.
- editing/pasteboard/copy-in-password-field.html
 * two selection change events are dropped.
 * I'll convert this test to JS test.
- editing/pasteboard/paste-plaintext-user-select-none.html
 * two selection change events are dropped.
 * I'll convert this test to JS test.
- editing/pasteboard/select-element-1.html
 * two selection change events are dropped.
 * I'll convert this test to JS test.
- editing/selection/4895428-3.html
 * New behavior render selection for pixel test result. This is timing issue
when content_shell dump pixels.
 * I'll convert this test to JS test.
- editing/spelling/markers.html
 * selection rendering timing difference.
 * I'm figuring out this test to be robust.

[yosin_UTC9, 2014-07-31 09:46:39]

On 2014/07/31 07:50:10, Yosi_UTC9 wrote:
> On 2014/07/30 19:19:19, leviw wrote:
> >
>
https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
> > File Source/core/page/FocusController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/424973003/diff/20001/Source/core/page/FocusCo...
> > Source/core/page/FocusController.cpp:680: if (newFocusedNode &&
> > (newFocusedNode->hasEditableStyle()
> > On 2014/07/30 00:48:07, Yosi_UTC9 wrote:
> > > On 2014/07/29 19:56:40, leviw wrote:
> > > > Is it actually possible for newFocusedNode to be null here?
> > > 
> > > Yes.  EventHandler::handleMouseFocus() calls
> > > FocusController::setFocusedElement() with nullptr.
> > > Here is excerpt from EventHandler.cpp:
> > > 
> > >  // We call setFocusedElement even with !element in order to blur
> > >  // current focus element when a link is clicked; this is expected by
> > >  // some sites that rely on onChange handlers running from form
> > >  // fields before the button click is processed.
> > >  if (!page->focusController().setFocusedElement(0, m_frame))
> > 
> > I know we can have no newFocusedNode, I'm just not convinced we'd get this
far
> > in the function before early returning.
> > 
> > Anyhow, lgtm.
> > 
> >
>
https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
> > File Source/core/page/FocusController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
> > Source/core/page/FocusController.cpp:681: ||
(newFocusedNode->isElementNode()
> &&
> > isHTMLTextFormControlElement(toElement(newFocusedNode))))) {
> > How about a named function to document why you're checking hasEditableStyle
> and
> > isHTMLTextFormControlElement?
> 
> This is my bad. there is no good reason to do so. We can completed remove
> |clearSelectionIfNeeded|.
> 
> The reason is passed following tests, although we should adopt them to new
> behavior:
> - editing/inserting/return-key-in-hidden-field.html
>  * this test inserts newline into hidden textarea
>  * I'm figuring out this test to be robust.
> - editing/pasteboard/copy-in-password-field.html
>  * two selection change events are dropped.
>  * I'll convert this test to JS test.
> - editing/pasteboard/paste-plaintext-user-select-none.html
>  * two selection change events are dropped.
>  * I'll convert this test to JS test.
> - editing/pasteboard/select-element-1.html
>  * two selection change events are dropped.
>  * I'll convert this test to JS test.
> - editing/selection/4895428-3.html
>  * New behavior render selection for pixel test result. This is timing issue
> when content_shell dump pixels.
>  * I'll convert this test to JS test.
> - editing/spelling/markers.html
>  * selection rendering timing difference.
>  * I'm figuring out this test to be robust.

I updated above tests, except for markers.html, in http://crrev.com/431933002.

editing/spelling/markers.html tests failed with:
FAIL internals.markerCountForNode(element.firstChild, next.marker) failed to
change to 1 in 0.5 seconds.
FAIL range.toString() should be a. Threw exception TypeError: Cannot read
property 'toString' of null
FAIL internals.markerCountForNode(element.firstChild, next.marker) failed to
change to 1 in 0.5 seconds.
FAIL range.toString() should be zz. Threw exception TypeError: Cannot read
property 'toString' of null

It seems spelling/grammar checker depends on clearing selection at focus change.

[yosin_UTC9, 2014-08-01 04:39:19]

Waiting for trybots

Finally I found the root cause of failure of editing/spelling/markers.html.

During focus change, spell checking for old selection is kicked by
FrameSelection::setSelection with CloseTyping flag, selection motion during
typing text call setSeelction without CloseTyping. Before this patch calling
setSeelction with CloseTyping is done by FrameSelection::clear().

Thus, we need to call setSeleciton() with CloseTyping within Element::focus().

https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/424973003/diff/40001/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:681: || (newFocusedNode->isElementNode() &&
isHTMLTextFormControlElement(toElement(newFocusedNode))))) {
On 2014/07/30 19:19:19, leviw wrote:
> How about a named function to document why you're checking hasEditableStyle
and
> isHTMLTextFormControlElement?

Fumm... There are no good reasons to exclude them.
I should think again.

[leviw_travelin_and_unemployed, 2014-08-05 00:28:41]

The code change looks reasonable, but you have failing tests. Can you upload new
test results for those to make it clear they're expected changes?

[yosin_UTC9, 2014-08-06 00:42:09]

Following tests are failed. I'm investigating... :-<
Note: these failed tests are union of mac, linux and win bots.

1 fast/dom/blur-contenteditable.html
2 fast/events/selectionchange-user-initiated.html
3 fast/events/selectstart-prevent-selectall.html
4 fast/events/touch/gesture/focus-selectionchange-on-tap.html
5 fast/forms/focus-selection-input.html
6 fast/forms/focus-selection-textarea.html
7 fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html
8 fast/layers/scroll-rect-to-visible.html
9 fast/spatial-navigation/snav-input.html
10 fast/spatial-navigation/snav-textarea.html
11 plugins/mouse-click-plugin-clears-selection.html
12 plugins/touch-events-details.html
13 plugins/touch-events-synthesized.html
14 plugins/transformed-events.html
15
virtual/softwarecompositing/squashing/invalidations-with-large-negative-margin.html

[yosin_UTC9, 2014-08-12 09:49:40]

On 2014/08/06 00:42:09, Yosi_UTC9 wrote:
> Following tests are failed. I'm investigating... :-<
> Note: these failed tests are union of mac, linux and win bots.
> 
> 1 fast/dom/blur-contenteditable.html
> 2 fast/events/selectionchange-user-initiated.html
> 3 fast/events/selectstart-prevent-selectall.html
> 4 fast/events/touch/gesture/focus-selectionchange-on-tap.html
> 5 fast/forms/focus-selection-input.html
> 6 fast/forms/focus-selection-textarea.html
> 7 fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html
> 8 fast/layers/scroll-rect-to-visible.html
> 9 fast/spatial-navigation/snav-input.html
> 10 fast/spatial-navigation/snav-textarea.html
> 11 plugins/mouse-click-plugin-clears-selection.html
> 12 plugins/touch-events-details.html
> 13 plugins/touch-events-synthesized.html
> 14 plugins/transformed-events.html
> 15
>
virtual/softwarecompositing/squashing/invalidations-with-large-negative-margin.html

The root cause of above failure is we didn't clear selection for text field and
textarea when they lose focus.
Patch set #5 fixes this and running try bot.

[yosin_UTC9, 2014-08-13 04:14:53]

PTAL

All tests are green now.

[blink-reviews, 2014-08-13 04:16:14]

Awesome!


Jelte Liebrand | Software Engineer | jelte@google.com <jliebrand@google.com>
 | +1 (415) 318 6507


On 12 August 2014 21:14, <yosin@chromium.org> wrote:

> PTAL
>
> All tests are green now.
>
> https://codereview.chromium.org/424973003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[leviw_travelin_and_unemployed, 2014-08-13 18:28:14]

LGTM. Great!

[yosin_UTC9, 2014-08-14 00:58:58]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-08-14 01:00:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yosin@chromium.org/424973003/140001

[commit-bot: I haz the power, 2014-08-14 03:55:20]

Committed patchset #6 (140001) as 180228