focus() behaviour differs depending on how value is set

input.focus() should not change selection.

The api focus() shows different behaviour depending on how the value
    is set. If the value is set using the html "value" attribute and
    focus() is called then text get selected whereas if value is set using
    javascript value IDL attribute and then focus() is called caret appear at end
    of text in the field. All other browser shows same behaviour for both cases by
    displaying caret in input field. This patch removes this bug by initializing
    m_cachedSelection{Start,End} with 0. This way for HTML value attribute case
    the caret will be displayed at the start of input field. Since m_cachedSelection{Start,End}
    has been initialize with 0 so hasCachedSelection() api will always return true and hence
    is removed.

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

[harpreet.sk, 2014-04-02 11:03:09]

Please take a look

[tkent, 2014-04-03 05:00:40]

Are you sure other browsers set caret on focus, not on updating value?

[harpreet.sk, 2014-04-03 05:33:25]

On 2014/04/03 05:00:40, tkent wrote:
> Are you sure other browsers set caret on focus, not on updating value?


I have checked the behaviour in FireFox and IE  if we focus by calling the api
focus() from javascript (ie when FocusType == FocusTypeNone) in that case they
are setting caret , for other cases when focus is done say by pressing TAB key
in that case they will select the text. So this patch also make the chromium
behave in the same manner.

[tkent, 2014-04-03 06:01:00]

> > Are you sure other browsers set caret on focus, not on updating value?
> 
> 
> I have checked the behaviour in FireFox and IE  if we focus by calling the api
> focus() from javascript (ie when FocusType == FocusTypeNone) in that case they
> are setting caret , for other cases when focus is done say by pressing TAB key
> in that case they will select the text. So this patch also make the chromium
> behave in the same manner.

I didn't ask it.  I suspect focus() should not change selection at all.
e.g.

input.value = 'abc';
input.setSelectionRange(1, 1);
input.focus();

This should show caret between 'a' and 'b', right?

[harpreet.sk, 2014-04-03 06:21:11]

On 2014/04/03 06:01:00, tkent wrote:
> > > Are you sure other browsers set caret on focus, not on updating value?
> > 
> > 
> > I have checked the behaviour in FireFox and IE  if we focus by calling the
api
> > focus() from javascript (ie when FocusType == FocusTypeNone) in that case
they
> > are setting caret , for other cases when focus is done say by pressing TAB
key
> > in that case they will select the text. So this patch also make the chromium
> > behave in the same manner.
> 
> I didn't ask it.  I suspect focus() should not change selection at all.
> e.g.
> 
> input.value = 'abc';
> input.setSelectionRange(1, 1);
> input.focus();
> 
> This should show caret between 'a' and 'b', right?


Yes with my patch it's working fine. It will focus caret between 'a' and 'b'.
The focus() will change the selection only when "restorePreviousSelection" will
be false or when there is no cached Selection. Since by default
"restorePreviousSelection" is set to true so if there is cached Selection it
will restore the cached selection ( hence not changing selection at all ).

[tkent, 2014-04-03 06:27:25]

On 2014/04/03 06:21:11, harpreet.sk wrote:
> Yes with my patch it's working fine. It will focus caret between 'a' and 'b'.
> The focus() will change the selection only when "restorePreviousSelection"
will
> be false or when there is no cached Selection. Since by default
> "restorePreviousSelection" is set to true so if there is cached Selection it
> will restore the cached selection ( hence not changing selection at all ).

So, the problem is that there are no cached selection when the value is set by
value attribute.
Would you investigate input.selectionStart and input.selectionEnd values just
after parsing <input value=foo> on IE, Firefox, and Chrome please?

[harpreet.sk, 2014-04-03 08:58:22]

