Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(380)

Issue 258063005: Blink does not respect input.selectionStart and input.selectionEnd for some cases (Closed)

Created:
6 years, 7 months ago by harpreet.sk
Modified:
6 years, 5 months ago
CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org, ojan
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Blink does not respect input.selectionStart and input.selectionEnd for some cases Blink does not respect the input.selectionStart and input.selectionEnd. When input.selectionStart and input.selectionEnd is set and then value is set the blink always shows the selection with caret at end of text. This patch removes this bug by caching the selectionStart and selectionEnd in m_cachedSelectionStart and m_cachedSelectionEnd and checking the cached selection while setting input value. This patch also removes the bug specifying that caret poistion is incompatible after updating value IDL attribute of unfocused INPUT element. When the value IDL attribute is updated of unfocused INPUT element the caret moves at the end but other browser move the caret at the beginning of the value. The other browsers move the caret to the end of the value only if the INPUT is focused. This patch removes this bug too. BUG=367736, 133242 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177651 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177724

Patch Set 1 #

Total comments: 7

Patch Set 2 : Addressing changes asked in patch set 1 #

Patch Set 3 : Added some changes in patch set 2 #

Total comments: 8

Patch Set 4 : Addressing the changes asked for LayoutTests in patch set 3 #

Total comments: 8

Patch Set 5 : Addressing the expected behavior and changes asked for LayoutTests #

Total comments: 10

Patch Set 6 : Addressing changes asked in patch set 5 #

Total comments: 8

Patch Set 7 : New patch fixing existing bug #

Total comments: 16

Patch Set 8 : Addressing changes asked in previous patch #

Total comments: 1

Patch Set 9 : Rebase #

Total comments: 22

Patch Set 10 : Addressing changes asked #

Total comments: 10

