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

[meta] Implement value sanitizations for different input modes #19172

Closed
KiChjang opened this issue Nov 10, 2017 · 39 comments
Closed

[meta] Implement value sanitizations for different input modes #19172

KiChjang opened this issue Nov 10, 2017 · 39 comments

Comments

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Nov 10, 2017

#18262 laid the groundwork for this, but we're still missing value sanitizations for:

All of the missing algorithms above are expected to be implemented in this function:

// https://html.spec.whatwg.org/multipage/#value-sanitization-algorithm
fn sanitize_value(&self, value: &mut DOMString) {
match self.input_type() {
InputType::Text | InputType::Search | InputType::Tel | InputType::Password => {
value.strip_newlines();
},
InputType::Url => {
value.strip_newlines();
value.strip_leading_and_trailing_ascii_whitespace();
},
InputType::Date => {
if !value.is_valid_date_string() {
value.clear();
}
},
InputType::Month => {
if !value.is_valid_month_string() {
value.clear();
}
},
InputType::Week => {
if !value.is_valid_week_string() {
value.clear();
}
},
InputType::Color => {
let is_valid = {
let mut chars = value.chars();
if value.len() == 7 && chars.next() == Some('#') {
chars.all(|c| c.is_digit(16))
} else {
false
}
};
if is_valid {
value.make_ascii_lowercase();
} else {
*value = "#000000".into();
}
},
InputType::Time => {
if !value.is_valid_time_string() {
value.clear();
}
},
InputType::DatetimeLocal => {
if value
.convert_valid_normalized_local_date_and_time_string()
.is_err()
{
value.clear();
}
},
InputType::Number => {
if !value.is_valid_floating_point_number_string() {
value.clear();
}
},
// https://html.spec.whatwg.org/multipage/#range-state-(type=range):value-sanitization-algorithm
InputType::Range => {
value.set_best_representation_of_the_floating_point_number();
},
_ => (),
}
}

The implementation for these value sanitizations are given by the spec, which I have linked accordingly. When each of these are implemented, some subtests under tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html should be expected to PASS instead. Some other tests under the same directory may also pass as a result of a correct implementation.

If you want to help out with this, please comment on this issue or reach out on IRC (#servo in irc.mozilla.org), and we'll find one which is interesting to you; or, you can also file an issue and mention this one, and I'll get it assigned to you.

@highfive
Copy link

@highfive highfive commented Nov 10, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 10, 2017

@highfive: assign me

@highfive
Copy link

@highfive highfive commented Nov 10, 2017

Hey @tigercosmos! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Nov 10, 2017
@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 10, 2017

@KiChjang Can I divide it to several parts and solve them one by one? Maybe two or three?

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 10, 2017

@tigercosmos Sure! Just comment and make a list of the ones that you'd like to solve.

@KiChjang KiChjang removed the C-assigned label Nov 10, 2017
@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 10, 2017

Keeping this open for other potential contributors.

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 11, 2017

Seems min max step attributes have not implemented?
Where I guess it should be in:

fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
self.super_type().unwrap().attribute_mutated(attr, mutation);
match attr.local_name() {

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 11, 2017

That is incorrect. I can find them under the HTMLInputElementMethods trait implementation.

// https://html.spec.whatwg.org/multipage/#dom-input-max
make_getter!(Max, "max");
// https://html.spec.whatwg.org/multipage/#dom-input-max
make_setter!(SetMax, "max");

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 11, 2017

That just get the value. But we need to do something? e.g. min should be number.

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 11, 2017

I assume the reason why you're asking is because the spec mentions max, min and step attributes should be valid date strings? You can safely ignore that part - the spec is written with different target audiences in mind: HTML authors (which is to say programmers who write HTML) and HTML implementors (us). Those requirements are for HTML authors, since they do not mention how user agents (browsers) should act when they are invalid.

In addition, the spec defines the value sanitization algorithm as "if the value of the element is not a valid date string, then set it to the empty string instead". The max, min and step attribute stuff is not part of the value sanitization algorithm.

The valid date strings algorithm is what you will need to implement, and I think the appropriate place to put that algorithm would be in components/style/attr.rs.

@Eijebong
Copy link
Member

@Eijebong Eijebong commented Nov 21, 2017

Going to do the color one

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 23, 2017

I believe @Eijebong is also working on the number and range type inputs.

@Eijebong
Copy link
Member

@Eijebong Eijebong commented Nov 23, 2017

Right, sorry I forgot to put it there.

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Nov 23, 2017

@tigercosmos I haven't heard back from you for a while, do you need any help? Any questions?

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 23, 2017

@KiChjang I am stuck in other issues (19280) ... and preparing for midterm exams recently.
I have deleted my declaration before. I will be back later (If this issue is still not fixed)

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 25, 2017

I am back, and working on Date one

@SWW13
Copy link
Contributor

@SWW13 SWW13 commented Nov 25, 2017

I'm going to work on the number and range validation.

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 25, 2017

@SWW13 that one have been picked by @Eijebong

@SWW13
Copy link
Contributor

@SWW13 SWW13 commented Nov 25, 2017

Oh, missed that. @KiChjang Could you update the initial issue and name the selected parts?

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 25, 2017

I believe these are unassigned:

  • Month type inputs
  • Week type inputs
  • Time type inputs
  • Date-time type inputs

But some would be related to Date one, so I suggest choose those after Date done.

@SWW13
Copy link
Contributor

@SWW13 SWW13 commented Nov 25, 2017

Then I'll work on the time validation if you don't mind @tigercosmos? That should not be related to the Date input.

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Nov 26, 2017

That would be great!
Good luck :)

@SWW13 SWW13 mentioned this issue Nov 26, 2017
5 of 5 tasks complete
bors-servo added a commit that referenced this issue Nov 27, 2017
Implemented sanitize_value for time input

Implemented value sanitization for `<input type=time/>`.
The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string.

---

The following test results look expected to me, but I'm not sure:
```
   Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from time to text
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to search
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to tel
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to url
  │   → assert_equals: input.value should be foobar after change of state expected "foobar" but got ""
  │ FAIL [expected PASS] change state from time to password
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9

   Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from color to time
  │   → assert_equals: input.value should be #000000 after change of state expected "#000000" but got ""
  │
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9
```

All other tests do now `PASS` instead of `FAIL`.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix *part of* #19172
- [x] There are tests for these changes
  - [ ] All tests `PASS`

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19379)
<!-- Reviewable:end -->
@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Dec 12, 2017