On 2014/04/03 06:27:25, tkent wrote:
> On 2014/04/03 06:21:11, harpreet.sk wrote:
> > Yes with my patch it's working fine. It will focus caret between 'a' and
'b'.
> > The focus() will change the selection only when "restorePreviousSelection"
> will
> > be false or when there is no cached Selection. Since by default
> > "restorePreviousSelection" is set to true so if there is cached Selection it
> > will restore the cached selection ( hence not changing selection at all ).
> 
> So, the problem is that there are no cached selection when the value is set by
> value attribute.
> Would you investigate input.selectionStart and input.selectionEnd values just
> after parsing <input value=foo> on IE, Firefox, and Chrome please?




I have check the following cases and found the following value for
selectionStart and selectionEnd 


Case 1:  <input id="val1" type="text" value="Foo" />
         input.focus();

        Before focus is called:   

        Browser                             selectionStart                     
selectionEnd

        FireFox                                0                                
  0

        IE                                     undefined                        
  undefined

        chrome                                 0                                
  0

        With my patch                          0                                
  0


        After focus is called

        Browser                             selectionStart                     
selectionEnd

        FireFox                                0                                
  0

        IE                                     undefined                        
  undefined

        chrome                                 0                                
  3

        With my patch                          3                                
  3

[harpreet.sk, 2014-04-03 09:07:02]

The previous comment does not come correctly so putting it again.


I have check the following case and found the following value for selectionStart
and selectionEnd 


case   <input id="val1" type="text" value="Foo" />
       input.focus()


Before focus() is called: 

FireFox:
selection start = 0  selection end = 0

IE
selection start = undefined selection end = undefined

chrome
selection start = 0 selection end = 0

With my patch
selection start = 0 selection end = 0


After focus() is called

FireFox: 
selection start = 0  selection end = 0

IE
selection start = undefined selection end = undefined

chrome
selection start = 0 selection end = 3

With my patch
selection start = 3 selection end = 3

[tkent, 2014-04-03 09:07:43]

Your patch is not compatible with Firefox.  IMO, we should not change selection
at all by focus().
Did you test the standard mode IE?

[harpreet.sk, 2014-04-03 09:59:23]

On 2014/04/03 09:07:43, tkent wrote:
> Your patch is not compatible with Firefox.  IMO, we should not change
selection
> at all by focus().
> Did you test the standard mode IE

Sorry i didn't check in standard mode in IE . I checked it now  it also showing
selection start 0 and selection end 0 for both cases.

Earlier i thought that since according to spec the caret should be display at
end of text so i made it at end but since it is not compatible with Firefox so i
will upload a new patch soon with the behavior similar to Firefox and IE .

[tkent, 2014-04-04 01:09:31]

On 2014/04/03 09:59:23, harpreet.sk wrote:
> Earlier i thought that since according to spec the caret should be display at
> end of text

What part of the specification defines it?

[harpreet.sk, 2014-04-04 03:03:12]

On 2014/04/04 01:09:31, tkent wrote:
> On 2014/04/03 09:59:23, harpreet.sk wrote:
> > Earlier i thought that since according to spec the caret should be display
at
> > end of text
> 
> What part of the specification defines it?


Actually i misread a little. Actually it was mention here
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-elem...

They have mention that
"On setting value, it must set the element's value to the new value, set the
element's dirty value flag to true, invoke the value sanitization algorithm, if
the element's type attribute's current state defines one, and then, if the
element has a text entry cursor position, should move the text entry cursor
position to the end of the text field, unselecting any selected text and
resetting the selection direction to none."


Actually for the case

<input id="val1" type="text"/>
input.value = "Bar";
input.focus();

the input.fcous() finish executing earlier before value is set and make the
caret point at the start. Then when the value is set the caret points at the
last. Actually we need to change the flow such that input.focus() finish
executing after value is set. That way the caret will point to the start
(behavior similar to other browser). I'm still working on it.

[tkent, 2014-04-04 03:17:22]

On 2014/04/04 03:03:12, harpreet.sk wrote:
> Actually i misread a little. Actually it was mention here
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-elem...
> 
> They have mention that
> "On setting value, it must set the element's value to the new value, set the
> element's dirty value flag to true, invoke the value sanitization algorithm,
if
> the element's type attribute's current state defines one, and then, if the
> element has a text entry cursor position, should move the text entry cursor
> position to the end of the text field, unselecting any selected text and
> resetting the selection direction to none."

