Parse HTMLInputElement attributes #10962

KiChjang · 2016-05-02T08:07:09Z

Fixes #10491.

This change is

highfive · 2016-05-02T08:07:13Z

Heads up! This PR modifies the following files:

@bholley: components/style/attr.rs
@KiChjang: components/script/dom/element.rs, components/script/dom/htmlinputelement.rs

highfive · 2016-05-02T08:07:14Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

jdm · 2016-05-02T16:09:08Z

components/script/dom/htmlinputelement.rs

@@ -862,6 +862,8 @@ impl VirtualMethods for HTMLInputElement {
    fn parse_plain_attribute(&self, name: &Atom, value: DOMString) -> AttrValue {
        match name {
            &atom!("accept") => AttrValue::from_comma_separated_tokenlist(value),
+            &atom!("alt") |
+            &atom!("dirname") => AttrValue::String(value),


This is the default, so we don't specify it explicitly.

asajeffrey · 2016-05-02T16:09:44Z

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 454 [r6] (raw file):
According to the IDL at https://html.spec.whatwg.org/multipage/forms.html#the-input-element:dom-input-alt, max and min are DOMStrings, should we be discarding non-numeric values?

Can max and min not be given a valid value of the current type? https://html.spec.whatwg.org/multipage/forms.html#attr-input-type This includes non-numeric types such as times.

components/script/dom/htmlinputelement.rs, line 457 [r6] (raw file):
Code style: write this as else if?

components/script/dom/htmlinputelement.rs, line 459 [r6] (raw file):
This is doing a malloc every time, rather than reusing the value string. Is there no way to reuse the given string?

components/script/dom/htmlinputelement.rs, line 461 [r6] (raw file):
Is there really no default case?

components/script/dom/htmlinputelement.rs, line 474 [r6] (raw file):
Same comments as for SetMax.

components/script/dom/htmlinputelement.rs, line 516 [r6] (raw file):
Can we use an any atom?

components/script/dom/htmlinputelement.rs, line 523 [r6] (raw file):
Again, a malloc every time we set step?

components/script/dom/htmlinputelement.rs, line 529 [r6] (raw file):
Discarding step in most cases?

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):
Converting the value into a double, then back to a string, then storing the string?

Comments from Reviewable

KiChjang · 2016-05-03T01:29:09Z

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 459 [r6] (raw file):
This is where https://html.spec.whatwg.org/multipage/forms.html#attr-input-max says that we should use the default value if the type is range, so reusing the value looks wrong.

components/script/dom/htmlinputelement.rs, line 461 [r6] (raw file):
From what I'm reading, https://html.spec.whatwg.org/multipage/forms.html#attr-input-max states that we should ignore the input if it can't be parsed into a number (or a double in this case).

components/script/dom/htmlinputelement.rs, line 516 [r6] (raw file):
Not sure. Spec says that it should be a case-insensitive match to the "any" string here https://html.spec.whatwg.org/multipage/forms.html#attr-input-step.

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):
I figured that we want to store a DOMString instead of a double. The conversion is simply there to see whether it's possible to be parsed as a double, and if not, it should be assigned a default value, according to its type.

Comments from Reviewable

frewsxcv · 2016-05-03T03:23:14Z

components/script/dom/htmlinputelement.rs

+    // https://html.spec.whatwg.org/multipage/#dom-input-step
+    fn SetStep(&self, value: DOMString) {
+        let element = self.upcast::<Element>();
+        if &*value.to_lowercase() == "any" {


You could use AsciiExt::eq_ignore_ascii_case if you wanted to avoid a heap allocation here

frewsxcv · 2016-05-03T03:24:05Z

components/script/dom/htmlinputelement.rs

+            return;
+        }
+
+        let value = match self.type_() {


If you wanted to DRY this out, you could have the match just return the number

asajeffrey · 2016-05-12T17:34:20Z

Reviewed 1 of 1 files at r9.
Review status: 1 of 4 files reviewed at latest revision, 12 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 459 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

This is where https://html.spec.whatwg.org/multipage/forms.html#attr-input-max says that we should use the default value if the type is range, so reusing the value looks wrong.

Especially annoying is that we're converting a double to a UTF8 DOMString, then the JS conversion re-encodes the DOMString in UCS2, so there's a lot of conversion happening.

components/script/dom/htmlinputelement.rs, line 461 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

From what I'm reading, https://html.spec.whatwg.org/multipage/forms.html#attr-input-max states that we should ignore the input if it can't be parsed into a number (or a double in this case).

Goes and reads https://html.spec.whatwg.org/multipage/forms.html#number-state... Looks like you're right: "User agents must not allow the user to set the value to a non-empty string that is not a valid floating-point number," so discarding the value is appropriate.

components/script/dom/htmlinputelement.rs, line 516 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

Not sure. Spec says that it should be a case-insensitive match to the "any" string here https://html.spec.whatwg.org/multipage/forms.html#attr-input-step.

Sigh, can we not at least use AsciiExt::eq_ignore_ascii_case?

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

I figured that we want to store a DOMString instead of a double. The conversion is simply there to see whether it's possible to be parsed as a double, and if not, it should be assigned a default value, according to its type.

Why do we want to store the string representation for step and not for min and max?

Comments from Reviewable

KiChjang · 2016-05-12T19:25:58Z

Review status: 1 of 4 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Why do we want to store the string representation for step and not for min and max?

Because step can have the value of any, whereas min and max are purely numeric (if I understand the spec correctly).

Comments from Reviewable

highfive · 2016-05-12T19:26:00Z

New code was committed to pull request.

KiChjang · 2016-05-12T19:29:15Z

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

Because step can have the value of any, whereas min and max are purely numeric (if I understand the spec correctly).

Looks like I'm wrong... if the input is of type time, it can have a max and min value that is a time string, in which case the parse_double stuff is essentially useless (as in, we never use the returned double value at all).

Comments from Reviewable

highfive · 2016-05-12T20:09:22Z

New code was committed to pull request.

asajeffrey · 2016-05-13T13:41:49Z

Reviewed 2 of 3 files at r2, 1 of 3 files at r11, 1 of 1 files at r12, 1 of 1 files at r15.
Review status: 3 of 4 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 531 [r6] (raw file):

Previously, KiChjang (Keith Yeung) wrote…

Looks like I'm wrong... if the input is of type time, it can have a max and min value that is a time string, in which case the parse_double stuff is essentially useless (as in, we never use the returned double value at all).

Indeed, I'd expect the code for max, min and step to be the same, if step can be non-numeric then so can max and min.

Comments from Reviewable

bors-servo · 2016-05-18T19:21:09Z

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

asajeffrey · 2016-05-20T16:18:36Z

@KiChjang any news?

KiChjang · 2016-05-20T16:38:01Z

I'm in fact working on this, it's been rebased, but now i'm thinking of storing min, max and step as DOMStrings, and then add special logic for getters.

asajeffrey · 2016-05-20T16:46:40Z

Does this not get rid of most of the advantage of storing them as numbers? Could we store them as numbers where possible, and as DOMStrings otherwise? (Dates/Times might be better stored as an Instant, but we may hit spec issues around that.)

KiChjang · 2016-05-21T21:28:30Z

So, after some discussion on #whatwg, I discovered that there are parts of the spec that are not intended for implementors of HTML to read, e.g. the following paragraph:

The step attribute, if specified, must either have a value that is a valid floating-point number that parses to a number that is greater than zero, or must have a value that is an ASCII case-insensitive match for the string "any".

is intended for developers wanting to understand how to use the input element in their frontend code, and implementors (us) need not worry about the provided step value being valid floating-point. All we need to do in such a case is simply to reflect the content attribute of min, max and step, as shown in https://html.spec.whatwg.org/multipage/forms.html#dom-input-min, i.e. the behaviour as witnessed in FF and CR are correct.

highfive · 2016-05-21T21:34:57Z

New code was committed to pull request.

highfive · 2016-05-21T21:41:15Z

New code was committed to pull request.

asajeffrey · 2016-05-23T17:52:27Z

A couple of minor nits, at this point you can r=me after fixing and squashing.

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 1 of 3 files at r17, 1 of 2 files at r21, 1 of 1 files at r23, 1 of 1 files at r24.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

components/style/attr.rs, line 119 [r24] (raw file):

        let atoms =
            split_html_space_chars(&tokens).map(Atom::from)
                                           .fold(vec![], |mut acc, atom| {

The only change here is white space?

components/style/attr.rs, line 193 [r24] (raw file):

    // https://html.spec.whatwg.org/multipage/#limited-to-numbers-greater-than-zero
    pub fn from_limited_double(string: DOMString, default: f64) -> AttrValue {

Is this used anywhere?

Comments from Reviewable

bors-servo · 2016-05-24T09:59:58Z

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

KiChjang · 2016-05-24T12:40:34Z

@bors-servo r=asajeffrey

bors-servo · 2016-05-24T12:40:35Z

📌 Commit aad8036 has been approved by asajeffrey

bors-servo · 2016-05-24T13:09:21Z

⌛ Testing commit aad8036 with merge ac1e4ca...


        Auto merge of #10962 - KiChjang:input-attr-parse, r=asajeffrey

Parse HTMLInputElement attributes Fixes #10491.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10962)

bors-servo · 2016-05-24T13:48:45Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-24T13:48:47Z

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to &#34;&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to &#34; foo &#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to &#34;//site.example/path???@#l&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to &#34;\\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f &#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to undefined followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to 7 followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to 1.5 followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAt</span><span class="stdout">tribute() to true followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to false followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to object &#34;[object Object]&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to NaN followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to Infinity followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to -Infinity followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to &#34;\\0&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to null followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to object &#34;test-toString&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: setAttribute() to object &#34;test-valueOf&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to &#34;&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to &#34; foo &#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to &#34;//site.example/path???@#l&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to &#34;\\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f &#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to undefined followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to 7 followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to 1.5 followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to true followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to false followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to object &#34;[object Object]&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to NaN followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to Infinity followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to -Infinity followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to &#34;\\0&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to null followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to object &#34;test-toString&#34; followed by IDL get

  ▶ Unexpected subtest result in /html/dom/reflection-forms.html:
  └ PASS [expected FAIL] input.src: IDL set to object &#34;test-valueOf&#34; followed by IDL get

asajeffrey · 2016-05-24T13:58:15Z

Well, that's a good failure to have, lots of tests passing which were failing before.


        Parse HTMLInputElement attributes

highfive · 2016-05-24T14:12:22Z

New code was committed to pull request.

KiChjang · 2016-05-24T14:12:28Z

@bors-servo r=asajeffrey

bors-servo · 2016-05-24T14:12:29Z

📌 Commit c93ed39 has been approved by asajeffrey

bors-servo · 2016-05-24T14:42:24Z

⌛ Testing commit c93ed39 with merge 8c4929a...


        Auto merge of #10962 - KiChjang:input-attr-parse, r=asajeffrey

Parse HTMLInputElement attributes Fixes #10491.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10962)

bors-servo · 2016-05-24T15:33:39Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned asajeffrey May 2, 2016

highfive added the S-awaiting-review label May 2, 2016

jdm reviewed May 2, 2016
View changes

KiChjang force-pushed the KiChjang:input-attr-parse branch 2 times, most recently from 651bd7b to e9cc748 May 3, 2016

frewsxcv reviewed May 3, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

return;

}

let value = match self.type_() {

This comment has been minimized.

Sign in to view

Copy link

frewsxcv May 3, 2016
Member

If you wanted to DRY this out, you could have the match just return the number

asajeffrey added S-awaiting-answer and removed S-awaiting-review labels May 12, 2016

KiChjang force-pushed the KiChjang:input-attr-parse branch from e9cc748 to 3f8e7ef May 12, 2016

highfive added the S-awaiting-review label May 12, 2016

KiChjang removed the S-awaiting-answer label May 12, 2016

KiChjang force-pushed the KiChjang:input-attr-parse branch from 3f8e7ef to 8e96e7c May 12, 2016

highfive added the S-needs-rebase label May 18, 2016

asajeffrey added S-awaiting-answer and removed S-awaiting-review labels May 20, 2016

highfive added the S-awaiting-review label May 21, 2016

KiChjang removed the S-needs-rebase label May 21, 2016

KiChjang force-pushed the KiChjang:input-attr-parse branch from 1b7e172 to f6b308b May 21, 2016

KiChjang removed the S-awaiting-answer label May 21, 2016

KiChjang force-pushed the KiChjang:input-attr-parse branch from f6b308b to f8ea98c May 21, 2016

KiChjang force-pushed the KiChjang:input-attr-parse branch 2 times, most recently from 21c3f85 to aad8036 May 24, 2016

Parse HTMLInputElement attributes

Loading status checks…

c93ed39

KiChjang force-pushed the KiChjang:input-attr-parse branch from aad8036 to c93ed39 May 24, 2016

bors-servo merged commit c93ed39 into servo:master May 24, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request May 24, 2016

Report use statements that use {} with only one entry #11329

Merged

1 of 2 tasks complete

KiChjang deleted the KiChjang:input-attr-parse branch Dec 20, 2016

servo / servo

Parse HTMLInputElement attributes #10962

Parse HTMLInputElement attributes #10962

KiChjang commented May 2, 2016 •

edited by larsbergstrom

highfive commented May 2, 2016

highfive commented May 2, 2016

This comment has been minimized.

asajeffrey commented May 2, 2016

KiChjang commented May 3, 2016

This comment has been minimized.

This comment has been minimized.

asajeffrey commented May 12, 2016

KiChjang commented May 12, 2016

highfive commented May 12, 2016

KiChjang commented May 12, 2016

highfive commented May 12, 2016

asajeffrey commented May 13, 2016

bors-servo commented May 18, 2016

asajeffrey commented May 20, 2016

KiChjang commented May 20, 2016

asajeffrey commented May 20, 2016

KiChjang commented May 21, 2016 •

edited

highfive commented May 21, 2016

highfive commented May 21, 2016

asajeffrey commented May 23, 2016

bors-servo commented May 24, 2016

KiChjang commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

highfive commented May 24, 2016

asajeffrey commented May 24, 2016

highfive commented May 24, 2016

KiChjang commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

servo / servo

Join GitHub today

Parse HTMLInputElement attributes #10962

Parse HTMLInputElement attributes #10962

Conversation

KiChjang commented May 2, 2016 • edited by larsbergstrom

highfive commented May 2, 2016

highfive commented May 2, 2016

This comment has been minimized.

jdm May 2, 2016

asajeffrey commented May 2, 2016

KiChjang commented May 3, 2016

This comment has been minimized.

frewsxcv May 3, 2016

This comment has been minimized.

frewsxcv May 3, 2016

asajeffrey commented May 12, 2016

KiChjang commented May 12, 2016

highfive commented May 12, 2016

KiChjang commented May 12, 2016

highfive commented May 12, 2016

asajeffrey commented May 13, 2016

bors-servo commented May 18, 2016

asajeffrey commented May 20, 2016

KiChjang commented May 20, 2016

asajeffrey commented May 20, 2016

KiChjang commented May 21, 2016 • edited

highfive commented May 21, 2016

highfive commented May 21, 2016

asajeffrey commented May 23, 2016

bors-servo commented May 24, 2016

KiChjang commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

highfive commented May 24, 2016

asajeffrey commented May 24, 2016

highfive commented May 24, 2016

KiChjang commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

KiChjang commented May 2, 2016 •

edited by larsbergstrom

KiChjang commented May 21, 2016 •

edited