search

servo / servo

Servo, the embeddable, independent, memory-safe, modular, parallel web rendering engine
https://servo.org
Mozilla Public License 2.0
26.25k stars 2.92k forks source link

parse_floating_point_number does not conform to spec #32269

Closed sagudev closed 2 weeks ago

sagudev commented 3 weeks ago

As noted in https://github.com/servo/servo/pull/32230#discussion_r1597356650

SPEC: https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values

sagudev commented 3 weeks ago

Will you do this @shanehandley?

shanehandley commented 3 weeks ago

I will take a look! Thank you.

shanehandley commented 3 weeks ago

I was wrong 🙇 . parse_floating_point_number is probably correct, my mistake was using it when I should have used set_best_representation_of_the_floating_point_number, which is the one we need to touch up 👍

This comment in the WPT tests repo clarified things for me: https://github.com/web-platform-tests/wpt/issues/44315#issuecomment-1918932774

the best represetation of the number n as a floating point number, referenced from the setter, that relies on the ECMAScript ToString, itself dictating that the number -0 is stringified to 0

We have a DOMString::set_best_representation_of_the_floating_point_number(), It is not called from anywhere in the codebase currently, and it uses the plain rust conversion to_string() without applying any logic in the spec. It looks like it was originally added with the <range> logic, but fell out of use.

It seems the work required is:

  1. Improve the implementation of set_best_representation_of_the_floating_point_number to be more compliant with the ECMAScript ToString spec
  2. Use it in place of parse_floating_point_number for affected elements: <meter>, <progress>, <input type="number"> and <input type="range">.

For 1., there is a lot of overlap between the ECMAScript and HTML specs for parsing floats (with radix omitted) - so maybe we can pluck out the some key steps and mostly fall back to it?

sagudev commented 2 weeks ago

Oh, you're right, I also completely missed best represetation of the number n as a floating point number part.

ECMAScript parsing of floats is implemented in SM (I think), so we would need conversions it might be better to just do some pre/post processing like it's done in parse_floating_point_number on top of rust's to_string.