1839572 - input[type=number] does not return expected value when there is a trailing decimal point [was: does not have the :invalid pseudo class for some invalid inputs]

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect)

Description

[Kai.Ruhnau]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36 Edg/114.0.1823.51

Steps to reproduce:

Go to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number and start using the input type=number in the "Try it" section.

The initial input state is .value='' and .matches(':invalid')=false.
For valid inputs like "20", the input state is .value='20' and .matches(':invalid')=false.
For invalid inputs like "a", the input state is .value='' and .matches(':invalid')=true.
That's all expected and good.

However, for an input of "20." (note the additional dot), the input is now in an inconsistent state.

Actual results:

The input state is .value='' and .matches(':invalid')=false, that is something was entered, it couldn't be interpreted, but is still considered valid.

Expected results:

I can think of two outcomes: Either "20." is invalid and this is reflected in the pseudo class, or (and this is the preferred behavior as it matches Chromium) "20." is valid and interpreted as ".value='20'"

Note that the spinners of the input consider "20." valid enough to increase to 21 or decrease to 19 while doing nothing for inputs like "a"

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Form Controls' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Jonathan Kew [:jfkthame]]

I believe "20." should be valid; the dot is a decimal point, and this is equivalent to "20.0" or just "20". So the bug here is that when the input has a trailing dot (and no following digit), it's returning an empty .value string.

Interestingly, .valueAsNumber returns the expected number.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

This was changed recently in bug 1821569: https://searchfox.org/mozilla-central/rev/d31e56f7b3c2c18b8071a7b2a2fb6b4e01e3d3e8/dom/html/HTMLInputElement.cpp#4590

Can you take a look?

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1821569

Comment 5

[imurran03402]

Hi all, according to this post v113 is affected too. Just as some posters say, our users cannot enter decimal numbers in Firefox as it gets cleared out. Copying a decimal value into the input field, however, does not clear it. When a trailing dot is entered OnInput event is fired with an empty string as a value

Comment 6

[Pascal Chevrel:pascalc]

Jonathan, do you think this could have a serious webcompat impact? Thanks

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1821569

Comment 8

[Adam Vandolder [:avandolder]]

According to the HTML spec, a number with a trailing decimal point is not considered valid, and should fail to parse. This is checked by the html/semantics/forms/the-input-element/number.html web platform test, which Firefox was failing before bug 1821569 was resolved. That patch did not include the constraint validation that should be setting the bad input flag, so that needs to be fixed at the very least.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Adam Vandolder [:avandolder] from comment #8)

According to the HTML spec, a number with a trailing decimal point is not considered valid,

So it is....

and should fail to parse.

...but I'm not sure this follows. The parsing rules there, AFAICT, will successfully parse a number with a trailing decimal point: in particular, step 13.2 does not return an error, it simply jumps to the "conversion" stage.

This is checked by the html/semantics/forms/the-input-element/number.html web platform test, which Firefox was failing before bug 1821569 was resolved.

I'm finding it hard to follow all the twists and turns of the spec, but I think there may be two different scenarios to consider. When the value attribute is set, the spec tells us to run the value sanitization algorithm, which would replace a number-with-trailing-period with an empty string. That's what is tested by the above WPT test.

But when the user is changing the value, the spec seems to be telling us to "apply[ing] the rules for parsing floating-point number values to it", which would interpret "10." as the number 10.

It seems wrong/inconsistent that at present, when the user types "123." into the number field, we return an empty string for value, yet we return the number 123 for valueAsNumber. The spec for this tells us that

The valueAsNumber IDL attribute represents the value of the element, interpreted as a number.

but if the value has been set to an empty string (because "123." was not a valid floating-point number), how can we return 123 here?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Pascal Chevrel:pascalc from comment #6)

Jonathan, do you think this could have a serious webcompat impact? Thanks

Yes, it looks like it. As well as comment 5 here, we have regression reports like bug 1821569 and bug 1838254, which I think have been incorrectly closed as invalid. This seems to indicate people are encountering (and being surprised by) the behavior change.

Comment 11

[Adam Vandolder [:avandolder]]

