New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 678601 link

Starred by 1 user
Status: Fixed
Owner:
█ hu...@opera.com
Email to this user bounced
Closed: Apr 2017
Cc:
yosin@chromium.org
editing-dev@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Redundant TextInputStateChanged when focus jumps <input>

Reported by hu...@opera.com, Jan 5 2017 
Chrome Version: master (don't know when it was introduced)

What steps will reproduce the problem?
(1) out/Release/content_shell -u "data:text/html;charset=utf-8,<input type=text> text<br><input type=number> number"
(2) Click the text-field.
(3) Click the number-field

What is the expected result?
One ViewHostMsg_TextInputStateChanged is sent.

What happens instead?
Three ViewHostMsg_TextInputStateChanged are sent.

This bug can be confirmed by adding extra prints:

[1:1:0105/104055.807397:175946904983:ERROR:render_frame_impl.cc(3999)] RenderFrameImpl::didChangeSelection
[1:1:0105/104055.807636:175946905164:ERROR:render_widget.cc(1065)] RenderWidget::UpdateTextInputState sends new ViewHostMsg_TextInputStateChanged
[12931:12931:0105/104055.808441:175946905874:ERROR:render_widget_host_impl.cc(1980)] RenderWidgetHostImpl::OnTextInputStateChange type: 0
[1:1:0105/104055.814126:175946911710:ERROR:render_frame_impl.cc(3999)] RenderFrameImpl::didChangeSelection
[1:1:0105/104055.814434:175946911959:ERROR:render_widget.cc(1065)] RenderWidget::UpdateTextInputState sends new ViewHostMsg_TextInputStateChanged
[12931:12931:0105/104055.815127:175946912561:ERROR:render_widget_host_impl.cc(1980)] RenderWidgetHostImpl::OnTextInputStateChange type: 5
[12931:12931:0105/104055.815824:175946913257:ERROR:render_widget_host_impl.cc(1471)] RenderWidgetHostImpl::OnSelectionBoundsChanged
[1:1:0105/104055.902154:175946999751:ERROR:render_widget.cc(1065)] RenderWidget::UpdateTextInputState sends new ViewHostMsg_TextInputStateChanged
[12931:12931:0105/104055.903195:175947000629:ERROR:render_widget_host_impl.cc(1980)] RenderWidgetHostImpl::OnTextInputStateChange type: 5

It seems like Chromium does redundant work here. 

Does bug affect a downstream product?
Yes. The first message (type = 0 = TEXT_INPUT_TYPE_NONE) causes a UI glitch in the IME of Opera's Chromium-based SDK.
The IME hides itself on TEXT_INPUT_TYPE_NONE and reopens just after (looks like a quick flash).

Comment 1 by yosin@chromium.org, Jan 10 2017

Blockedon: 679635

Comment 2 by dtapu...@chromium.org, Jan 16 2017

Components: Blink>Editing

Comment 3 by hu...@opera.com, Feb 15 2017

Blockedon: -679635

Comment 4 by hu...@opera.com, Mar 14 2017

Owner: hu...@opera.com
Status: Started (was: Available)
Work in progress: https://codereview.chromium.org/2616623002
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31d55d991baf03aa9bc3ff5d5e67f709eee77959

commit 31d55d991baf03aa9bc3ff5d5e67f709eee77959
Author: hugoh <hugoh@opera.com>
Date: Tue Mar 28 02:09:07 2017

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}

