Page MenuHomePhabricator

JsonSchema::TYPE_NUMBER does not handle floats correctly
Closed, ResolvedPublic3 Estimated Story Points

Description

When adding underlinkedWeight and minimumLinkScore from Suggested edits to MediaWiki-extensions-CommunityConfiguration, it works, but upon saving, I get an error message from the browser on the form field: Please enter a valid value. The two nearest value are 0 and 1.

This is not actually true, as underlinkedWeight and minimumLinkScore are floats. We should support floats in CC as well.

Screenshot of an example of the undesired error:

image.png (159×436 px, 19 KB)

Acceptance criteria:

  • Browser validation should not prevent a valid value from being submitted
    • A new json schema type "integer" is provided to be able to differentiate floats (floats will use the existing number type)
    • When the existing json schema type "number" is used, the form should allow to enter decimals
    • Growth Schemas using integers should use the integer type
    • Growth Schemas using floats should use the number type

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I think the most json-schema compliant way to represent that a value is a float is to use the multipleOf keyword. eg:

{
  "type": "number",
  "multipleOf" : 0.01
}

This way we could differentiate between floats and integers. Not sure if we should push this responsibility to consumers or do it internally.

AFAICS, JSON schema differentiates between "type": "integer" and "type": "number". Floats are only possible in the latter case. Or at least, this is an impression I got from reading the docs.

If this is indeed the case, maybe we can render the number input with size=1 (the default) when the integer type is used, and an arbitrary float (for example, 0.01) when the number type is used? That way, consumers would have an easy way to declare whether floats are permitted or not. It'd have limitations (mainly on the precision, which would be lower than float's), but it should work reasonable well for the MVP I think. In the future, we might consider using multipleOf (or something else) to support floats with an arbitrary precision.

I think we do need to implement some support for floats for the MVP (otherwise, migrating Suggested edits would be fairly difficult to do), but we definitely do not need an arbitary precision float here.

Any other opinions on this (@Sgs or others)?

Mh, based on https://json-schema.org/understanding-json-schema/reference/numeric, it sounds like 0.123 should just be plain valid for a "type": "number"? This to me almost sounds like an upstream bug.

Maybe we can have a workaround, but should probably create an upstream bug report and track it as Upstream .

Nevermind, I mistook this from reading the conversation as a json-schema bug, but this is actually purely in the frontend. So, if at all, the upstream to talk to would be Codex.

KStoller-WMF set the point value for this task to 3.Apr 30 2024, 4:16 PM

Looking at the MDN documentation for <input type="number">, it seems we can set step="any" and it will just allow arbitrary floats. This works on my Firefox 126, but I'm actually not sure how good the browser support for that is. I did not find that feature on caniuse.com

Looking at the MDN documentation for <input type="number">, it seems we can set step="any" and it will just allow arbitrary floats. This works on my Firefox 126, but I'm actually not sure how good the browser support for that is. I did not find that feature on caniuse.com

Alternatively, we should probably consider to plain not use type="number" at all on the <input>. The designs don't have the spin buttons (and I doubt we actually want them), and we want to do our own validation anyway. The recommended alternative seems to be <input type="text" inputmode="numeric">.

What do you think?

Alternatively, we should probably consider to plain not use type="number" at all on the <input>. The designs don't have the spin buttons (and I doubt we actually want them), and we want to do our own validation anyway. The recommended alternative seems to be <input type="text" inputmode="numeric">.

inputmode="numeric" does indeed result in allowing floats, but it also results in disabling the validation altogether. This means you'd be able to enter foo and submit it to the server. Of course, JSON schema validation would catch that as invalid data and result in an error, but my understanding is we want to eventually make server side validation errors next to impossible to trigger in the form (T358659), rather than opening new avenues in which it could happen. The spin buttons are a valid point, but probably a browser decision we just have to accept.

[...] my understanding is we want to eventually make server side validation errors next to impossible to trigger in the form (T358659), rather than opening new avenues in which it could happen