Thanks.  This part mentions setting 'value' IDL attribute. It seems we correctly
handle it.

What we should fix is selection handling of 'value' HTML attribute.  The
paragraph of the specification is not applied, and browsers agree that
selectionStart=selectionEnd=0.  A simple fix would be to initialize
m_cachedSelection{Start,End} with 0.

[harpreet.sk, 2014-04-04 03:57:20]

On 2014/04/04 03:17:22, tkent wrote:

> Thanks.  This part mentions setting 'value' IDL attribute. It seems we
correctly
> handle it.
> 
> What we should fix is selection handling of 'value' HTML attribute.  The
> paragraph of the specification is not applied, and browsers agree that
> selectionStart=selectionEnd=0.  A simple fix would be to initialize
> m_cachedSelection{Start,End} with 0.

Although i agree with you but this will not fix the bug as for both cases ( case
1 setting 'value' HTML attribute and case 2 setting 'value' IDL attribute) the
behavior of focus() will be different. Case 1 will point caret at start and case
2 at end. That's the reason i thought that in order to have same behavior i will
make the caret position at last for case 1. What your opinion about that?

[tkent, 2014-04-04 05:16:37]

On 2014/04/04 03:57:20, harpreet.sk wrote:
> Although i agree with you but this will not fix the bug as for both cases (
case
> 1 setting 'value' HTML attribute and case 2 setting 'value' IDL attribute) the
> behavior of focus() will be different. Case 1 will point caret at start and
case
> 2 at end. That's the reason i thought that in order to have same behavior i
will
> make the caret position at last for case 1. What your opinion about that?

The resultant selection difference exists in IE and Firefox too, right? And it's
not due to focus() behavior, but due to the initial selection before focus().

If focus() changed no selection in Blink, focus() would be compatible with IE
and Firefox.  It's nice, and we should not make focus() incompatible with other
browsers.

[harpreet.sk, 2014-04-04 05:34:40]

On 2014/04/04 05:16:37, tkent wrote:
 
> The resultant selection difference exists in IE and Firefox too, right? And
it's
> not due to focus() behavior, but due to the initial selection before focus().
> 
> If focus() changed no selection in Blink, focus() would be compatible with IE
> and Firefox.  It's nice, and we should not make focus() incompatible with
other
> browsers.


As per the bug 133242,  IE and FireFox are showing the same behavior that is for
both cases (which i mention earlier) the caret points at start (focus() does not
change the selection) but for chrome with what change you are suggesting (ie
setting of cachedSelection()) it will work fine for case 1 but for case 2 it
will not work as focus()  execution complete before value is set which is not
correct. It should complete after value is set. Only then for case 2 also it
will point the caret at start and behavior will be same. 

The specification text clearly states that "  <<  if the
element has a text entry cursor position  >> , should move the text entry cursor
position to the end of the text field, unselecting any selected text and
resetting the selection direction to none."     

Since initially for case 2 the cursor does not points inside the field so
ideally first value should be set then focus() should be called and then the
cursor should point at start. That's what other browser do.

[harpreet.sk, 2014-04-07 07:26:10]

As per the bug 133242,  IE and FireFox are showing the same behavior that is for
both cases (which i mention earlier) the caret points at start (focus() does not
change the selection) but for chrome with what change you are suggesting (ie
setting of cachedSelection()) it will work fine for case 1 but for case 2 it
will not work as focus()  execution complete before value is set which is not
correct. It should complete after value is set. Only then for case 2 also it
will point the caret at start and behavior will be same. 

The specification text clearly states that "  <<  if the
element has a text entry cursor position  >> , should move the text entry cursor
position to the end of the text field, unselecting any selected text and
resetting the selection direction to none."     

