Implement range input sanitization #21952

Eijebong · 2018-10-15T16:47:16Z

This change is

Fixes servo#19773

It was setting the value to " 123" without setting a maximum above. Since the specs specifies 100 to be the default max, the value we'd get after sanitization would be 100, not 123. This fixes it by setting the value to something that is between the default min/max.

highfive · 2018-10-15T16:47:20Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/element.rs, components/script/dom/bindings/htmlconstructor.rs, components/script/dom/servoparser/mod.rs, components/script/dom/htmlinputelement.rs
@KiChjang: components/script/dom/element.rs, components/script/dom/bindings/htmlconstructor.rs, components/script/dom/servoparser/mod.rs, components/script/dom/htmlinputelement.rs

servo-wpt-sync · 2018-10-15T16:47:59Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13529.

nox

First initial review.

nox · 2018-10-16T08:40:42Z

components/script/dom/bindings/htmlconstructor.rs

@@ -166,6 +166,8 @@ where
            // Step 8.4
            element.set_custom_element_definition(definition.clone());

+            element.done_creating();


This seems to have become substeps of step 9, could you renumber the steps and add a todo for the now missing part?

nox · 2018-10-16T08:42:00Z

components/script/dom/element.rs

+    pub fn done_creating(&self) {
+        self.done_creating.set(true);
+        if let Some(el) = self.downcast::<HTMLInputElement>() {
+            el.refresh_value(el.Value());


Can we avoid passing the element's own value to refresh_value? This seems like that involves cloning the value for no reason.

nox · 2018-10-16T08:42:38Z

components/script/dom/htmlinputelement.rs

    // https://html.spec.whatwg.org/multipage/#value-sanitization-algorithm
    fn sanitize_value(&self, value: &mut DOMString) {
+        let el = self.upcast::<Element>();


Nit: no need for intermediate binding here.

nox · 2018-10-16T08:42:55Z

components/script/dom/servoparser/mod.rs


    // Step 8.
    for attr in attrs {
        element.set_attribute_from_parser(attr.name, attr.value, None);
    }

+    element.done_creating();


Is this somewhere in the spec btw?

I don't think so but without that the input was revalidating it's input when we set value (with a default min/max), then for min then for max and it was causing troubles. For example, setting no value would set it to 50, then setting a min to 2, a max to 6 would set the value to 6 since 50 > 6 instead of setting it to 4.

The same thing has happened to me taking an independent attempt at getting min/max/step working; I'm going to pick up this approach and run with it.

nox · 2018-10-16T08:43:32Z

components/script/dom/htmlinputelement.rs

@@ -1193,7 +1193,47 @@ impl HTMLInputElement {
            },
            // https://html.spec.whatwg.org/multipage/#range-state-(type=range):value-sanitization-algorithm
            InputType::Range => {
-                value.set_best_representation_of_the_floating_point_number();
+                let minimum = self.Min().parse().unwrap_or(0f64);


Would be nice to not directly go through the IDL getters to avoid a DOMString copy. AFAIK min, max and step attribute values should be stored as integers in HTMLInputElement, feel free to ping me on IRC if you need more details about that.

nox · 2018-10-16T08:45:29Z

components/script/dom/htmlinputelement.rs

+                        value
+                    }
+                } else {
+                    if maximum <= minimum {


This should be just <.

nox · 2018-10-16T08:46:53Z

components/script/dom/htmlinputelement.rs

+                        maximum
+                    } else {
+                        value
+                    }


I think this can be a shorter expression:

value.min(maximum).max(minimum)

Clamping by the maximum before the minimum covers the case where the former is less than the latter.

nox · 2018-10-16T08:50:03Z

components/script/dom/htmlinputelement.rs

+                    }
+                } else {
+                    new_v
+                };


Note to self: I haven't reviewed this yet.

tigercosmos · 2018-10-31T18:56:23Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/range.html

@@ -34,7 +34,7 @@ <h1>Input Range</h1>
      <input type="range" id="stepdown_beyond_min" min=3 max=11 value=6 step=3 />
      <input type="range" id="illegal_min_and_max" min="ab" max="f" />
      <input type="range" id="illegal_value_and_step" min=0 max=5 value="ppp" step="xyz" />
-      <input type="range" id="should_skip_whitespace" value=" 123"/>
+      <input type="range" id="should_skip_whitespace" value=" 98"/>


Why use 98 rather than 123?

Because 123 is outside the default min-max range (0-100)

bors-servo · 2018-11-06T21:10:39Z

☔ The latest upstream changes (presumably #22086) made this pull request unmergeable. Please resolve the merge conflicts.

nox · 2018-11-22T12:24:06Z

I vaguely remember a discussion where we realised done_editing wasn't needed, did I dream that?

Eijebong · 2018-11-22T12:28:14Z

Probably, I have nothing related to done_editing in my logs

jdm · 2019-11-25T17:04:33Z

Closing due to lack of activity.

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

@Eijebong

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement  Steppy operation of input elements now functions. A few related tests still fail because of incorrect test expectations. A couple range reftests fail on what might be correct expectations; solving that involves layout, not DOM. Main changes here: - str.rs now has its datetime format parsers public, with some changes to align their output with chrono::NaiveDateTime and fix a bug in milliseconds validation - HTMLInputElement has new attributes, and code behind them for relevant concepts - servoparser/mod.rs now has some extra code to ensure that input sanitization happens at the right moment during input element creation; the spec is non-specific about when this happens, but it has to happen somewhere and in #21952 @Eijebong had independently come to the same conclusion as me about the timing. - Some tests are added where WPT wasn't already testing functionality I was adding; Firefox and Chrome pass them, Safari fails some in a reasonably expected way - Lots of test metadata changed to passing One test continues to fail because 0.1 * 6 != 0.6 and the spec is written in abstractions instead of actual floating-point numbers; other test failures are WPT bugs I've reported as such and will write fixes for soon if no one else does. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25390 and fix #25388 and fix #19773 (except validation doesn't exist) and fix #25386  - [X] There are tests for these changes OR

Eijebong added 3 commits October 15, 2018 14:50

Add a way not to sanitize values in an input until its creation is done

cb87025

Implement range input sanitization

e790002

Fixes servo#19773

highfive assigned avadacatavra Oct 15, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 15, 2018

servo-wpt-sync mentioned this pull request Oct 15, 2018

Implement range input sanitization web-platform-tests/wpt#13529

Closed

nox reviewed Oct 16, 2018

View reviewed changes

jdm assigned nox and unassigned avadacatavra Oct 16, 2018

tigercosmos reviewed Oct 31, 2018

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Nov 6, 2018

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 22, 2018

jdm closed this Nov 25, 2019

jdm mentioned this pull request Nov 25, 2019

consider min, max, step for range type input element #19773

Closed

pshaughn mentioned this pull request Dec 31, 2019

make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement #25405

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement range input sanitization #21952

Implement range input sanitization #21952

Eijebong commented Oct 15, 2018 •

edited by SimonSapin

Loading

highfive commented Oct 15, 2018

servo-wpt-sync commented Oct 15, 2018

nox left a comment

nox Oct 16, 2018

nox Oct 16, 2018

nox Oct 16, 2018

nox Oct 16, 2018

Eijebong Oct 16, 2018

pshaughn Dec 27, 2019

nox Oct 16, 2018

nox Oct 16, 2018

nox Oct 16, 2018

nox Oct 16, 2018

tigercosmos Oct 31, 2018

Eijebong Oct 31, 2018

bors-servo commented Nov 6, 2018

nox commented Nov 22, 2018

Eijebong commented Nov 22, 2018

jdm commented Nov 25, 2019

Implement range input sanitization #21952

Implement range input sanitization #21952

Conversation

Eijebong commented Oct 15, 2018 • edited by SimonSapin Loading

highfive commented Oct 15, 2018

servo-wpt-sync commented Oct 15, 2018

nox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Nov 6, 2018

nox commented Nov 22, 2018

Eijebong commented Nov 22, 2018

jdm commented Nov 25, 2019

Eijebong commented Oct 15, 2018 •

edited by SimonSapin

Loading