Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionBlink 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 #Messages
Total messages: 59 (0 generated)
Please take a look. https://codereview.chromium.org/258063005/diff/1/LayoutTests/fast/forms/input... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/1/LayoutTests/fast/forms/input... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:18: log('Slection start: ' + input.selectionStart + ' and end: ' + input.selectionEnd); I checked this layout test on the browser and it was showing correct value of selectionStart ie 3 and selectionEnd ie 5 but when i ran this test on test runner it shows selectionStart and selectionEnd value to be 6. I dont know why test runner is returning such value. Can you please tell why it's happening?
This CL contains https://codereview.chromium.org/258953002/, right? https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputEl... File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputEl... Source/core/html/HTMLInputElement.cpp:514: if (!isEmptyValue()) This looks weird. If we need such check, we should do it in selectionStart(). https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElement.cpp:282: cacheSelection(start, end, direction); What happens if |start| or |end| is larger than value().length()? https://codereview.chromium.org/258063005/diff/1/Source/core/html/forms/TextF... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/forms/TextF... Source/core/html/forms/TextFieldInputType.cpp:164: if (max <= (unsigned)input->selectionStart()) Do not use C-style cast. Probably Making |max| int is better.
https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputEl... File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLInputEl... Source/core/html/HTMLInputElement.cpp:514: if (!isEmptyValue()) On 2014/04/29 23:55:44, tkent wrote: > This looks weird. > If we need such check, we should do it in selectionStart(). Move the check to selectionStart() https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElement.cpp:282: cacheSelection(start, end, direction); On 2014/04/29 23:55:44, tkent wrote: > What happens if |start| or |end| is larger than value().length()? The browser will displays the caret at end of the text when value.length() is less than |start| or |end|. I have added this case in layout test. https://codereview.chromium.org/258063005/diff/1/Source/core/html/forms/TextF... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/1/Source/core/html/forms/TextF... Source/core/html/forms/TextFieldInputType.cpp:164: if (max <= (unsigned)input->selectionStart()) On 2014/04/29 23:55:44, tkent wrote: > Do not use C-style cast. Probably Making |max| int is better. Done.
On 2014/04/29 23:55:44, tkent wrote: > This CL contains https://codereview.chromium.org/258953002/, right? > Yes this CL contains https://codereview.chromium.org/258953002/. It resolves the bug=133242 also (http://code.google.com/p/chromium/issues/detail?id=133242). Should i add in the description that this patch also solves the given bug and add its id there?
On 2014/04/30 14:42:35, harpreet.sk wrote: > Yes this CL contains https://codereview.chromium.org/258953002/. It resolves the > bug=133242 also (http://code.google.com/p/chromium/issues/detail?id=133242). > Should i add in the description that this patch also solves the given bug and > add its id there? Yes please.
I still feel it's weird to adjust selectionStart/End values on getting them. Can we adjust them on setting? https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) Please remove this and use debug() in ../../resource/js-test.js. https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:12: if (window.testRunner) { We prefer early return. if (window.testRunner) { debug('This test needs window.testRunner.'); return; } https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:14: var firstInput = document.getElementById('val1'); Wrong indentation.
https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) I recommend to use "resources/js-test.js". It provides logging, |shouldBe|, |shouldBeEqualToString|, and so on. You don't need to write your logging.
https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) On 2014/05/07 00:54:39, tkent wrote: > Please remove this and use debug() in ../../resource/js-test.js. Done. https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:5: function log(msg) On 2014/05/07 01:06:22, Yoshi wrote: > I recommend to use "resources/js-test.js". It provides logging, |shouldBe|, > |shouldBeEqualToString|, and so on. You don't need to write your logging. Done. https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:12: if (window.testRunner) { On 2014/05/07 00:54:39, tkent wrote: > We prefer early return. > > if (window.testRunner) { > debug('This test needs window.testRunner.'); > return; > } Done. https://codereview.chromium.org/258063005/diff/30001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:14: var firstInput = document.getElementById('val1'); On 2014/05/07 00:54:39, tkent wrote: > Wrong indentation. Corrected.
On 2014/05/07 00:47:38, tkent wrote: > On 2014/04/30 14:42:35, harpreet.sk wrote: > > Yes this CL contains https://codereview.chromium.org/258953002/. It resolves > the > > bug=133242 also (http://code.google.com/p/chromium/issues/detail?id=133242). > > Should i add in the description that this patch also solves the given bug and > > add its id there? > > Yes please. Added the other bug id and description.
On 2014/05/07 00:54:38, tkent wrote: > I still feel it's weird to adjust selectionStart/End values on getting them. > Can we adjust them on setting? > Even i find this way a little weird but i think this is better option to adjust value on getting rather than setting because if we made adjustment while setting we need two more variables to store the selectionStart and selectionEnd from javascript for future reference as cachedSelectionStart and cachedSelectionEnd will store the current selectionStart ( value which will be returned on calling selectionStart()) and current selectionEnd (value which will be returned on calling selectionEnd() ). Since more memory will be required while setting because of 2 new variables that has to be introduced for storing values from javascript so i thought of making adjustment while getting.
PING
On 2014/05/07 12:19:29, harpreet.sk wrote: > On 2014/05/07 00:54:38, tkent wrote: > > I still feel it's weird to adjust selectionStart/End values on getting them. > > Can we adjust them on setting? > > > > Even i find this way a little weird but i think this is better option to adjust > value on getting rather than setting because if we made adjustment while setting > we need two more variables to store the selectionStart and selectionEnd from > javascript for future reference as cachedSelectionStart and cachedSelectionEnd > will store > the current selectionStart ( value which will be returned on calling > selectionStart()) and current selectionEnd (value which will be returned on > calling selectionEnd() ). Why don't you always return cachedSelectionStart/End?
On 2014/05/08 08:00:07, tkent wrote:
> On 2014/05/07 12:19:29, harpreet.sk wrote:
> > On 2014/05/07 00:54:38, tkent wrote:
> > > I still feel it's weird to adjust selectionStart/End values on getting
them.
>
> > > Can we adjust them on setting?
> > >
> >
> > Even i find this way a little weird but i think this is better option to
> adjust
> > value on getting rather than setting because if we made adjustment while
> setting
> > we need two more variables to store the selectionStart and selectionEnd from
> > javascript for future reference as cachedSelectionStart and
cachedSelectionEnd
> > will store
> > the current selectionStart ( value which will be returned on calling
> > selectionStart()) and current selectionEnd (value which will be returned on
> > calling selectionEnd() ).
>
> Why don't you always return cachedSelectionStart/End?
cachedSelectionStart/End in my current patch stores the value from javascript
which might be different from actual selectionStart/End for some cases. Say for
eg
case:
We set selectionStart = 3 and selectionEnd = 5 and input.value = 'p'
In this case if we now call selectionStart() and selectionEnd() it should
return 1 ( firefox also return 1)
so for cases when text length is smaller than selectionStart we have to return
value other than cachedSelectionStart/End
On 2014/05/08 08:21:09, harpreet.sk wrote: > > Why don't you always return cachedSelectionStart/End? > > > cachedSelectionStart/End in my current patch stores the value from javascript > which might be different from actual selectionStart/End for some cases. Say for > eg > > case: > We set selectionStart = 3 and selectionEnd = 5 and input.value = 'p' > In this case if we now call selectionStart() and selectionEnd() it should > return 1 ( firefox also return 1) So, we don't need to keep selectionStart=3 and selectionEnd=5. cachedSelectionStart/End should keep actual values.
On 2014/05/08 08:26:14, tkent wrote:
> On 2014/05/08 08:21:09, harpreet.sk wrote:
> > > Why don't you always return cachedSelectionStart/End?
> >
> >
> > cachedSelectionStart/End in my current patch stores the value from
javascript
> > which might be different from actual selectionStart/End for some cases. Say
> for
> > eg
> >
> > case:
> > We set selectionStart = 3 and selectionEnd = 5 and input.value = 'p'
> > In this case if we now call selectionStart() and selectionEnd() it
should
> > return 1 ( firefox also return 1)
>
> So, we don't need to keep selectionStart=3 and selectionEnd=5.
> cachedSelectionStart/End should keep actual values.
But we had to sotre somewhere selectionStart=3 and selectionEnd=5 (values from
javascript) because it might be required for future reference. Say for eg
Case: We set selectionStart = 3 and selectionEnd = 5 and input.value = 'p'
. Here according to you cachedSelectionStart/End should be 1.
Then the user again set input.value = "Parsed";
Now when we call selectionStart() and selectionEnd() then it should
return 3 and 5 (same behaviour in firefox)
so in that case we require 2 more variables to store values from javascript if
you want to store actual values in cachedSelectionStart/End
On 2014/05/08 08:35:40, harpreet.sk wrote: > Case: We set selectionStart = 3 and selectionEnd = 5 and input.value = 'p' > . Here according to you cachedSelectionStart/End should be 1. > Then the user again set input.value = "Parsed"; > Now when we call selectionStart() and selectionEnd() then it should > return 3 and 5 (same behaviour in firefox) Wow, it's a crazy behavior. How about IE?
On 2014/05/08 08:45:04, tkent wrote: > On 2014/05/08 08:35:40, harpreet.sk wrote: > > Case: We set selectionStart = 3 and selectionEnd = 5 and input.value = > 'p' > > . Here according to you cachedSelectionStart/End should be 1. > > Then the user again set input.value = "Parsed"; > > Now when we call selectionStart() and selectionEnd() then it > should > > return 3 and 5 (same behaviour in firefox) > > Wow, it's a crazy behavior. How about IE? IE is also not respecting selectionStart/End (similar to chrome). IE also have the same bug. For the above case when value is "P" it shows selectionStart/End as 1 and for value="Parsed" it shows selectionStart/End as 6
PING
https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... 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/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:14: testSelection(); nit: It seems we can inline |testSelection()| here. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:36: </head> nit: Please remove this extra "</head>". https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:37: nit: Please remove this extra blank line.
The specification doesn't ask the Firefox behavior. IE's behavior looks reasonable.
On 2014/05/09 08:25:13, tkent wrote: > The specification doesn't ask the Firefox behavior. IE's behavior looks > reasonable. But IE is also not following the selectionStart and selectionEnd from javascript as when we specify it start as 3 and end as 5 and setting text length to be more than selectionEnd it should select the text but instead it displaying caret at end. I think behaviour of firefox is fine as browser should obey the selectionStart and selectionEnd specify by user no matter how many times user change the input.value. When user set start to be 3 and end to be 5 browser in my opinion should show the following behaviour input.value = "P" start and end to be 1 input.value = "Pars" start = 3 and end = 4 input.value = "Parsed" start =3 and end =5 What's your opinino for above behavior?
On 2014/05/09 08:42:19, harpreet.sk wrote: > I think behaviour of firefox is fine as browser should obey the selectionStart > and selectionEnd specify by user no matter how many times user change the > input.value. I don't agree with it. > When user set start to be 3 and end to be 5 browser in my opinion should show > the following behaviour > > input.value = "P" start and end to be 1 > input.value = "Pars" start = 3 and end = 4 > input.value = "Parsed" start =3 and end =5 > > What's your opinino for above behavior? In this case, I expect as follows: input.value = "P" start and end to be 1 input.value = "Pars" start and end are still 1 input.value = "Parsed" Ditto.
New patch set follows the behavior as mention in previous comment. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: On 2014/05/09 07:12:21, Yosi OOF til May 12 wrote: > nit: Please remove this extra blank line. Done. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:14: testSelection(); On 2014/05/09 07:12:21, Yosi OOF til May 12 wrote: > nit: It seems we can inline |testSelection()| here. Done. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:36: </head> On 2014/05/09 07:12:21, Yosi OOF til May 12 wrote: > nit: Please remove this extra "</head>". Done. https://codereview.chromium.org/258063005/diff/50001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:37: On 2014/05/09 07:12:21, Yosi OOF til May 12 wrote: > nit: Please remove this extra blank line. Done.
https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: description("<b>Test for BUG=367736: Blink does not respect input.selectionStart and input.selectionEnd for some cases.</b>"); <b> tags are unnecessary. Also, double-quotes are used here though other quotes are single. https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:22: shouldBe('firstInput.selectionStart', '6'); Why is it 6? firstInput doesn't have focus, so selectionStart should be 3. https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:26: } We should have another scenario that secondInput.value='Parsed' and confirming selectionStart and selectionEnd after that. https://codereview.chromium.org/258063005/diff/70001/Source/core/html/HTMLTex... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/70001/Source/core/html/HTMLTex... Source/core/html/HTMLTextFormControlElement.cpp:343: if (document().focusedElement() != this) { We should not need to adjust the value on getting. https://codereview.chromium.org/258063005/diff/70001/Source/core/html/forms/T... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/70001/Source/core/html/forms/T... Source/core/html/forms/TextFieldInputType.cpp:166: if (max < input->selectionStart()) This |if| statement should be input->setSelectionRange(input->selectionStart(), input->selectionEnd()); and setSelectionRange should adjust the values.
https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:10: description("<b>Test for BUG=367736: Blink does not respect input.selectionStart and input.selectionEnd for some cases.</b>"); On 2014/05/19 00:00:34, tkent wrote: > <b> tags are unnecessary. > Also, double-quotes are used here though other quotes are single. Done. https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:22: shouldBe('firstInput.selectionStart', '6'); On 2014/05/19 00:00:34, tkent wrote: > Why is it 6? firstInput doesn't have focus, so selectionStart should be 3. I checked this case on the browser and it was showing correct value of selectionStart as 3 and selectionEnd as 5 but when i ran this test on test runner it shows selectionStart and selectionEnd value to be 6. I dont know why test runner is returning such value. Can you please tell why it's happening? https://codereview.chromium.org/258063005/diff/70001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:26: } On 2014/05/19 00:00:34, tkent wrote: > We should have another scenario that secondInput.value='Parsed' and confirming > selectionStart and selectionEnd after that. Done. https://codereview.chromium.org/258063005/diff/70001/Source/core/html/HTMLTex... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/70001/Source/core/html/HTMLTex... Source/core/html/HTMLTextFormControlElement.cpp:343: if (document().focusedElement() != this) { On 2014/05/19 00:00:34, tkent wrote: > We should not need to adjust the value on getting. Only for given case we need to adjust value while getting: We set input.selectionStart = 3 and input.selectionEnd = 5; input.value is empty currently. Now if we call selectionStart()/selectionEnd() it should return start/End as 0 ( all browsers return 0 ) but cache selection should still store 3 and 5 as start/End for future. Now if we set value of input as input.value = "Parsed" it should show selectionStart/End as 3 and 5. https://codereview.chromium.org/258063005/diff/70001/Source/core/html/forms/T... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/70001/Source/core/html/forms/T... Source/core/html/forms/TextFieldInputType.cpp:166: if (max < input->selectionStart()) On 2014/05/19 00:00:34, tkent wrote: > This |if| statement should be > input->setSelectionRange(input->selectionStart(), input->selectionEnd()); > and setSelectionRange should adjust the values. Done. https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:19: shouldBe('firstInput.selectionStart', '6'); I checked this case on the browser and it was showing correct value of selectionStart as 3 and selectionEnd as 5 but when i ran this test on test runner it shows selectionStart and selectionEnd value to be 6. I dont know why test runner is returning such value. Can you please tell why it's happening? https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:27: shouldBe('secondInput.selectionStart', '6'); Ditto. I think test runner always returns selectionStart/End equal to the length of input.value
https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... 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/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:18: firstInput.value = "Parsed"; nit: double-quotes though other quotes are single. https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:19: shouldBe('firstInput.selectionStart', '6'); On 2014/05/19 11:16:52, harpreet.sk wrote: > I checked this case on the browser and it was showing correct value of > selectionStart as 3 and selectionEnd as 5 but when i ran this test on test > runner it shows selectionStart and selectionEnd value to be 6. I dont know why > test runner is returning such value. Can you please tell why it's happening? I don't know. Please investigate by yourself. https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:23: secondInput.value = "P"; nit: double-quotes https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:26: secondInput.value = "Parsed"; nit: double-quotes
https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... File LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html (right): https://codereview.chromium.org/258063005/diff/90001/LayoutTests/fast/forms/i... LayoutTests/fast/forms/input-set-selectionStart-set-selectionEnd.html:19: shouldBe('firstInput.selectionStart', '6'); On 2014/05/20 09:51:40, tkent wrote: > On 2014/05/19 11:16:52, harpreet.sk wrote: > > I checked this case on the browser and it was showing correct value of > > selectionStart as 3 and selectionEnd as 5 but when i ran this test on test > > runner it shows selectionStart and selectionEnd value to be 6. I dont know why > > test runner is returning such value. Can you please tell why it's happening? > > I don't know. Please investigate by yourself. I found that whenver we give test file as an input it focused the input field first. That's the reason why test runner is showing such values. Consider the following source code: <!DOCTYPE html> <html> <head> </head> <body> <input id="val1" type="text" /> <script type="text/javascript"> var input = document.getElementById('val1'); input.selectionStart = 3; </script> </body> </html> If you simply load the following source code you will only see unfocused input field. But if you open the page by giving file name as input (out/Release/chrome file.html) or run it with test runner it focused the field. Even if you simply first open page and then refresh it it will focus the input field. Is it a bug that test runner behaves differently than browser for given case?
On 2014/05/23 10:46:23, harpreet.sk wrote:
> I found that whenver we give test file as an input it focused the input field
> first. That's the reason why test runner is showing such values. Consider the
> following source code:
>
> <!DOCTYPE html>
> <html>
> <head>
> </head>
> <body>
> <input id="val1" type="text" />
> <script type="text/javascript">
> var input = document.getElementById('val1');
> input.selectionStart = 3;
> </script>
> </body>
> </html>
>
> If you simply load the following source code you will only see unfocused input
> field.
> But if you open the page by giving file name as input (out/Release/chrome
> file.html) or run it with test runner it focused the field.
> Even if you simply first open page and then refresh it it will focus the input
> field. Is it a bug that test runner behaves differently than browser for given
> case?
This behavior sounds a bug of content shell. You may call input.blur() in your
test to avoid this behavior.
On 2014/05/27 00:26:41, tkent wrote:
> On 2014/05/23 10:46:23, harpreet.sk wrote:
> > I found that whenver we give test file as an input it focused the input
field
> > first. That's the reason why test runner is showing such values. Consider
the
> > following source code:
> >
> > <!DOCTYPE html>
> > <html>
> > <head>
> > </head>
> > <body>
> > <input id="val1" type="text" />
> > <script type="text/javascript">
> > var input = document.getElementById('val1');
> > input.selectionStart = 3;
> > </script>
> > </body>
> > </html>
> >
> > If you simply load the following source code you will only see unfocused
input
> > field.
> > But if you open the page by giving file name as input (out/Release/chrome
> > file.html) or run it with test runner it focused the field.
> > Even if you simply first open page and then refresh it it will focus the
input
> > field. Is it a bug that test runner behaves differently than browser for
given
> > case?
>
> This behavior sounds a bug of content shell. You may call input.blur() in
your
> test to avoid this behavior.
The input field gets focused whenever directly or indirectly
HTMLTextFormControlElement::setSelectionRange() is called because this api calls
FrameSelection::setSelection() which then calls setFocusNodeIfNeeded().
setFocusNodeIfNeeded() then checks if FrameSelection is focused or not and based
on it, it focus the node. So for my test to pass i need to add input.blur()
after each line that's call setSelectionRange() indirectly (like after setting
selectionStart, selectionEnd).
Also for existing layout tests whoose behavior will change because of my patch
then for them also we need to add blur() everywhere. Instead of adding blur()
everywhere i think we can report this bug and wait for it to be fixed.
On 2014/05/27 15:41:56, harpreet.sk wrote: > The input field gets focused whenever directly or indirectly > HTMLTextFormControlElement::setSelectionRange() is called because this api calls > FrameSelection::setSelection() which then calls setFocusNodeIfNeeded(). > setFocusNodeIfNeeded() then checks if FrameSelection is focused or not and based > on it, it focus the node. So for my test to pass i need to add input.blur() > after each line that's call setSelectionRange() indirectly (like after setting > selectionStart, selectionEnd). > > Also for existing layout tests whoose behavior will change because of my patch > then for them also we need to add blur() everywhere. Instead of adding blur() > everywhere i think we can report this bug and wait for it to be fixed. Oh, I understand the situation. So, we should not call FrameSelection::setSelection if the field has no focus, and should update m_cachedSelection*.
I mean we should not add blur() to tests.
On 2014/05/27 23:13:44, tkent wrote: > On 2014/05/27 15:41:56, harpreet.sk wrote: > > The input field gets focused whenever directly or indirectly > > HTMLTextFormControlElement::setSelectionRange() is called because this api > calls > > FrameSelection::setSelection() which then calls setFocusNodeIfNeeded(). > > setFocusNodeIfNeeded() then checks if FrameSelection is focused or not and > based > > on it, it focus the node. So for my test to pass i need to add input.blur() > > after each line that's call setSelectionRange() indirectly (like after setting > > selectionStart, selectionEnd). > > > > Also for existing layout tests whoose behavior will change because of my patch > > then for them also we need to add blur() everywhere. Instead of adding blur() > > everywhere i think we can report this bug and wait for it to be fixed. > > Oh, I understand the situation. > So, we should not call FrameSelection::setSelection if the field has no focus, > and should update m_cachedSelection*. Yes it's right not to call FrameSelection::setSelection if field is not focus but it will fail for the case when input has value with length >= selectionStart and selectionStart ! selectionEnd Case selectionStart = 3 selectionEnd = 5 and input.value = 'Parsed' Here input is not focused and we want the text to be selected ( since here selection is not caret ) Even if we add a check something like this (document().focusedElement() == this) || (!selectionIsCaret && textLength >= start) will solve this problem but will fail for different case Case: selectionStart = 3 selectionEnd = 5 and input.value = 'Parsed' input.value = "Barsed" First time when input.value is set FrameSelection::setSelection is called as per above condtion and then it focuses the input element . Now when input.value is set second time it had a check in TextFieldInputType::setValue of input field being focused or not and if focused change the cached selection with the value of text length. So for given cached it will show selectionStart/End as 6 (but ideally it should show 3 and 5) So should we report this bug and wait for it to be fixed.
It sounds like the current Blink or your patch has a bug. You should fix the bug if you want to proceed this CL.
Uploaded new patch set. Please have a look.
https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html:13: var startX = document.getElementById("i1").offsetLeft + 30; This looks depending on a specific font, and looks not portable. document.getElementById('i1').focus(); document.getElementById('i1').setSelectionRange(4, 4); Does this work? https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/paste-into-textarea.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/paste-into-textarea.html:7: var startX = e.offsetLeft + 10; Ditto. https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/textarea-arrow-navigation.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/textarea-arrow-navigation.html:26: var startX = textarea.offsetLeft + 30; Ditto. https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/validationMessage-expected.txt (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/validationMessage-expected.txt:31: input tooLong: This looks wrong. https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:299: bool shouldSetSelection = (document().focusedElement() == this) || (!isCaretSelection && start < textLength); Why is !isCaretSelection necessary? https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:356: if (!innerTextValue().length()) As I wrote again and again, adjusting value on selectionStart/End getters would be wrong. Please fix the bug without updating selectionStart() and selectionEnd, and without adding selection*ForJavaScriptBindings().
https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:293: } else if (textLength >= start && textLength < end) { nit: The "textLength >= start && " part seems unnecessary? (you are in the else case of (textLength < start) so textLength has to be greater or equal to start here). https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:387: if (!innerTextValue().length()) if (innerTextValue().isEmpty())
Uploaded new patch. Please have a look... https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/input-placeholder-visibility-2-expected.html:13: var startX = document.getElementById("i1").offsetLeft + 30; On 2014/06/01 23:40:55, tkent wrote: > This looks depending on a specific font, and looks not portable. > > document.getElementById('i1').focus(); > document.getElementById('i1').setSelectionRange(4, 4); > > Does this work? Yes it will work. I had made the changes in new patch. https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/paste-into-textarea.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/paste-into-textarea.html:7: var startX = e.offsetLeft + 10; On 2014/06/01 23:40:55, tkent wrote: > Ditto. Done. https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/textarea-arrow-navigation.html (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/textarea-arrow-navigation.html:26: var startX = textarea.offsetLeft + 30; On 2014/06/01 23:40:55, tkent wrote: > Ditto. Done. https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/validationMessage-expected.txt (right): https://codereview.chromium.org/258063005/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/validationMessage-expected.txt:31: input tooLong: On 2014/06/01 23:40:55, tkent wrote: > This looks wrong. It is not wrong because for the given case caret was at the start of text when delete command is called so nothing got deleted. So variable m_lastChangeWasUserEdit remains false as no event occur. Now when the api HTMLInputElement::tooLong is called in this api they mention in a comment that "return false for the default value or a value set by a script even if it is longer than maxLength." bool HTMLInputElement::tooLong(const String& value, NeedsToCheckDirtyFlag check) const { // We use isTextType() instead of supportsMaxLength() because of the // 'virtual' overhead. if (!isTextType()) return false; int max = maxLength(); if (max < 0) return false; if (check == CheckDirtyFlag) { // Return false for the default value or a value set by a script even if // it is longer than maxLength. if (!hasDirtyValue() || !lastChangeWasUserEdit()) return false; } return value.length() > static_cast<unsigned>(max); } That is the reason that the validation message is empty for this case. https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:293: } else if (textLength >= start && textLength < end) { On 2014/06/02 15:32:45, OOO until July 9 wrote: > nit: The "textLength >= start && " part seems unnecessary? (you are in the else > case of (textLength < start) so textLength has to be greater or equal to start > here). Done. https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:299: bool shouldSetSelection = (document().focusedElement() == this) || (!isCaretSelection && start < textLength); On 2014/06/01 23:40:55, tkent wrote: > Why is !isCaretSelection necessary? This check is necessary because when input is not focused we only shows selection when it is not caret selection. The caret is not displayed if input is not focused. Same behaviour by firefox and IE. https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:356: if (!innerTextValue().length()) On 2014/06/01 23:40:55, tkent wrote: > As I wrote again and again, adjusting value on selectionStart/End getters would > be wrong. > Please fix the bug without updating selectionStart() and selectionEnd, and > without adding selection*ForJavaScriptBindings(). We require a check in selectionStart() and selectionEnd() for the case when innerEditorValue() is empty because in only that case we would always return value 0. Consider case below: input.selectionStart = 3; input.selectEnd = 5; // Right now we have not set value. So when we check selectionStart/End it shoudlreturn 0 // Now set value input.value = "Parsed"; // Now selectionStart/End to be 3 and 5 I had removed the api selection*ForJavaScriptBindings() and try to adjust the value in setSelectionStart/End. https://codereview.chromium.org/258063005/diff/110001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:387: if (!innerTextValue().length()) On 2014/06/02 15:32:45, OOO until July 9 wrote: > if (innerTextValue().isEmpty()) Done. https://codereview.chromium.org/258063005/diff/130001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/130001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:358: if (!isTextFormControl() || innerEditorValue().isEmpty()) We require check in selectionStart() and selectionEnd() for the case when innerEditorValue() is empty because in only that case we would always return value 0. Consider case below: input.selectionStart = 3; input.selectEnd = 5; // Right now we haven't set value of input element. So when we check selectionStart/End it shoudlreturn 0 but m_cachedSelectionStart/End values here should be 3/5 // Now set value input.value = "Parsed"; // Now selectionStart/End should return 3 and 5
Please try to keep cached selection is up-to-date. When JS set value, we set 0,0 to cached selection. I think m_cachedSelection is incorrect variable name. It should represent real value of selection in text form control. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:166: if (!innerEditorValue().isEmpty()) It is better to call setSelectionRange(start, selectionEnd(), selectionDirection()) then adjust start and end in setSelectionRange. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:169: setSelectionRange(start, std::max(start, m_cachedSelectionEnd), selectionDirection()); Since, control has empty string, we can set 0, 0 to selection. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:177: setSelectionRange(std::min(end, m_cachedSelectionStart), end, selectionDirection()); Since, control has empty string, we can set 0, 0 to selection. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:291: end = std::max(end, 0); How about int textLength = innerEditorValue.length(); end = std::max(std::min(end, length), 0); start = std::max(std::min(start, end), 0); https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:303: bool isCaretSelection = (start == end); nit: we don't need to have surrounding parenthesis. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:306: if (!hasVisibleTextArea(renderer(), innerEditorElement())) It seems we should set |FrameSelection| if text area has focus. But this isn't related to this patch. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:329: if (frame && shouldSetSelection) Please move this if-statement just after L306 if (!hasVisibleTextArea(...). When we don't have |LocalFrame| and |!shouldSelection| we don't need to construct |VisibleSelection|. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:358: if (!isTextFormControl() || innerEditorValue().isEmpty()) We should reset |m_cachedSelectionStart| when value is changed. Rather than check whether value is empty or not. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:377: if (!isTextFormControl() || innerEditorValue().isEmpty()) We should reset |m_cachedSelectionEnd| when value is changed. Rather than check whether value is empty or not. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:496: if (document().focusedElement() == this) We should keep cached selection up-to date. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/forms/... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/150001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:169: input->setSelectionRange(input->selectionStart(), input->selectionEnd()); Why do we need to set same value to input field selection? It seems we should to same thing for TEXTAREA.
On 2014/07/03 01:58:36, Yosi_UTC9 wrote:
> Please try to keep cached selection is up-to-date. When JS set value, we set
0,0
> to cached selection.
>
> I think m_cachedSelection is incorrect variable name. It should represent real
> value of selection in text form control.
>
@Yosi: The reason why we cannot set the cachedSelection to 0,0 is because
consider the following case:
<input type="text"/>
<script>
input.selectionStart = 3;
input.selectionEnd = 5;
log('Slection now is start ' + input.selectionStart + ' and end is ' +
input.selectionEnd);
// Right now inputEditorValue is empty and if we check selectionStart and
selectionEnd value it should return 0,0. Here we cannot set
cachedSelectionStart/End to 0,0 because it will be storing the value 3 and 5
which is set by JS and which can be used in future if we set the value using
javascript.
// Now let's say we set input value
input.value = "Parsed"
log('Slection now is start ' + input.selectionStart + ' and end is ' +
input.selectionEnd);
// Now it should return selectionStart/End to be 3, 5 and not 0, 0 as now value
is set. See crbug.com/367736 test case. Firefox also show same behaviour
</script>
As for the above case i added this innerEditorValue.isEmpty() check in the
selectionStart()/End() api so that it should not return cached selection
start/end.
cannot
Please give your comments about it. We need to somewhere store
selectionStart/End from JS for future refernce so cannot update
m_cachedSelectionStart/End for this case to 0,0.
@yosi thanks for the review. I will upload the new patch with your suggestion but before that i would like to have your opinion about my previous comment.
On 2014/07/03 05:23:05, harpreet.sk wrote:
> On 2014/07/03 01:58:36, Yosi_UTC9 wrote:
> > Please try to keep cached selection is up-to-date. When JS set value, we set
> 0,0
> > to cached selection.
> >
> > I think m_cachedSelection is incorrect variable name. It should represent
real
> > value of selection in text form control.
> >
>
> @Yosi: The reason why we cannot set the cachedSelection to 0,0 is because
> consider the following case:
>
> <input type="text"/>
>
> <script>
>
> input.selectionStart = 3;
> input.selectionEnd = 5;
>
> log('Slection now is start ' + input.selectionStart + ' and end is ' +
> input.selectionEnd);
>
> // Right now inputEditorValue is empty and if we check selectionStart and
> selectionEnd value it should return 0,0. Here we cannot set
> cachedSelectionStart/End to 0,0 because it will be storing the value 3 and 5
> which is set by JS and which can be used in future if we set the value using
> javascript.
>
> // Now let's say we set input value
>
> input.value = "Parsed"
>
> log('Slection now is start ' + input.selectionStart + ' and end is ' +
> input.selectionEnd);
>
> // Now it should return selectionStart/End to be 3, 5 and not 0, 0 as now
value
> is set. See crbug.com/367736 test case. Firefox also show same behaviour
>
> </script>
As I already wrote, we should not follow this strange behavior. The
specification doesn't ask it.
On 2014/07/04 02:20:23, tkent wrote:
> On 2014/07/03 05:23:05, harpreet.sk wrote:
> > On 2014/07/03 01:58:36, Yosi_UTC9 wrote:
> > > Please try to keep cached selection is up-to-date. When JS set value, we
set
> > 0,0
> > > to cached selection.
> > >
> > > I think m_cachedSelection is incorrect variable name. It should represent
> real
> > > value of selection in text form control.
> > >
> >
> > @Yosi: The reason why we cannot set the cachedSelection to 0,0 is because
> > consider the following case:
> >
> > <input type="text"/>
> >
> > <script>
> >
> > input.selectionStart = 3;
> > input.selectionEnd = 5;
> >
> > log('Slection now is start ' + input.selectionStart + ' and end is ' +
> > input.selectionEnd);
> >
> > // Right now inputEditorValue is empty and if we check selectionStart and
> > selectionEnd value it should return 0,0. Here we cannot set
> > cachedSelectionStart/End to 0,0 because it will be storing the value 3 and 5
> > which is set by JS and which can be used in future if we set the value using
> > javascript.
> >
> > // Now let's say we set input value
> >
> > input.value = "Parsed"
> >
> > log('Slection now is start ' + input.selectionStart + ' and end is ' +
> > input.selectionEnd);
> >
> > // Now it should return selectionStart/End to be 3, 5 and not 0, 0 as now
> value
> > is set. See crbug.com/367736 test case. Firefox also show same behaviour
> >
> > </script>
>
> As I already wrote, we should not follow this strange behavior. The
> specification doesn't ask it.
So you mean to say for the both cases above we should return selectionStart/End
as 3,5 ( even if input field is empty)
On 2014/07/04 03:24:02, harpreet.sk wrote: > > As I already wrote, we should not follow this strange behavior. The > > specification doesn't ask it. > > > > So you mean to say for the both cases above we should return selectionStart/End > as 3,5 ( even if input field is empty) No. Both should be 0,0.
On 2014/07/04 03:39:15, tkent wrote: > On 2014/07/04 03:24:02, harpreet.sk wrote: > > > As I already wrote, we should not follow this strange behavior. The > > > specification doesn't ask it. > > > > > > > > So you mean to say for the both cases above we should return > selectionStart/End > > as 3,5 ( even if input field is empty) > > No. Both should be 0,0. But as per bug 367736 and what we have decided earlier you told that for following test case input.selectionStart = 3 input.selectionEnd = 5 input.value = "Parsed" it should show selection from 3, 5. This is what i added in my layout test and you agree with the expected value earlier. Now u want it to be 0, 0.
Uploaded new patch... please have a look https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:166: if (!innerEditorValue().isEmpty()) On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > It is better to call setSelectionRange(start, selectionEnd(), > selectionDirection()) then adjust start and end in setSelectionRange. > > > Adjusting values in setSelectionRange https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:169: setSelectionRange(start, std::max(start, m_cachedSelectionEnd), selectionDirection()); On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > Since, control has empty string, we can set 0, 0 to selection. This condition is removed from here and adjusting values in setSelectionRange https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:177: setSelectionRange(std::min(end, m_cachedSelectionStart), end, selectionDirection()); On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > Since, control has empty string, we can set 0, 0 to selection. ditto https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:291: end = std::max(end, 0); On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > How about > int textLength = innerEditorValue.length(); > end = std::max(std::min(end, length), 0); > start = std::max(std::min(start, end), 0); thanks for the suggestion... incorporated in new patch. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:303: bool isCaretSelection = (start == end); On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > nit: we don't need to have surrounding parenthesis. Done. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:306: if (!hasVisibleTextArea(renderer(), innerEditorElement())) On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > It seems we should set |FrameSelection| if text area has focus. But this isn't > related to this patch. this check was already there ... i just moved the calling of cacheSelection from the inside of the check.. this check is not part of this bug https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:329: if (frame && shouldSetSelection) On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > Please move this if-statement just after L306 if (!hasVisibleTextArea(...). > When we don't have |LocalFrame| and |!shouldSelection| we don't need to > construct |VisibleSelection|. Done. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:358: if (!isTextFormControl() || innerEditorValue().isEmpty()) On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > We should reset |m_cachedSelectionStart| when value is changed. Rather than > check whether value is empty or not. Done. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:377: if (!isTextFormControl() || innerEditorValue().isEmpty()) On 2014/07/03 01:58:35, Yosi_UTC9 wrote: > We should reset |m_cachedSelectionEnd| when value is changed. Rather than check > whether value is empty or not. Done. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:496: if (document().focusedElement() == this) On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > We should keep cached selection up-to date. The cached selection is getting up-to-date in setSelectionRange. https://codereview.chromium.org/258063005/diff/150001/Source/core/html/forms/... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/150001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:169: input->setSelectionRange(input->selectionStart(), input->selectionEnd()); On 2014/07/03 01:58:36, Yosi_UTC9 wrote: > Why do we need to set same value to input field selection? > > It seems we should to same thing for TEXTAREA. In case if input field is focused and selection is caret and we are setting value then caret should so move to the end of text
> you agree with the expected value earlier. Now u want it to be 0, 0. I have never agreed with it. https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/input-setvalue-selection.html (right): https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... LayoutTests/fast/forms/input-setvalue-selection.html:15: document.write(input.selectionStart == 0 ? "1. SUCCESS" : "1. FAILURE (the insertion point should be at the end)"); We should update the comment for the FAILURE. at the end -> at the beginning? https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/validationMessage-expected.txt (right): https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... LayoutTests/fast/forms/validationMessage-expected.txt:31: input tooLong: We expect this test shows a validation message. We need to call inputWithMax.setSelectionRange(6, 6) before document.execCommand("delete"). https://codereview.chromium.org/258063005/diff/170001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (left): https://codereview.chromium.org/258063005/diff/170001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:345: nit: This blank line change is unnecessary. https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:163: if (input->focused() && input->selectionStart() == input->selectionEnd()) Why is |input->selectionStart() == input->selectionEnd()| necessary? https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:167: nit: This blank line change is unnecessary.
Made the changes ... please have a look https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/input-setvalue-selection.html (right): https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... LayoutTests/fast/forms/input-setvalue-selection.html:15: document.write(input.selectionStart == 0 ? "1. SUCCESS" : "1. FAILURE (the insertion point should be at the end)"); On 2014/07/08 00:34:48, tkent wrote: > We should update the comment for the FAILURE. at the end -> at the beginning? > Done. https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/validationMessage-expected.txt (right): https://codereview.chromium.org/258063005/diff/170001/LayoutTests/fast/forms/... LayoutTests/fast/forms/validationMessage-expected.txt:31: input tooLong: On 2014/07/08 00:34:48, tkent wrote: > We expect this test shows a validation message. We need to call > inputWithMax.setSelectionRange(6, 6) before document.execCommand("delete"). Done. https://codereview.chromium.org/258063005/diff/170001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (left): https://codereview.chromium.org/258063005/diff/170001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:345: On 2014/07/08 00:34:48, tkent wrote: > nit: This blank line change is unnecessary. Done. https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:163: if (input->focused() && input->selectionStart() == input->selectionEnd()) On 2014/07/08 00:34:48, tkent wrote: > Why is |input->selectionStart() == input->selectionEnd()| necessary? This was the part of old change but it is not required now, hence removing this check. https://codereview.chromium.org/258063005/diff/170001/Source/core/html/forms/... Source/core/html/forms/TextFieldInputType.cpp:167: On 2014/07/08 00:34:48, tkent wrote: > nit: This blank line change is unnecessary. Done.
lgtm finally
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/258063005/190001
Message was sent while issue was closed.
Change committed as 177651
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/374883004/ by peter@chromium.org. The reason for reverting is: Suspected of breaking browser_tests on various platforms: ClearPreviewWithAutofilledUsernameAndPassword ClearPreviewWithUsernameAutofilled PreviewSuggestionSelectionRange [PasswordAutofillAgentTest.ClearPreviewWithUsernameAutofilled] Value of: username_element_.selectionStart() Actual: 0 (expected: |start|, which is: 3) Logs are available here: http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%2... .
Message was sent while issue was closed.
@tkent: i have uploaded patch for the fix of browser_tests that were getting failed because of this patch. (https://codereview.chromium.org/373023004/) Can you please tell what is the procedure to land both these patches together?
Reopening issue again because of the revert. The browser test that were getting failed because of this CL has been fixed and is landed in the following revision https://src.chromium.org/viewvc/chrome?view=rev&revision=281975
The CQ bit was checked by harpreet.sk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/258063005/190001
Message was sent while issue was closed.
Change committed as 177724
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 |
