Bug 250535 – Parsing input type=number should skip spaces

• Component: Forms
• Status: RESOLVED FIXED

[Ahmad Saleem]

Hi Team,

While going through Blink's commit, I came across another potential merge:

Blink - https://src.chromium.org/viewvc/blink?view=revision&revision=171834

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/platform/text/PlatformLocale.cpp#300

I haven't checked Web-Spec but I think we can have a quick check.

Thanks!

[Karl Dubost]

All tests pass here 
https://wpt.fyi/results/html/semantics/forms/the-input-element/number.html?label=master&label=experimental&aligned&view=subtest&q=input

But that doesn't mean it covers the full extent of the spec.


Spec 
https://html.spec.whatwg.org/multipage/input.html#number-state-(type=number)

> The value sanitization algorithm is as follows: If the value of the element is not a valid floating-point number, then set it to the empty string instead.

Links to  an algorithm to convert from string to number. 
https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-floating-point-number


> Step 6 Skip ASCII whitespace within input given position.

links to https://infra.spec.whatwg.org/#skip-ascii-whitespace

> To skip ASCII whitespace within a string input given a position variable position, collect a sequence of code points that are ASCII whitespace from input given position. The collected code points are not used, but position is still updated.

And ASCII whitespace means:
https://infra.spec.whatwg.org/#ascii-whitespace

> ASCII whitespace is U+0009 TAB, U+000A LF, U+000C FF, U+000D CR, or U+0020 SPACE.

[Karl Dubost]

to Note that the current code is using.

String input = localized.stripWhiteSpace();
https://searchfox.org/wubkat/source/Source/WebCore/platform/text/PlatformLocale.cpp#300




which is 
https://searchfox.org/wubkat/source/Source/WTF/wtf/text/WTFString.cpp#160


String String::stripWhiteSpace() const
{
    // FIXME: Should this function, and the many others like it, be inlined?
    // FIXME: This function needs a new name. For one thing, "whitespace" is a single
    // word so the "s" should be lowercase. For another, it's not clear from this name
    // that the function uses the Unicode definition of whitespace. Most WebKit callers
    // don't want that and eventually we should consider deleting this.
    return m_impl ? m_impl->stripWhiteSpace() : String { };
}


https://searchfox.org/wubkat/source/Source/WTF/wtf/text/StringImpl.cpp#744
Ref<StringImpl> StringImpl::stripWhiteSpace()
{
    return stripMatchedCharacters(isSpaceOrNewline);
}

https://searchfox.org/wubkat/source/Source/WTF/wtf/text/StringImpl.cpp#717

and

inline bool isSpaceOrNewline(UChar32 character)
{
    // Use isASCIISpace() for all Latin-1 characters. This will include newlines, which aren't included in Unicode DirWS.
    return isLatin1(character) ? isASCIISpace(character) : u_charDirection(character) == U_WHITE_SPACE_NEUTRAL;
}

[Radar WebKit Bug Importer]

<rdar://problem/104453204>

[Ahmad Saleem]

Just to add, we fail the attached test with following output while run on WebKit ToT using Webkit Testrunner:

should skip spaces from user input number

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


FAIL input.valueAsNumber should be 123456. Was NaN.
FAIL input.value should be 123456. Was .
PASS successfullyParsed is true
Some tests failed.

TEST COMPLETE


^ This is similar to Firefox Nightly 118 while Chrome Canary 118 passes this test.

[Ahmad Saleem]

Changing to:

auto input = localized.removeCharacters([](auto character) {
        return isUnicodeCompatibleASCIIWhitespace(character);
    });

Fixes this bug and make us pass this test case.

@Anne - any input before I do PR?

[Anne van Kesteren]

There's two different things here, right? There's parsing the value attribute of the input element correctly and there's dealing with end user input.

I would expect that we don't have bugs for the former and the Chromium bug confirms this is about the latter and therefore not covered by WPT.

For the latter however if we decide we want to ignore whitespace in the input (and that is probably a decision that Aditya should make a call on for Apple platforms and potentially coordinate on internally) we would want to ignore more than just ASCII whitespace. We'd want to ignore all Unicode whitespace code points.

Hope that helps!

[Ahmad Saleem]

(In reply to Anne van Kesteren from comment #6)
> There's two different things here, right? There's parsing the value
> attribute of the input element correctly and there's dealing with end user
> input.
> 
> I would expect that we don't have bugs for the former and the Chromium bug
> confirms this is about the latter and therefore not covered by WPT.
> 
> For the latter however if we decide we want to ignore whitespace in the
> input (and that is probably a decision that Aditya should make a call on for
> Apple platforms and potentially coordinate on internally) we would want to
> ignore more than just ASCII whitespace. We'd want to ignore all Unicode
> whitespace code points.
> 
> Hope that helps!

Thanks @Anne. Let's wait for Aditya's input. :-)

[Ahmad Saleem]

Commit - https://source.chromium.org/chromium/chromium/src/+/534707556c6c556379a42a8fac700433246e4ecd

[Ahmad Saleem]

should skip spaces from user input number

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS input.valueAsNumber is 123456
PASS input.value is "123456"
PASS successfullyParsed is true

TEST COMPLETE

_______

Fixed on 302938@main but will do PR to import test.

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/53839

[EWS]

Committed 302995@main (4fc9e19fc6ab): <https://commits.webkit.org/302995@main>

Reviewed commits have been landed. Closing PR #53839 and removing active labels.