Since initially for case 2 the cursor does not points inside the field so
ideally first value should be set then focus() should be called and then the
cursor should point at start. That's what other browser do.

[tkent, 2014-04-08 05:57:46]

On 2014/04/07 07:26:10, harpreet.sk wrote:
> As per the bug 133242,  IE and FireFox are showing the same behavior that is
for
> both cases (which i mention earlier) the caret points at start (focus() does
not
> change the selection)

By 'both cases', do you mean the HTML value case and JavaScript value case?
If so, the bug tells nothing about caret position and it mentions only
range-selection-or-not.

In both cases, the behavior of IE and Firefox is simple.  They don't change
caret/selection at all. So the resultant caret positions look different, and it
is not a problem.

> The specification text clearly states that "  <<  if the
> element has a text entry cursor position  >> , should move the text entry
cursor
> position to the end of the text field, unselecting any selected text and
> resetting the selection direction to none."     

input[type=text] always has "text entry cursor position", which are represented
by selectionStart and selectionEnd.

[tkent, 2014-04-08 05:59:46]

IMO, a codereview should discuss about the code change.  We had better discuss
the expected behavior on the bug.

[harpreet.sk, 2014-04-08 15:33:58]

On 2014/04/08 05:57:46, tkent wrote:

> In both cases, the behavior of IE and Firefox is simple.  They don't change
> caret/selection at all. So the resultant caret positions look different, and
it
> is not a problem.


Since position of caret is not a problem for the cases ( HTML value case and
JavaScript value case ) i have uploaded a new patch with the initialization of
m_cachedSelection{Start,End} with 0. By this we will have caret at start for
HTML value case and at end for JavaScript value case.

[tkent, 2014-04-09 00:16:47]

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:964: crbug.com/133242 [ Android Lion Mac
SnowLeopard Win ] fast/forms/input-double-click-selection-gap-bug.html [
NeedsManualRebaseline ]
'Mac' covers Lion and SnowLeopard.  Anyway, you should remove these [] for
platforms to include Linux.  The result files in this patch might be different
from our bots expectation.

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1091: # Way to Underline position code changed,
need to rebase files after https://codereview.chromium.org/182923003/ .
Unrelated to this CL.

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/fast/forms/i...
File LayoutTests/fast/forms/input-double-click-selection-gap-bug.html (right):

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/fast/forms/i...
LayoutTests/fast/forms/input-double-click-selection-gap-bug.html:10: <p>The top
table was resized while the caret postion
The test name contains 'double-click-selection'.  I guess we should select the
INPUT content.

https://codereview.chromium.org/222023002/diff/20001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/222023002/diff/20001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:57: , m_cachedSelectionEnd(0)
hasCachedSelection() never returns false.  We should remove it, and should add
assertions to make sure m_cachedSelection* can't be -1.

[tkent, 2014-04-09 00:18:17]

Please update the CL description.

[harpreet.sk, 2014-04-09 14:19:42]

Patch set 3 is uploaded with changes asked in comments in patch set 2 and CL
description is updated.

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:964: crbug.com/133242 [ Android Lion Mac
SnowLeopard Win ] fast/forms/input-double-click-selection-gap-bug.html [
NeedsManualRebaseline ]
On 2014/04/09 00:16:48, tkent wrote:
> 'Mac' covers Lion and SnowLeopard.  Anyway, you should remove these [] for
> platforms to include Linux.  The result files in this patch might be different
> from our bots expectation.


[] has been removed for all files except 
"fast/forms/number/number-appearance-spinbutton-disabled-readonly.html" as it
has been added earlier in this file as
 
crbug.com/353736 [ Linux ]
fast/forms/number/number-appearance-spinbutton-disabled-readonly.html [ Pass
ImageOnlyFailure ]

and if we do rebaseline for linux then in git cl presubmit it shows an error
that rebaseline overides the previous line. Hence i also added the expected
results for this layout test.

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1091: # Way to Underline position code changed,
need to rebase files after https://codereview.chromium.org/182923003/ .
On 2014/04/09 00:16:48, tkent wrote:
> Unrelated to this CL.


