Input type url with line break and leading-trailing whitespace should be stripped.

Input type url with line break and leading-trailing whitespace should be stripped.

According to spec https://html.spec.whatwg.org/multipage/forms.html#url-state-%28type=url%29,
we have to strip the line break and leading-trailing whitespaces from the url value, so added santization
algorithm to strip the line break and leading-trailing whitespaces.
Firefox works according to spec and strips the linebreak and whitespaces from value.

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

[prashant.hiremath, 2015-01-06 07:09:33]

prashhir@cisco.com changed reviewers:
+ eae@chromium.org

[prashant.hiremath, 2015-01-06 07:10:12]

Please Review.

[mlamouri (slow - plz ping), 2015-01-06 10:14:39]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-01-06 10:14:39]

Could you also strip the line breaks in that CL? Sounds like you want to do like
we currently do for email, right?

[prashant.hiremath, 2015-01-06 11:09:58]

On 2015/01/06 10:14:39, Mounir Lamouri wrote:
> Could you also strip the line breaks in that CL? Sounds like you want to do
like
> we currently do for email, right?

Yes, I'm referring our EmailInputType class Implementation.
Doesn't this code

String value = proposedValue.removeCharacters(isHTMLLineBreak);
(URLInputType.cpp : 59)

strip the line breaks present in the value?

[mlamouri (slow - plz ping), 2015-01-06 12:05:13]

On 2015/01/06 11:09:58, prashant.hiremath wrote:
> On 2015/01/06 10:14:39, Mounir Lamouri wrote:
> > Could you also strip the line breaks in that CL? Sounds like you want to do
> like
> > we currently do for email, right?
> 
> Yes, I'm referring our EmailInputType class Implementation.
> Doesn't this code
> 
> String value = proposedValue.removeCharacters(isHTMLLineBreak);
> (URLInputType.cpp : 59)
> 
> strip the line breaks present in the value?

That's definitely correct. Sorry for the oversight. Maybe you should add that to
the CL name/description?

Also, it might be worth considering using test-harness.js for the test. You
wouldn't need an expected file and it would make sharing the test more easily.

[prashant.hiremath, 2015-01-07 12:32:59]

Thanks for the review Mounir.
I have updated LayoutTest using testharness.js, PTAL.

[mlamouri (slow - plz ping), 2015-01-07 12:59:34]

lgtm, thanks! :)

[eae, 2015-01-08 19:56:58]

LGTM

[prashant.hiremath, 2015-01-09 02:22:05]

The CQ bit was checked by prashhir@cisco.com

[commit-bot: I haz the power, 2015-01-09 02:22:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836113002/20001

[commit-bot: I haz the power, 2015-01-09 03:33:45]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188106

[tkent, 2015-02-02 07:21:09]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2015-02-02 07:21:10]

I have a concern about this change.  What happens if a user types spaces into an
input[type=url] box?

[prashant.hiremath, 2015-02-03 04:43:43]

On 2015/02/02 07:21:10, available but slow wrote:
> I have a concern about this change.  What happens if a user types spaces into
an
> input[type=url] box?

@tkent, sorry for the delayed response.

When you say user types spaces into an input box, did you mean:
1. When user inputs the value with space in between. For eg. "home page". My
change removes only leading and trailing
   white space. so value will be "home page" only.
2. When user inputs all spaces into an input box, this case I didn't consider in
above change. But after reading 
   your comment I did try by entering all spaces