It was my assumption that we want to make use of custom validation styles (see for example the style guide https://doc.wikimedia.org/codex/latest/style-guide/constructing-forms.html#validation ) and our own copy for the error/validation messages. For the browser validation, we can affect neither (as far as I know). Also, browser validation has a reputation for being not very accessible, neither visually (font-size, color) nor by screen readers (However, my information about screen readers is a couple of years out of date. That might have improved significantly by now, I do not know.)

On the other hand, I can see your point in that the browser validation probably requires much less effort, and that CommunityConfiguration is already well behind schedule and there is still significant work to do. Also, we can probably add custom validation styles and messages afterward and incrementally, if that is thought to be worthwhile.

It was my assumption that we want to make use of custom validation styles (see for example the style guide https://doc.wikimedia.org/codex/latest/style-guide/constructing-forms.html#validation ) and our own copy for the error/validation messages. For the browser validation, we can affect neither (as far as I know). Also, browser validation has a reputation for being not very accessible, neither visually (font-size, color) nor by screen readers (However, my information about screen readers is a couple of years out of date. That might have improved significantly by now, I do not know.)

In my exploration in T358659 I found the browser validation API to provide a decent amount of customization. For instance the default bubble can be styled or completely prevented and an inlined text can be provided instead.

On the other hand, I can see your point in that the browser validation probably requires much less effort, and that CommunityConfiguration is already well behind schedule and there is still significant work to do. Also, we can probably add custom validation styles and messages afterward and incrementally, if that is thought to be worthwhile.

Right, the main blocker with server-driven schema validation is T351879. That task is not easy to solve because there are different solutions: (a) stick with justinrainbow library and re-use some of WikiLambda's work on error translations, b) update justinrainbow across MW products, c) use Opis library instead, which also requires MW-broad updates (eg T319054). The solution proposed T358659 entails a different validation approach, in the lines of make server side validation errors next to impossible to trigger in the form. The idea is that a good form UX validation should not push users to know anything about json schema or json at all. With that in mind, the remaining question is: is it worth to build a system that maps json-schema errors to a form, or is it good enough to build a system that creates a form which validation rules result in a valid json-schema?

Looking at the MDN documentation for <input type="number">, it seems we can set step="any" and it will just allow arbitrary floats. This works on my Firefox 126, but I'm actually not sure how good the browser support for that is. I did not find that feature on caniuse.com

I think this is the caniuse page: html_elements_input_step, the support seems sufficient for our product.

AFAICS, JSON schema differentiates between "type": "integer" and "type": "number". Floats are only possible in the latter case. Or at least, this is an impression I got from reading the docs.

Agree.

If this is indeed the case, maybe we can render the number input with size=1 (the default) when the integer type is used, and an arbitrary float (for example, 0.01) when the number type is used? That way, consumers would have an easy way to declare whether floats are permitted or not. It'd have limitations (mainly on the precision, which would be lower than float's), but it should work reasonable well for the MVP I think. In the future, we might consider using multipleOf (or something else) to support floats with an arbitrary precision.

I guess you mean step=1 for integers and step=0.01 for numbers. I would use step=any for the latter but I agree this is a good initial solution that we can improve later by using multipleOf. I have updated the acceptance criteria to reflect the current proposal.

Change #1026913 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/CommunityConfiguration@master] JsonSchema: introduce type integer

https://gerrit.wikimedia.org/r/1026913

Change #1026914 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/CommunityConfiguration@master] Number control: use input attribute step

https://gerrit.wikimedia.org/r/1026914

Change #1026915 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Config: use type integer instead of number

https://gerrit.wikimedia.org/r/1026915

Change #1026913 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] JsonSchema: introduce type integer

https://gerrit.wikimedia.org/r/1026913

Change #1026914 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] Number control: use input attribute step

https://gerrit.wikimedia.org/r/1026914

Change #1026915 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Config: use type integer instead of number

https://gerrit.wikimedia.org/r/1026915