Removed.

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/fast/forms/i...
File LayoutTests/fast/forms/input-double-click-selection-gap-bug.html (right):

https://codereview.chromium.org/222023002/diff/20001/LayoutTests/fast/forms/i...
LayoutTests/fast/forms/input-double-click-selection-gap-bug.html:10: <p>The top
table was resized while the caret postion
On 2014/04/09 00:16:48, tkent wrote:
> The test name contains 'double-click-selection'.  I guess we should select the
> INPUT content.
> 

In the given test value set in the input field (ie "foo bar") is not a
continuous word and if we double click the input field it should partially
selects the word (either foo or either bar) but here it selecting the whole
text. I doubt whether the LayoutTest really is for double click selection or for
something else.

In new patch set i had replaced the focus() with select() so that behavior of
this test remains same (ie selection of whole text in input field).

https://codereview.chromium.org/222023002/diff/20001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/222023002/diff/20001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:57: , m_cachedSelectionEnd(0)
On 2014/04/09 00:16:48, tkent wrote:
> hasCachedSelection() never returns false.  We should remove it, and should add
> assertions to make sure m_cachedSelection* can't be -1.


hasCachedSelection() is removed and assertions is added where the value of
m_cachedSelection is set.

[tkent, 2014-04-10 01:37:38]

lgtm.

Please rebase.

https://codereview.chromium.org/222023002/diff/40001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/222023002/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:244: // If this is the first focus, set
a caret at the beginning of the text.
This comment is obsolete.  Please remove it.

[harpreet.sk, 2014-04-10 06:13:27]

Patch set 4 uploaded with comment removed and with rebase.

https://codereview.chromium.org/222023002/diff/40001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/222023002/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:244: // If this is the first focus, set
a caret at the beginning of the text.
On 2014/04/10 01:37:39, tkent wrote:
> This comment is obsolete.  Please remove it.

Done.

[harpreet.sk, 2014-04-10 06:20:58]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-04-10 06:21:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/222023002/60001

[commit-bot: I haz the power, 2014-04-10 06:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 06:51:41]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[tkent, 2014-04-10 09:07:05]

not lgtm.  Many tests are failing and we need to re-review.

[harpreet.sk, 2014-04-10 13:01:02]

New patch is uploaded fixing LayoutTest that were getting failed. Please have a
look.

[harpreet.sk, 2014-04-11 05:02:25]

Try bots runs fine for patch set 5 and no more LayoutTests are failing. Please
have a look.

[tkent, 2014-04-11 06:27:17]

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1077: 
I recommend to add new entries not to the bottom of TestExpectations.  The
bottom is very fragile.

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/copy-in-password-field.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/copy-in-password-field.html:10:
document.getElementById("text").select();
We need .focus() prior to every select().   Without them, execCommand("Copy" or
"Paste") doesn't work.

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
Why does the test work?  We need focus() to make the InsertHTML command work.

[harpreet.sk, 2014-04-11 07:33:01]

Patch set 6 addresses the change asked in comments in patch set 5

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1077: 
On 2014/04/11 06:27:17, tkent wrote:
> I recommend to add new entries not to the bottom of TestExpectations.  The
> bottom is very fragile.

New entries moved from the bottom and added in new place.

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/copy-in-password-field.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/copy-in-password-field.html:10:
document.getElementById("text").select();
On 2014/04/11 06:27:17, tkent wrote:
> We need .focus() prior to every select().   Without them, execCommand("Copy"
or
> "Paste") doesn't work.


Added focus().

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 06:27:17, tkent wrote:
> Why does the test work?  We need focus() to make the InsertHTML command work.


Added focus().

[tkent, 2014-04-11 07:40:19]

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 07:33:02, harpreet.sk wrote:
> On 2014/04/11 06:27:17, tkent wrote:
> > Why does the test work?  We need focus() to make the InsertHTML command
work.
> 
> 
> Added focus().

