Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement HTMLInputElement stepUp, stepDown,valueAsDate, valueAsNumber, list #25386

Closed
pshaughn opened this issue Dec 24, 2019 · 7 comments
Closed

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 24, 2019

https://html.spec.whatwg.org/multipage/input.html#htmlinputelement defines these and corresponding WPT tests under html/semantics/forms/the-input-element test them.

Servo also has commented-out IDL for valueHigh and valueLow; these were at one time proposed for use in <input type="range" multiple>, but the spec now says multiple doesn't apply to range and they can be removed entirely.

@pshaughn pshaughn changed the title implement HTMLInputElement stepUp, stepDown,valueAsDate, valueAsNumber implement HTMLInputElement stepUp, stepDown,valueAsDate, valueAsNumber, list Dec 25, 2019
@CYBAI CYBAI added the A-content/dom label Dec 25, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 25, 2019
@pshaughn pshaughn self-assigned this Dec 26, 2019
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 26, 2019

I need guidance about something to make sure I'm not reinventing a wheel or adding a needless dependency: Several of these algorithms want to convert a parsed time to time-since-1970 (in milliseconds and in months), and I'm guessing something Servo already has a dependency on can do that, but I don't know what.

Dates also have another wrinkle; https://html.spec.whatwg.org/multipage/input.html#time-state-(type=time):concept-input-value-string-number wants to be able to invoke "parse a time string" on a DOMString, which isn't defined at all, and https://html.spec.whatwg.org/multipage/input.html#date-state-(type=date):concept-input-value-string-number wants to invoke "parse a date string", which is defined but isn't public. DOMString is going to need some new public methods to accommodate these (and "parse a time string" is going to be a bit complicated, since a "time component" can have either two or three fields instead of the exact three a "date component" has).

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 27, 2019

stepUp, stepDown, and valueAsNumber are sort of working for non-chronological values in pshaughn@494dd76 but not in PR-worthy shape yet.

@Eijebong's idea in cb87025 got past a problem that seems like it could be a spec issue: https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token step 8 defines no order-of-operations for appending attributes, and browser/WPT behavior seems to assume all attributes are appended "simultaneously" and then can see each other during their setter side effects, so that for example <input type="range" min=1 value=2 max=3> sees both min=1 and max=3 while it's setting value to 2.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 28, 2019

pshaughn@eab3398 is much closer on getting the non-chrono steppable types (number and range) working.

I am having one problem where if min=0.1 and step=0.1 I end up resolving the stepping to a string value of 0.6000000000000001 (fourteen zeroes) instead of the test-expected 0.6. I'm not sure yet if this is because of:

  • Rust f64 significand rounding direction differing from Javascript
  • Rust f64 stringification differing from Javascript
  • a subtle algorithmic error that's letting in a rounding error that a better algorithm would prevent
  • a test expecting floating-point equality where the reality of floating-point handling means no such equality is guaranteed
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 28, 2019

On the chronological front, I see script is already importing https://docs.rs/chrono/0.4.10/chrono/ for week-of-year computations and media-fragment timestamp parsing; this crate seems like it probably includes what we need for the necessary back-and-forth between milliseconds and datetimes to make chronological value stepping and valueAsDate work.

bors-servo added a commit that referenced this issue Dec 30, 2019
stepUp, stepDown, and valueAsNumber work [draft]

<!-- Please describe your changes on the following line: -->
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.
#25386 still needs valueAsDate and list; I'll probably update that within this PR since it's going to have changes in common with this.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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 progress #25386

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 31, 2019
stepUp, stepDown, and valueAsNumber work [draft]

<!-- Please describe your changes on the following line: -->
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.
#25386 still needs valueAsDate and list; I'll probably update that within this PR since it's going to have changes in common with this.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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 progress #25386

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 31, 2019

valueAsDate wasn't hard to get to compile once I figured out how to get Date objects in and out of jsapi code, but the only tests in WPT about it are for error conditions and basic IDL binding status. I'm going to have to add more tests to know it actually runs. (web-platform-tests/wpt#20960)

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 31, 2019

pshaughn@6eb8e2a I've added a test for valueAsDate; do I just check it in here and let upstreaming to WPT happen that way, or should I make a PR at wpt/wpt about it?

bors-servo added a commit that referenced this issue Dec 31, 2019
stepUp, stepDown, and valueAsNumber work [draft]

<!-- Please describe your changes on the following line: -->
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.
#25386 still needs valueAsDate and list; I'll probably update that within this PR since it's going to have changes in common with this.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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 progress #25386

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 31, 2019
stepUp, stepDown, and valueAsNumber work [draft]

<!-- Please describe your changes on the following line: -->
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.
#25386 still needs valueAsDate and list; I'll probably update that within this PR since it's going to have changes in common with this.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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 progress #25386

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 31, 2019
stepUp, stepDown, and valueAsNumber work [draft]

<!-- Please describe your changes on the following line: -->
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.
#25386 still needs valueAsDate and list; I'll probably update that within this PR since it's going to have changes in common with this.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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 progress #25386

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 1, 2020
make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2020

Upstreaming as part of PRs that do the implementation work is the preferred mechanism.

bors-servo added a commit that referenced this issue Jan 10, 2020
make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 11, 2020
make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 11, 2020
make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 11, 2020
make stepUp, stepDown, valueAsNumber, valueAsDate, and list work in HTMLInputElement

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
web-platform-test failures automation moved this from To do to Done Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.