(http://jsbin.com/dosehizaza/1/edit?html,output) in input box,
   In this case Blink(even without my change) and Firefox behave differently. On
outputting length of value, Firefox 
   considers entered spaces and gives length to be number of spaces, but Blink
doesn't. I'm not sure whose behavior is 
   correct. I think this case is what your comment is about. Am I right?

[tkent, 2015-02-03 05:01:09]

On 2015/02/03 04:43:43, prashant.hiremath wrote:
> On 2015/02/02 07:21:10, available but slow wrote:
> > I have a concern about this change.  What happens if a user types spaces
into
> an
> > input[type=url] box?
> 
> @tkent, sorry for the delayed response.
> 
> When you say user types spaces into an input box, did you mean:
> 1. When user inputs the value with space in between. For eg. "home page". My
> change removes only leading and trailing
>    white space. so value will be "home page" only.
> 2. When user inputs all spaces into an input box, this case I didn't consider
in
> above change. But after reading 

I have a concern about leading/trailing spaces in user input.
e.g.
1. A user typed "  http://example.com/"  (typed length = 21)
2. input.value returns "http://example.com/" (stripped, length = 19)
3. JavaScript code wants to put text caret to the end of the value;
    input.selectionStart = input.value.length;  input.selectionEnd =
input.value.length;
4. It won't work well because input.value and the actual string are mismatched.

The current behavior is inconsistent and will break existing web pages.  I
recommend to revert this change.

>    your comment I did try by entering all spaces
> (http://jsbin.com/dosehizaza/1/edit?html,output) in input box,
>    In this case Blink(even without my change) and Firefox behave differently.
On
> outputting length of value, Firefox 
>    considers entered spaces and gives length to be number of spaces, but Blink
> doesn't. I'm not sure whose behavior is 
>    correct. I think this case is what your comment is about. Am I right?

So, Firefox doesn't sanitize user-input strings?

[prashant.hiremath, 2015-02-03 08:45:41]

On 2015/02/03 05:01:09, available but slow wrote:
> On 2015/02/03 04:43:43, prashant.hiremath wrote:
> > On 2015/02/02 07:21:10, available but slow wrote:
> > > I have a concern about this change.  What happens if a user types spaces
> into
> > an
> > > input[type=url] box?
> > 
> > @tkent, sorry for the delayed response.
> > 
> > When you say user types spaces into an input box, did you mean:
> > 1. When user inputs the value with space in between. For eg. "home page". My
> > change removes only leading and trailing
> >    white space. so value will be "home page" only.
> > 2. When user inputs all spaces into an input box, this case I didn't
consider
> in
> > above change. But after reading 
> 
> I have a concern about leading/trailing spaces in user input.
> e.g.
> 1. A user typed "  http://example.com/%22  (typed length = 21)
> 2. input.value returns "http://example.com/" (stripped, length = 19)
> 3. JavaScript code wants to put text caret to the end of the value;
>     input.selectionStart = input.value.length;  input.selectionEnd =
> input.value.length;
> 4. It won't work well because input.value and the actual string are
mismatched.
> 
> The current behavior is inconsistent and will break existing web pages.  I
> recommend to revert this change.
> 
> >    your comment I did try by entering all spaces
> > (http://jsbin.com/dosehizaza/1/edit?html,output) in input box,
> >    In this case Blink(even without my change) and Firefox behave
differently.
> On
> > outputting length of value, Firefox 
> >    considers entered spaces and gives length to be number of spaces, but
Blink
> > doesn't. I'm not sure whose behavior is 
> >    correct. I think this case is what your comment is about. Am I right?
> 
> So, Firefox doesn't sanitize user-input strings?

Yes @tkent, I agree with you. I now verified it breaks the current webpage
behavior.

I checked the Firefox behavior, It behaves as follows:
1. When user inputs the data, they don't sanitize the input value.
2. When JavaScript tries to set the value, they run sanitization 
   algorithm : trim leading and trailing whitespace  
   from input value.

After I revert my changes, case 1 will work fine. But 2nd case we
have to handle separately in case of value set by JavaScript, we call 
input value sanitization. Do you agree with above?

[mlamouri (slow - plz ping), 2015-02-03 09:53:10]

> So, Firefox doesn't sanitize user-input strings?

Firefox let the user type whatever they want and will sanitize the value being
sent to the server when the form is submitted. Obviously, things are different
if the value is not valid.

[tkent, 2015-02-04 00:20:55]

On 2015/02/03 08:45:41, prashant.hiremath wrote:
> After I revert my changes, case 1 will work fine. But 2nd case we
> have to handle separately in case of value set by JavaScript, we call 
> input value sanitization. Do you agree with above?

IMO, we should revert the CL, should keep the previous type=url behavior, and
discuss what's the correct behavior in crbug.com and/or the standard body.

Type=number and type=email have similar mismatch issues.  So they don't support
selection API.

[prashant.hiremath, 2015-02-04 11:54:40]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/893013003/ by prashhir@cisco.com.

The reason for reverting is: Reverting this Patch, as this will break the old
behavior of browser in case of user types spaces into input box type url.

It need to be discussed with wider audience, what should be the correct behavior
in case of user inputting and JavaScript setting the input value, to reland this
patch.
.