What's the answer of the question?  Why it worked?
If select() is enough, we don't need to add focus() back.

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-visibility-hidden.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-visibility-hidden.html:14:
input.select();
Ditto.

[harpreet.sk, 2014-04-11 08:09:13]

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 07:40:20, tkent wrote:
> On 2014/04/11 07:33:02, harpreet.sk wrote:
> > On 2014/04/11 06:27:17, tkent wrote:
> > > Why does the test work?  We need focus() to make the InsertHTML command
> work.
> > 
> > 
> > Added focus().
> 
> What's the answer of the question?  Why it worked?
> If select() is enough, we don't need to add focus() back.


For the given layout tests it worked because focus() was indirectly calling the
select() api which then select the text of input field. So even if we use
select() alone it will work .

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-visibility-hidden.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-visibility-hidden.html:14:
input.select();
On 2014/04/11 07:40:20, tkent wrote:
> Ditto.


For the given layout tests it worked because in those case focus() was
indirectly calling the select() api which then select the text of input field.
So even if we use select() alone it will work .

[tkent, 2014-04-11 08:19:40]

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 08:09:14, harpreet.sk wrote:
> On 2014/04/11 07:40:20, tkent wrote:
> > On 2014/04/11 07:33:02, harpreet.sk wrote:
> > > On 2014/04/11 06:27:17, tkent wrote:
> > > > Why does the test work?  We need focus() to make the InsertHTML command
> > work.
> > > 
> > > 
> > > Added focus().
> > 
> > What's the answer of the question?  Why it worked?
> > If select() is enough, we don't need to add focus() back.
> 
> 
> For the given layout tests it worked because focus() was indirectly calling
the
> select() api which then select the text of input field. So even if we use
> select() alone it will work .
> 

I'm asking if execCommand('InsertHTML', ...) works with an INPUT with select()
without focus().

[harpreet.sk, 2014-04-11 08:26:22]

On 2014/04/11 08:19:40, tkent wrote:
>
https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
> File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):
> 
>
https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
> LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
> input.select();
> On 2014/04/11 08:09:14, harpreet.sk wrote:
> > On 2014/04/11 07:40:20, tkent wrote:
> > > On 2014/04/11 07:33:02, harpreet.sk wrote:
> > > > On 2014/04/11 06:27:17, tkent wrote:
> > > > > Why does the test work?  We need focus() to make the InsertHTML
command
> > > work.
> > > > 
> > > > 
> > > > Added focus().
> > > 
> > > What's the answer of the question?  Why it worked?
> > > If select() is enough, we don't need to add focus() back.
> > 
> > 
> > For the given layout tests it worked because focus() was indirectly calling
> the
> > select() api which then select the text of input field. So even if we use
> > select() alone it will work .
> > 
> 
> I'm asking if execCommand('InsertHTML', ...) works with an INPUT with select()
> without focus().


yes it will work without focus() . Even the copy() and paste() command will work
without focus() . select() alone is sufficient.

[harpreet.sk, 2014-04-11 08:37:55]

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 08:19:41, tkent wrote:

> 
> I'm asking if execCommand('InsertHTML', ...) works with an INPUT with select()
> without focus().
> 

Yes it works without focus() and using select() alone. Even the copy() and
paste() commands are getting executed without focus() using select() alone.

[tkent, 2014-04-11 08:42:14]

lgtm

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
File LayoutTests/editing/pasteboard/input-with-display-none-div.html (right):

https://codereview.chromium.org/222023002/diff/80001/LayoutTests/editing/past...
LayoutTests/editing/pasteboard/input-with-display-none-div.html:18:
input.select();
On 2014/04/11 08:37:56, harpreet.sk wrote:
> On 2014/04/11 08:19:41, tkent wrote:
> 
> > 
> > I'm asking if execCommand('InsertHTML', ...) works with an INPUT with
select()
> > without focus().
> > 
> 
> Yes it works without focus() and using select() alone. Even the copy() and
> paste() commands are getting executed without focus() using select() alone.