[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/content/test/data/android/input/input_forms.html
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/editing/input/keyboard_event_without_focus.html
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/fast/forms/focus-selection-input-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/android/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/linux/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/linux/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/linux/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac-mac10.10/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac-mac10.9/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/mac/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/win/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/LayoutTests/platform/win/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/core/editing/EditingUtilities.h
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/core/editing/PendingSelection.cpp
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[add] https://crrev.com/31d55d991baf03aa9bc3ff5d5e67f709eee77959/third_party/WebKit/Source/web/tests/data/editable_elements.html
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/914070a0e79c114aad81486aba7a3a15d94f590e

commit 914070a0e79c114aad81486aba7a3a15d94f590e
Author: imcheng <imcheng@chromium.org>
Date: Tue Mar 28 20:29:14 2017

Revert of Do not send redundant selectionchange-events (patchset #28 id:540001 of https://codereview.chromium.org/2616623002/ )

Reason for revert:
Broke webkit_tests: https://bugs.chromium.org/p/chromium/issues/detail?id=706119

Original issue's description:
> 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/+/31d55d991baf03aa9bc3ff5d5e67f709eee77959

TBR=aelias@chromium.org,amaralp@chromium.org,bokan@chromium.org,kochi@chromium.org,yosin@chromium.org,hugoh@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 678601 ,  679635 ,  699015 

Review-Url: https://codereview.chromium.org/2784653002
Cr-Commit-Position: refs/heads/master@{#460205}

[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/content/test/data/android/input/input_forms.html
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/editing/input/keyboard_event_without_focus.html
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/fast/forms/focus-selection-input-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/android/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/linux/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/linux/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/linux/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac-mac10.10/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac-mac10.9/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/mac/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/win/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/LayoutTests/platform/win/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/Source/core/editing/EditingUtilities.h
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/Source/core/editing/PendingSelection.cpp
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/914070a0e79c114aad81486aba7a3a15d94f590e/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[delete] https://crrev.com/d406a54b2a677132a79f9da2c9624bb8c3ab07c8/third_party/WebKit/Source/web/tests/data/editable_elements.html
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92bf3c29fccac551acceff254e53d7ea73367158

commit 92bf3c29fccac551acceff254e53d7ea73367158
Author: hugoh <hugoh@opera.com>
Date: Wed Mar 29 07:18:54 2017

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}

[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/content/test/data/android/input/input_forms.html
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/editing/input/keyboard_event_without_focus.html
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/fast/forms/focus-selection-input-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/android/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/linux/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/linux/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/linux/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac-mac10.10/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac-mac10.9/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/mac/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/win/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/LayoutTests/platform/win/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/core/editing/EditingUtilities.h
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/core/editing/PendingSelection.cpp
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[add] https://crrev.com/92bf3c29fccac551acceff254e53d7ea73367158/third_party/WebKit/Source/web/tests/data/editable_elements.html
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065

commit 1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065
Author: aelias <aelias@chromium.org>
Date: Sat Apr 01 00:00:23 2017

Revert of Reland: Do not send redundant selectionchange-events (patchset #29 id:560001 of https://codereview.chromium.org/2616623002/ )

Reason for revert:
Caused  http://crbug.com/707156 , please reland after fixing it.

BUG= 707156 

Original issue's description:
> 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/+/92bf3c29fccac551acceff254e53d7ea73367158

TBR=amaralp@chromium.org,bokan@chromium.org,kochi@chromium.org,yosin@chromium.org,hugoh@opera.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 678601 ,  679635 ,  699015 

Review-Url: https://codereview.chromium.org/2791753002
Cr-Commit-Position: refs/heads/master@{#461278}

[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/content/test/data/android/input/input_forms.html
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/editing/input/keyboard_event_without_focus.html
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/fast/forms/focus-selection-input-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/android/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/linux/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/linux/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac-mac10.10/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac-mac10.9/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/mac/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/win/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/LayoutTests/platform/win/plugins/mouse-click-plugin-clears-selection-expected.txt
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/Source/core/editing/EditingUtilities.h
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/Source/core/editing/PendingSelection.cpp
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/1fe7a105adbf7d7e58b3fcb38aefe2fd735c6065/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[delete] https://crrev.com/b4c713070f5634b5ea05a677bd0160370c776d7a/third_party/WebKit/Source/web/tests/data/editable_elements.html
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f65d25a4097959d977dbb1077717323438240a6

commit 0f65d25a4097959d977dbb1077717323438240a6
Author: hugoh <hugoh@opera.com>
Date: Fri Apr 14 07:10:29 2017

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}

[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/content/test/data/android/input/input_forms.html
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/editing/input/keyboard_event_without_focus.html
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/editing/selection/focus-and-display-none-and-redisplay.html
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/fast/forms/focus-selection-input-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/fast/forms/focus-selection-textarea-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/android/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/linux/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/LayoutTests/platform/win/fast/forms/textarea/textarea-scrolled-mask-expected.txt
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/core/editing/FrameSelection.h
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/core/editing/LayoutSelection.h
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[add] https://crrev.com/0f65d25a4097959d977dbb1077717323438240a6/third_party/WebKit/Source/web/tests/data/editable_elements.html
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec55bb5f6efdd3b9db80075ccba703064eeded80

commit ec55bb5f6efdd3b9db80075ccba703064eeded80
Author: hugoh <hugoh@opera.com>
Date: Fri Apr 14 10:21:23 2017

Update description of mouse-click-plugin-clears-selection.html

After crrev.com/2616623002, we now do expect the layout tree
to contain a selection (a selection that is hidden). So let's
update the test's inline description to reflect this exception.

BUG= 678601 

Review-Url: https://codereview.chromium.org/2817073003
Cr-Commit-Position: refs/heads/master@{#464710}

[modify] https://crrev.com/ec55bb5f6efdd3b9db80075ccba703064eeded80/third_party/WebKit/LayoutTests/plugins/mouse-click-plugin-clears-selection.html

Comment 12 by hu...@opera.com, Apr 25 2017

Status: Fixed (was: Started)
►

Sign in to add a comment