Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(46)

Issue 392573002: HTMLTextAreaElement.setSelectionRange should not change focus. (Closed)

Created:
6 years ago by yoichio
Modified:
5 years, 11 months ago
Reviewers:
tkent, yosin_UTC9
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

HTMLTextAreaElement.setSelectionRange should not change focus. HTMLTextAreaElement.setSelectionRange(start,end) changes focus if |start| is not |end|. However, HTMLTextAreaElement.setSelectionRange should not change focus in any condition. FireFox follows such behavior at least. Source/core/html/HTMLTextFormControlElement.cpp/h: - Let setSelectionRange not change focus without the ChangeFocus option. - SelectionOption option is used to change text selection like select() calling on disabled input field. - 278L: We must set 'forward' direction if direction is 'none' on not Mac. - FrameSelection.setSelection is called when selectionOption is ChangeSelection or this TextFormControl element is already focused. Source/core/html/HTMLTextAreaElement.cpp/h: - Change setValueCommon to call setSelectionRange except for initialize. - setSelectionRange calls update layout, which needs updated validity so call setNeedsValidityCheck earlier. LayoutTests/fast/forms/selection-setSelectionRange-focusing.html: - Added test. Check focus not to be changed. LayoutTests/fast/forms/textarea-selection-preservation.html: - Add test cases resetting selection. This is following the FireFox behavior. (Other layout tests): - Some tests expect setSelectionRange to change focus, thus I add extra focus. - Some tests expect resetting value of textarea to change selection to 0, thus I add extra setSelectionRange(0, 0); TEST=LayoutTests/fast/forms/selection-setSelectionRange-focusing.html, textarea- selection-preservation.html BUG=393504 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178822

Patch Set 1 : rename #

Total comments: 11

Patch Set 2 : Rename FocusOption #

Total comments: 2

Patch Set 3 : Change focusing #

Patch Set 4 : Rebase #

Patch Set 5 : Fixed failed tests #

Total comments: 4

Patch Set 6 : Update #

Patch Set 7 : Call setSelectionRange inside setValueCommon #

Total comments: 3

Patch Set 8 : Update validity before updatelayout #

Total comments: 6

Patch Set 9 : Set selection to end from setNonDirtyValue #

Total comments: 1