I'd rather not, most of the string parsing/validation logic is located in str.rs, I'd rather not move them around unnecessarily.

@tigercosmos tigercosmos mentioned this issue Dec 14, 2017
3 of 3 tasks complete
@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Dec 14, 2017

@Eijebong Are you still working on number and range type inputs?

bors-servo added a commit that referenced this issue Dec 17, 2017
implement valid week string

<!-- Please describe your changes on the following line: -->
implement valid week string
part of #19172

r? @KiChjang

---
<!-- 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] There are tests for these changes

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19559)
<!-- Reviewable:end -->
@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Dec 19, 2017

I just found that @SWW13' s time implement is not completed.
both parse a time string and parse a time component are not implemented.
I will do it, and then I can continue the other parts.

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Dec 22, 2017

Date-time type inputs has opened a PR #19602

bors-servo added a commit that referenced this issue Jan 7, 2018
implement valid DatetimeLocal input

<!-- Please describe your changes on the following line: -->
implement valid Date time Local input

part of #19172

---
<!-- 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 #19587 fix #19603(github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19602)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 7, 2018
implement valid DatetimeLocal input

<!-- Please describe your changes on the following line: -->
implement valid Date time Local input

part of #19172

---
<!-- 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 #19587 fix #19603(github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19602)
<!-- Reviewable:end -->
@NLincoln
Copy link
Contributor

@NLincoln NLincoln commented Jan 9, 2018

I've started working on basic support for type=number. I have all of the pre-existing tests passing, just need to update the test manifest (I might be asking about that on IRC tonight actually... there were some errors when I ran it this morning). Once that's done I'll open a PR.

My code is here. I used the standard f64 parser, just with a few changes where the browser spec is stricter than what rust allows. Is this approach Ok?

Currently I don't handle the min, max, or step attributes. I figured I'd reserve those for a future PR.

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Jan 9, 2018

@NLincoln this issue doesn't need to handle min, max, or step. I think you can open PR and that would be easier to discuss about the code.

@NLincoln NLincoln mentioned this issue Jan 9, 2018
4 of 4 tasks complete
@NLincoln
Copy link
Contributor

@NLincoln NLincoln commented Jan 9, 2018

👍 I just opened one.

bors-servo added a commit that referenced this issue Jan 10, 2018
…r=KiChjang

number input type validations

I used rust's builtin float parser to implement this. Rust's parser is more permissive than what browsers support (in this case), so I added some code to handle those edge cases.

This passes all the prewritten test cases locally, but I fell asleep last night before updating the manifests 😅

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes are part of #19172

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19730)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 10, 2018
…r=KiChjang

number input type validations

I used rust's builtin float parser to implement this. Rust's parser is more permissive than what browsers support (in this case), so I added some code to handle those edge cases.

This passes all the prewritten test cases locally, but I fell asleep last night before updating the manifests 😅

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes are part of #19172

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19730)
<!-- Reviewable:end -->
@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Jan 10, 2018

let me finish the last one range type inputs
@highfive: assign me

@highfive highfive added the C-assigned label Jan 10, 2018
@highfive
Copy link

@highfive highfive commented Jan 10, 2018

Hey @tigercosmos! Thanks for your interest in working on this issue. It's now assigned to you!

@tigercosmos tigercosmos mentioned this issue Jan 13, 2018
3 of 5 tasks complete
@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Jan 13, 2018

I have opened the last one PR in this issue.
Once #19761 finished, this will closed.

bors-servo added a commit that referenced this issue Jan 17, 2018
implement range input sanitization

<!-- Please describe your changes on the following line: -->
implement range input sanitation.
Since there is no `min`, `max`, `step` implementation currently, this should be continued in the future.

r? KiChjang

---
<!-- 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 #19172 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19761)
<!-- Reviewable:end -->
@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Sep 25, 2018

Is E-Mail sanitization missing for this meta issue?

Ref: https://html.spec.whatwg.org/multipage/input.html#e-mail-state-(type=email)

@tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Sep 25, 2018

Yes, let's reopen the issue.

@tigercosmos tigercosmos reopened this Sep 25, 2018
@tigercosmos tigercosmos removed the C-assigned label Sep 25, 2018
@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Sep 25, 2018

I've instead filed #21810, which contains other things that also needs attention for value sanitization.

@KiChjang KiChjang closed this Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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