(In reply to Jonathan Kew [:jfkthame] from comment #9)

I'm finding it hard to follow all the twists and turns of the spec, but I think there may be two different scenarios to consider. When the value attribute is set, the spec tells us to run the value sanitization algorithm, which would replace a number-with-trailing-period with an empty string. That's what is tested by the above WPT test.

But when the user is changing the value, the spec seems to be telling us to "apply[ing] the rules for parsing floating-point number values to it", which would interpret "10." as the number 10.

Ah, alright. That change was unintentional - the issue seems to be that we're sanitizing the input value in more cases than the spec says to, which was fine when the sanitizer was only running the parsing algorithm, and not checking the "valid floating-point number" part.

Comment 12

[Donal Meehan [:dmeehan]]

Ah, alright. That change was unintentional - the issue seems to be that we're sanitizing the input value in more cases than the spec says to, which was fine when the sanitizer was only running the parsing algorithm, and not checking the "valid floating-point number" part.

:avandolder do you have any rough idea when you plan on having a look at patching this?
Wondering if we will have something in time for Fx116 or potentially a FX115 dot release ride along

Comment 13

[Adam Vandolder [:avandolder]]

Attachment: Bug 1839572 - Fix number input value sanitization. r=#dom-core

Comment 14

[Adam Vandolder [:avandolder]]

(In reply to Donal Meehan [:dmeehan] from comment #12)

Ah, alright. That change was unintentional - the issue seems to be that we're sanitizing the input value in more cases than the spec says to, which was fine when the sanitizer was only running the parsing algorithm, and not checking the "valid floating-point number" part.

:avandolder do you have any rough idea when you plan on having a look at patching this?
Wondering if we will have something in time for Fx116 or potentially a FX115 dot release ride along

Hopefully will be able to land this patch before Fx116.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b2b24f16463
Fix number input value sanitization. r=dom-core,edgar

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41112 for changes under testing/web-platform/tests

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4b2b24f16463

Comment 18

[Pascal Chevrel:pascalc]

Adam, do you want to request an uplift to our last 116 beta? Thanks

Comment 19

[Kai.Ruhnau]

Sweet, thanks!

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Adam Vandolder [:avandolder]]

(In reply to Pascal Chevrel:pascalc from comment #18)

Adam, do you want to request an uplift to our last 116 beta? Thanks

Yes, if possible.

Comment 22

[Adam Vandolder [:avandolder]]

Attachment: Beta Uplift Approval Request

Approval Request Comment
[Feature/Bug causing the regression]: Interop-2022 web platform test fix from bug 1821569
[User impact if declined]: Number inputs will fail to accept user inputted values with leading '+' or '.', in particular breaking sites that check for input values upon each keystroke, which will fail as soon as a '.' is entered.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fixes an unintended regression and aligns Firefox with the HTML specification. Regression has only existed for 4 months, so it is unlikely any sites are relying on it.
[String changes made/needed]: n/a

Comment 23

[Adam Vandolder [:avandolder]]

Comment on attachment 9344917 [details]
Beta Uplift Approval Request

Approval Request Comment
[Feature/Bug causing the regression]: Interop-2022 web platform test fix from bug 1821569
[User impact if declined]: Number inputs will fail to accept user inputted values with leading '+' or '.', in particular breaking sites that check for input values upon each keystroke, which will fail as soon as a '.' is entered.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fixes an unintended regression and aligns Firefox with the HTML specification. Regression has only existed for 4 months, so it is unlikely any sites are relying on it.
[String changes made/needed]: n/a

Comment 24

[Dianna Smith [:diannaS]]

Comment on attachment 9342840 [details]
Bug 1839572 - Fix number input value sanitization. r=#dom-core

Approved for 116.0b8

Comment 25

[Dianna Smith [:diannaS]]

Comment on attachment 9344917 [details]
Beta Uplift Approval Request

clearing, since i added the approval in the phab patch

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/418fc97ccb72

Comment 27

[Dianna Smith [:diannaS]]

Backed out for causing build bustages in HTMLInputElement.cpp
Backout link
Push with failures
Failure log

most likely a dependency on bug 1842027, marking as wontfix for 116

Comment 28

[Emilio Cobos Álvarez [:emilio]]

I'm confused about the patch, why doing that only on SanitizationKind::Other? Feels pretty random, can we pass a more reasonable reason for the places we need this?

Comment 29

[Adam Vandolder [:avandolder]]

The patch for bug 1857506 seems to have addressed this.