Patch Set 10 : Fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -26 lines) Patch
M LayoutTests/accessibility/textarea-insertion-point-line-number.html View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/accessibility/textarea-insertion-point-line-number-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/editing/input/reveal-caret-of-multiline-input.html View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/editing/selection/shrink-selection-after-shift-pagedown.html View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/input-appearance-selection.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/fast/forms/selection-setSelectionRange-focusing.html View 1 chunk +26 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/selection-setSelectionRange-focusing-expected.txt View 1 chunk +14 lines, -0 lines 0 comments Download
M LayoutTests/fast/forms/textarea-selection-preservation.html View 1 2 3 4 5 6 7 8 2 chunks +33 lines, -2 lines 0 comments Download
M LayoutTests/fast/forms/textarea-selection-preservation-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +19 lines, -0 lines 0 comments Download
M LayoutTests/fast/spatial-navigation/snav-textarea.html View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M LayoutTests/fast/text/international/mixed-directionality-selection.html View 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/html/HTMLTextAreaElement.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/HTMLTextAreaElement.cpp View 1 2 3 4 5 6 7 8 3 chunks +20 lines, -11 lines 0 comments Download
M Source/core/html/HTMLTextFormControlElement.h View 1 2 3 4 5 6 2 chunks +6 lines, -1 line 0 comments Download
M Source/core/html/HTMLTextFormControlElement.cpp View 1 2 3 4 5 6 7 8 6 chunks +24 lines, -7 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
yoichio
6 years ago (2014-07-16 06:53:21 UTC) #1
yosin_UTC9
https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextAreaElement.h File Source/core/html/HTMLTextAreaElement.h (right): https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextAreaElement.h#newcode79 Source/core/html/HTMLTextAreaElement.h:79: bool setValueCommon(const String&, TextFieldEventBehavior); nit: Could you add comment ...
6 years ago (2014-07-16 07:09:15 UTC) #2
tkent
https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextFormControlElement.cpp#newcode184 Source/core/html/HTMLTextFormControlElement.cpp:184: setSelectionRange(0, std::numeric_limits<int>::max(), SelectionHasNoDirection, MustFocus); Is this compatible with other ...
6 years ago (2014-07-16 07:11:34 UTC) #3
yoichio
https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextAreaElement.h File Source/core/html/HTMLTextAreaElement.h (right): https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextAreaElement.h#newcode79 Source/core/html/HTMLTextAreaElement.h:79: bool setValueCommon(const String&, TextFieldEventBehavior); On 2014/07/16 07:09:14, Yosi_UTC9 wrote: ...
6 years ago (2014-07-16 08:07:12 UTC) #4
tkent
https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTextFormControlElement.cpp#newcode184 Source/core/html/HTMLTextFormControlElement.cpp:184: setSelectionRange(0, std::numeric_limits<int>::max(), SelectionHasNoDirection, MustFocus); On 2014/07/16 08:07:12, yoichio wrote: ...
6 years ago (2014-07-16 23:34:16 UTC) #5
tkent
https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTextFormControlElement.cpp#newcode289 Source/core/html/HTMLTextFormControlElement.cpp:289: return; Is this compatible with other browsers? Shouldn't we ...
6 years ago (2014-07-16 23:35:19 UTC) #6
yoichio
Change setSelectionRange not to change focus. However we still should manage text selection via FrameSelection, ...
6 years ago (2014-07-17 07:16:01 UTC) #7
yoichio
Ping?
5 years, 12 months ago (2014-07-18 05:52:07 UTC) #8
yosin_UTC9
LGTM
5 years, 12 months ago (2014-07-18 06:52:20 UTC) #9
tkent
> Source/core/html/HTMLTextAreaElement.cpp/h: > - Change the setSelectionRange calling to call from only setValue function, not ...
5 years, 12 months ago (2014-07-18 07:24:16 UTC) #10
tkent
https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTextFormControlElement.cpp#newcode188 Source/core/html/HTMLTextFormControlElement.cpp:188: if (isFocusable()) Doesn't focus() work? https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTextFormControlElement.cpp#newcode189 Source/core/html/HTMLTextFormControlElement.cpp:189: document().page()->focusController().setFocusedElement(this, document().frame()); ...
5 years, 12 months ago (2014-07-18 07:24:24 UTC) #11
yoichio
On 2014/07/18 07:24:16, tkent wrote: > > Source/core/html/HTMLTextAreaElement.cpp/h: > > - Change the setSelectionRange calling ...
5 years, 12 months ago (2014-07-18 09:07:37 UTC) #12
yoichio
https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTextFormControlElement.cpp#newcode188 Source/core/html/HTMLTextFormControlElement.cpp:188: if (isFocusable()) On 2014/07/18 07:24:24, tkent wrote: > Doesn't ...
5 years, 12 months ago (2014-07-18 09:07:48 UTC) #13
yoichio
Ping.
5 years, 12 months ago (2014-07-22 01:08:14 UTC) #14
tkent
On 2014/07/18 09:07:37, yoichio wrote: > On 2014/07/18 07:24:16, tkent wrote: > > > Source/core/html/HTMLTextAreaElement.cpp/h: ...
5 years, 12 months ago (2014-07-22 01:51:54 UTC) #15
yoichio
On 2014/07/22 01:51:54, tkent wrote: > On 2014/07/18 09:07:37, yoichio wrote: > > On 2014/07/18 ...
5 years, 12 months ago (2014-07-22 02:16:06 UTC) #16
tkent
On 2014/07/22 02:16:06, yoichio wrote: > > textarae.blur(); > > textarea.value = "abcd"; > > ...
5 years, 12 months ago (2014-07-22 02:22:44 UTC) #17
tkent
On 2014/07/22 02:22:44, tkent wrote: > On 2014/07/22 02:16:06, yoichio wrote: > > > textarae.blur(); ...
5 years, 12 months ago (2014-07-22 02:58:04 UTC) #18
yoichio
On 2014/07/22 02:22:44, tkent wrote: > On 2014/07/22 02:16:06, yoichio wrote: > > > textarae.blur(); ...
5 years, 12 months ago (2014-07-22 03:01:28 UTC) #19
tkent
On 2014/07/22 03:01:28, yoichio wrote: > > Does this compatible with other browsers? Does this ...
5 years, 12 months ago (2014-07-22 03:36:55 UTC) #20
yoichio
On 2014/07/22 03:36:55, tkent wrote: > On 2014/07/22 03:01:28, yoichio wrote: > > > Does ...
5 years, 12 months ago (2014-07-22 04:06:06 UTC) #21
yoichio
I know that this CL still leave some un-standard behaviors. However I'm working for a ...
5 years, 12 months ago (2014-07-22 08:34:14 UTC) #22
tkent
On 2014/07/22 08:34:14, yoichio wrote: > I know that this CL still leave some un-standard ...
5 years, 12 months ago (2014-07-22 09:50:08 UTC) #23
yoichio
On 2014/07/22 09:50:08, tkent wrote: > On 2014/07/22 08:34:14, yoichio wrote: > > I know ...
5 years, 12 months ago (2014-07-22 10:15:31 UTC) #24
tkent
On 2014/07/22 10:15:31, yoichio wrote: > On 2014/07/22 09:50:08, tkent wrote: > > On 2014/07/22 ...
5 years, 12 months ago (2014-07-22 11:04:12 UTC) #25
yoichio
Use the (renderer() && valid()) statement instead of (document().focusedElement() == this) to call setSelectionRange regardless ...
5 years, 11 months ago (2014-07-23 02:16:13 UTC) #26
tkent
https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp#newcode374 Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) { I don't understand this ...
5 years, 11 months ago (2014-07-23 02:25:24 UTC) #27
yoichio
In Firefox: selectionStart/End are initialized 0 even if a textarea has a default value. All ...
5 years, 11 months ago (2014-07-23 10:38:58 UTC) #28
yoichio
https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp#newcode374 Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) { On 2014/07/23 02:25:24, tkent ...
5 years, 11 months ago (2014-07-23 10:39:07 UTC) #29
tkent
On 2014/07/23 10:38:58, yoichio wrote: > In Firefox: > selectionStart/End are initialized 0 even if ...
5 years, 11 months ago (2014-07-24 00:58:27 UTC) #30
tkent
https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTextAreaElement.cpp#newcode374 Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) { On 2014/07/23 10:39:07, yoichio ...
5 years, 11 months ago (2014-07-24 01:04:22 UTC) #31
yoichio
https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTextAreaElement.cpp File Source/core/html/HTMLTextAreaElement.cpp (right): https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTextAreaElement.cpp#newcode224 Source/core/html/HTMLTextAreaElement.cpp:224: setSelectionRange(endOfString, endOfString, SelectionHasNoDirection, NotChangeSelection); On 2014/07/24 01:04:22, tkent wrote: ...
5 years, 11 months ago (2014-07-24 03:06:07 UTC) #32
tkent
Looks good, but some tests are failing.
5 years, 11 months ago (2014-07-24 04:31:27 UTC) #33
yoichio
On 2014/07/24 04:31:27, tkent wrote: > Looks good, but some tests are failing. Fixed. virtual/softwarecompositing/repaint/invalidations-on-composited-layers.html ...
5 years, 11 months ago (2014-07-24 06:34:48 UTC) #34
tkent
lgtm
5 years, 11 months ago (2014-07-24 06:36:38 UTC) #35
yoichio
The CQ bit was checked by yoichio@chromium.org
5 years, 11 months ago (2014-07-24 06:37:18 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/392573002/340001
5 years, 11 months ago (2014-07-24 06:38:22 UTC) #37
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink ...
5 years, 11 months ago (2014-07-24 07:23:52 UTC) #38
commit-bot: I haz the power
5 years, 11 months ago (2014-07-24 08:06:37 UTC) #39
Message was sent while issue was closed.
Change committed as 178822

Powered by Google App Engine
This is Rietveld 408576698