Patch Set 11 : Addressing changes asked in previous patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -17 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/accessibility/textarea-insertion-point-line-number.html View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/accessibility/textarea-insertion-point-line-number-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/editing/deleting/5290534.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/editing/inserting/4960120-1.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/editing/pasteboard/paste-removing-iframe.html View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/editing/pasteboard/pasting-tabs.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/editing/selection/5497643.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html View 1 2 3 4 5 6 7 8 9 1 chunk +33 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +17 lines, -0 lines 0 comments Download
M LayoutTests/fast/forms/input-setvalue-selection.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/paste-into-textarea.html View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/textarea-arrow-navigation.html View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/validationMessage.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLTextFormControlElement.cpp View 1 2 3 4 5 6 7 8 9 10 3 chunks +16 lines, -9 lines 0 comments Download
M Source/core/html/forms/TextFieldInputType.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 59 (0 generated)
harpreet.sk
Please take a look. https://codereview.chromium.org/258063005/diff/1/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/1/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode18 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:18: log('Slection start: ' + input.selectionStart ...
6 years, 7 months ago (2014-04-28 16:09:04 UTC) #1
tkent
This CL contains https://codereview.chromium.org/258953002/, right? https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputElement.cpp#newcode514 Source/core/html/HTMLInputElement.cpp:514: if (!isEmptyValue()) This looks ...
6 years, 7 months ago (2014-04-29 23:55:44 UTC) #2
harpreet.sk
https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputElement.cpp#newcode514 Source/core/html/HTMLInputElement.cpp:514: if (!isEmptyValue()) On 2014/04/29 23:55:44, tkent wrote: > This ...
6 years, 7 months ago (2014-04-30 14:36:26 UTC) #3
harpreet.sk
On 2014/04/29 23:55:44, tkent wrote: > This CL contains https://codereview.chromium.org/258953002/, right? > Yes this CL ...
6 years, 7 months ago (2014-04-30 14:42:35 UTC) #4
harpreet.sk
6 years, 7 months ago (2014-05-02 13:21:41 UTC) #5
tkent
On 2014/04/30 14:42:35, harpreet.sk wrote: > Yes this CL contains https://codereview.chromium.org/258953002/. It resolves the > ...
6 years, 7 months ago (2014-05-07 00:47:38 UTC) #6
tkent
I still feel it's weird to adjust selectionStart/End values on getting them. Can we adjust ...
6 years, 7 months ago (2014-05-07 00:54:38 UTC) #7
yosin_UTC9
https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode5 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) I recommend to use "resources/js-test.js". It provides ...
6 years, 7 months ago (2014-05-07 01:06:22 UTC) #8
harpreet.sk
https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode5 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) On 2014/05/07 00:54:39, tkent wrote: > Please ...
6 years, 7 months ago (2014-05-07 11:57:54 UTC) #9
harpreet.sk
On 2014/05/07 00:47:38, tkent wrote: > On 2014/04/30 14:42:35, harpreet.sk wrote: > > Yes this ...
6 years, 7 months ago (2014-05-07 11:58:55 UTC) #10
harpreet.sk
On 2014/05/07 00:54:38, tkent wrote: > I still feel it's weird to adjust selectionStart/End values ...
6 years, 7 months ago (2014-05-07 12:19:29 UTC) #11
harpreet.sk
PING
6 years, 7 months ago (2014-05-08 03:28:09 UTC) #12
tkent
On 2014/05/07 12:19:29, harpreet.sk wrote: > On 2014/05/07 00:54:38, tkent wrote: > > I still ...
6 years, 7 months ago (2014-05-08 08:00:07 UTC) #13
harpreet.sk
On 2014/05/08 08:00:07, tkent wrote: > On 2014/05/07 12:19:29, harpreet.sk wrote: > > On 2014/05/07 ...
6 years, 7 months ago (2014-05-08 08:21:09 UTC) #14
tkent
On 2014/05/08 08:21:09, harpreet.sk wrote: > > Why don't you always return cachedSelectionStart/End? > > ...
6 years, 7 months ago (2014-05-08 08:26:14 UTC) #15
harpreet.sk
On 2014/05/08 08:26:14, tkent wrote: > On 2014/05/08 08:21:09, harpreet.sk wrote: > > > Why ...
6 years, 7 months ago (2014-05-08 08:35:40 UTC) #16
tkent
On 2014/05/08 08:35:40, harpreet.sk wrote: > Case: We set selectionStart = 3 and selectionEnd = ...
6 years, 7 months ago (2014-05-08 08:45:04 UTC) #17
harpreet.sk
On 2014/05/08 08:45:04, tkent wrote: > On 2014/05/08 08:35:40, harpreet.sk wrote: > > Case: We ...
6 years, 7 months ago (2014-05-08 08:52:02 UTC) #18
harpreet.sk
PING
6 years, 7 months ago (2014-05-09 06:26:48 UTC) #19
yosin_UTC9
https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode10 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: nit: Please remove this extra blank line. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode14 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:14: ...
6 years, 7 months ago (2014-05-09 07:12:20 UTC) #20
tkent
The specification doesn't ask the Firefox behavior. IE's behavior looks reasonable.
6 years, 7 months ago (2014-05-09 08:25:13 UTC) #21
harpreet.sk
On 2014/05/09 08:25:13, tkent wrote: > The specification doesn't ask the Firefox behavior. IE's behavior ...
6 years, 7 months ago (2014-05-09 08:42:19 UTC) #22
tkent
On 2014/05/09 08:42:19, harpreet.sk wrote: > I think behaviour of firefox is fine as browser ...
6 years, 7 months ago (2014-05-13 10:51:10 UTC) #23
harpreet.sk
New patch set follows the behavior as mention in previous comment. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): ...
6 years, 7 months ago (2014-05-14 10:24:09 UTC) #24
tkent
https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode10 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: description("<b>Test for BUG=367736: Blink does not respect input.selectionStart and ...
6 years, 7 months ago (2014-05-19 00:00:34 UTC) #25
harpreet.sk
https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode10 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: description("<b>Test for BUG=367736: Blink does not respect input.selectionStart and ...
6 years, 7 months ago (2014-05-19 11:16:52 UTC) #26
tkent
https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode13 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:13: if (!window.testRunner) { This test doesn't need testRunner. https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode18 ...
6 years, 7 months ago (2014-05-20 09:51:40 UTC) #27
harpreet.sk
https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html#newcode19 LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:19: shouldBe('firstInput.selectionStart', '6'); On 2014/05/20 09:51:40, tkent wrote: > On ...
6 years, 7 months ago (2014-05-23 10:46:23 UTC) #28
tkent
On 2014/05/23 10:46:23, harpreet.sk wrote: > I found that whenver we give test file as ...
6 years, 7 months ago (2014-05-27 00:26:41 UTC) #29
harpreet.sk
On 2014/05/27 00:26:41, tkent wrote: > On 2014/05/23 10:46:23, harpreet.sk wrote: > > I found ...
6 years, 7 months ago (2014-05-27 15:41:56 UTC) #30
tkent
On 2014/05/27 15:41:56, harpreet.sk wrote: > The input field gets focused whenever directly or indirectly ...
6 years, 7 months ago (2014-05-27 23:13:44 UTC) #31
tkent
I mean we should not add blur() to tests.
6 years, 7 months ago (2014-05-27 23:14:17 UTC) #32
harpreet.sk
On 2014/05/27 23:13:44, tkent wrote: > On 2014/05/27 15:41:56, harpreet.sk wrote: > > The input ...
6 years, 6 months ago (2014-05-28 13:33:06 UTC) #33
tkent
It sounds like the current Blink or your patch has a bug. You should fix ...
6 years, 6 months ago (2014-05-29 05:31:07 UTC) #34
harpreet.sk
Uploaded new patch set. Please have a look.
6 years, 6 months ago (2014-05-30 12:04:08 UTC) #35
tkent
https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html File LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html#newcode13 LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html:13: var startX = document.getElementById("i1").offsetLeft + 30; This looks depending ...
6 years, 6 months ago (2014-06-01 23:40:55 UTC) #36
Inactive
https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTextFormControlElement.cpp#newcode293 Source/core/html/HTMLTextFormControlElement.cpp:293: } else if (textLength >= start && textLength < ...
6 years, 6 months ago (2014-06-02 15:32:44 UTC) #37
harpreet.sk
Uploaded new patch. Please have a look... https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html File LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html#newcode13 LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html:13: var startX ...
6 years, 5 months ago (2014-07-02 10:13:05 UTC) #38
yosin_UTC9
Please try to keep cached selection is up-to-date. When JS set value, we set 0,0 ...
6 years, 5 months ago (2014-07-03 01:58:36 UTC) #39
harpreet.sk
On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > Please try to keep cached selection is up-to-date. When ...
6 years, 5 months ago (2014-07-03 05:23:05 UTC) #40
harpreet.sk
@yosi thanks for the review. I will upload the new patch with your suggestion but ...
6 years, 5 months ago (2014-07-03 05:26:42 UTC) #41
tkent
On 2014/07/03 05:23:05, harpreet.sk wrote: > On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > > Please try ...
6 years, 5 months ago (2014-07-04 02:20:23 UTC) #42
harpreet.sk
On 2014/07/04 02:20:23, tkent wrote: > On 2014/07/03 05:23:05, harpreet.sk wrote: > > On 2014/07/03 ...
6 years, 5 months ago (2014-07-04 03:24:02 UTC) #43
tkent
On 2014/07/04 03:24:02, harpreet.sk wrote: > > As I already wrote, we should not follow ...
6 years, 5 months ago (2014-07-04 03:39:15 UTC) #44
harpreet.sk
On 2014/07/04 03:39:15, tkent wrote: > On 2014/07/04 03:24:02, harpreet.sk wrote: > > > As ...
6 years, 5 months ago (2014-07-04 03:58:18 UTC) #45
harpreet.sk
Uploaded new patch... please have a look https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTextFormControlElement.cpp#newcode166 Source/core/html/HTMLTextFormControlElement.cpp:166: if (!innerEditorValue().isEmpty()) ...
6 years, 5 months ago (2014-07-04 13:50:27 UTC) #46
tkent
> you agree with the expected value earlier. Now u want it to be 0, ...
6 years, 5 months ago (2014-07-08 00:34:48 UTC) #47
harpreet.sk
Made the changes ... please have a look https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/input-setvalue-selection.html File LayoutTests/fast/forms/input-setvalue-selection.html (right): https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/input-setvalue-selection.html#newcode15 LayoutTests/fast/forms/input-setvalue-selection.html:15: document.write(input.selectionStart ...
6 years, 5 months ago (2014-07-08 06:07:59 UTC) #48
tkent
lgtm finally
6 years, 5 months ago (2014-07-08 07:58:18 UTC) #49
tkent
The CQ bit was checked by tkent@chromium.org
6 years, 5 months ago (2014-07-08 07:58:24 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/258063005/190001
6 years, 5 months ago (2014-07-08 07:59:07 UTC) #51
commit-bot: I haz the power
Change committed as 177651
6 years, 5 months ago (2014-07-08 08:47:32 UTC) #52
Peter Beverloo
A revert of this CL has been created in https://codereview.chromium.org/374883004/ by peter@chromium.org. The reason for ...
6 years, 5 months ago (2014-07-08 13:17:21 UTC) #53
harpreet.sk
@tkent: i have uploaded patch for the fix of browser_tests that were getting failed because ...
6 years, 5 months ago (2014-07-08 15:41:43 UTC) #54
harpreet.sk
Reopening issue again because of the revert. The browser test that were getting failed because ...
6 years, 5 months ago (2014-07-09 07:16:05 UTC) #55
harpreet.sk
The CQ bit was checked by harpreet.sk@samsung.com
6 years, 5 months ago (2014-07-09 07:16:23 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/258063005/190001
6 years, 5 months ago (2014-07-09 07:16:43 UTC) #57
commit-bot: I haz the power
Change committed as 177724
6 years, 5 months ago (2014-07-09 07:17:49 UTC) #58
tkent
6 years, 5 months ago (2014-07-17 07:31:55 UTC) #59
Message was sent while issue was closed.
On 2014/07/09 07:17:49, I haz the power (commit-bot) wrote:
> Change committed as 177724

It was reverted by https://codereview.chromium.org/404483002

Powered by Google App Engine
This is Rietveld 408576698