Thanks.
Hmm, it sounds a bug.  Anyway, we may proceed as is.

[commit-bot: I haz the power, 2014-04-11 08:42:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/222023002/100001

[commit-bot: I haz the power, 2014-04-11 08:42:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 08:42:33]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 911.
  Hunk #2 succeeded at 1071 (offset -6 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
c1176d2f076fda532b4036001f9131063b6d8b96..3612a91e44ff62d15b08ffff44240640c37a0fbd
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -911,7 +911,7 @@ crbug.com/356957 [ Win Debug ]
editing/spelling/spellcheck-editable-on-focus.htm
 crbug.com/355489 media/activation-behavior.html [ Pass Failure Timeout ]
 crbug.com/356939 fast/forms/number/number-appearance-spinbutton-layer.html [
NeedsRebaseline ]
 crbug.com/356939 fast/forms/number/number-appearance-rtl.html [ NeedsRebaseline
]
-crbug.com/356939
fast/forms/number/number-appearance-spinbutton-disabled-readonly.html [
NeedsRebaseline ]
+crbug.com/356939 crbug.com/133242
fast/forms/number/number-appearance-spinbutton-disabled-readonly.html [
NeedsRebaseline ]
 crbug.com/356939 fast/speech/input-appearance-numberandspeech.html [
NeedsRebaseline ]
 crbug.com/356939 fast/forms/number/number-appearance-datalist.html [
NeedsRebaseline ]
 crbug.com/356939 fast/forms/date/date-appearance-basic.html [ NeedsRebaseline ]
@@ -1077,6 +1077,16 @@ crbug.com/362047 [ Mac Linux ] mhtml/mhtml_in_iframe.html
[ Crash Pass ]
 crbug.com/362050 [ Retina ]
fast/repaint/block-layout-inline-children-replaced.html [ Failure ]
 crbug.com/362073 [ Mac Debug ] fast/dom/shadow/input-color-in-content.html [
Crash Pass ]
 
+crbug.com/133242 editing/deleting/delete-all-text-in-text-field-assertion.html
[ NeedsRebaseline ]
+crbug.com/133242 fast/events/selectstart-prevent-selectall.html [
NeedsRebaseline ]
+crbug.com/133242 fast/forms/input-appearance-focus.html [ NeedsRebaseline ]
+crbug.com/133242 fast/forms/input-appearance-readonly.html [ NeedsRebaseline ]
+crbug.com/133242 fast/forms/onselect-textfield.html [ NeedsRebaseline ]
+crbug.com/133242 fast/forms/search-cancel-button-style-sharing.html [
NeedsRebaseline ]
+crbug.com/133242 fast/forms/search/search-appearance-basic.html [
NeedsRebaseline ]
+crbug.com/133242 fast/forms/textfield-outline.html [ NeedsRebaseline ]
+crbug.com/133242
w3c/web-platform-tests/shadow-dom/user-interaction/focus-navigation/test-002.html
[ NeedsRebaseline ]
+
 crbug.com/356780 [ Win ]
virtual/windows-directwrite/fast/text/basic/generic-family-reset.html [
NeedsManualRebaseline ]
 crbug.com/356780 [ Win ]
virtual/windows-directwrite/fast/text/drawBidiText.html [ NeedsManualRebaseline
]
 crbug.com/356780 [ Win ]
virtual/windows-directwrite/fast/text/international/bidi-listbox-atsui.html [
NeedsManualRebaseline ]

[harpreet.sk, 2014-04-11 09:03:27]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-04-11 09:03:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/222023002/120001

[commit-bot: I haz the power, 2014-04-11 09:34:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 09:34:24]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[harpreet.sk, 2014-04-11 13:11:35]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-04-11 13:11:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/222023002/120001

[commit-bot: I haz the power, 2014-04-11 14:15:32]

Change committed as 171347