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

Expand InputType to cover all possible types #19471

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
7 participants
@jonleighton
Contributor

jonleighton commented Dec 3, 2017

This came out of a conversation with nox in IRC:
https://mozilla.logbot.info/servo/20171201#c13946454-c13946594

The code I was working on which motivated this change is here:
#19461

Previously, InputType::Text was used to represent several different
values of the type attribute on an input element.

If an input element doesn't have a type attribute, or its type attribute
doesn't contain a recognised value, then the input's type defaults to
"text".

Before this change, there were a number of checks in the code which
directly looked at the type attribute. If those checks matched against
the value "text", then they were potentially buggy, since an input with
type=invalid should also behave like an input with type=text.

Rather than have every conditional which cares about the input type also
have to deal with invalid input types, we can convert the type attribute
to an InputType enum once, and then match against the enum.

A secondary benefit is that the compiler can tell us whether we've
missed branches in a match expression. While working on this I
discovered that the HTMLInputElement::value_mode() method misses a case
for inputs with type=hidden (this resulted in a failing WPT test
passing).

I've also implemented the Default trait for InputType, so we now only
have one place in the code which knows that InputType::Text is the
default, where previously there were several.


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Dec 3, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlelement.rs, components/script/dom/radionodelist.rs
  • @fitzgen: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlelement.rs, components/script/dom/radionodelist.rs
  • @KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlelement.rs, components/script/dom/radionodelist.rs
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 3, 2017

r? @nox

@highfive highfive assigned nox and unassigned mbrubeck Dec 3, 2017

InputCheckbox,
InputRadio,
InputPassword
pub enum InputType {

This comment has been minimized.

@jonleighton

jonleighton Dec 3, 2017

Contributor

I took the liberty of removing the Input prefix from the enum members, as this seemed redundant. Let me know if that's not OK.

&atom!("time") => InputType::Time,
&atom!("url") => InputType::Url,
&atom!("week") => InputType::Week,
v if *v == Atom::from("range") => InputType::Range,

This comment has been minimized.

@jonleighton

jonleighton Dec 3, 2017

Contributor

I couldn't get it to compile without this hack. The compiler said that "range" didn't match the atom! macro. I'm not sure what the right thing to do here is, but I suspect this isn't it! Please let me know.

This comment has been minimized.

@KiChjang

KiChjang Dec 4, 2017

Member

You'll need to add the "range" string to this file in order for the atom! macro to work with it.

This comment has been minimized.

@jonleighton

jonleighton Dec 4, 2017

Contributor

Thanks. Seems like the way atoms work could really use some documentation, I've had a lot of questions about this area of the code.

Checkbox,
Color,
Date,
Datetime,

This comment has been minimized.

@jonleighton

jonleighton Dec 3, 2017

Contributor

Do we really need this? datetime is an obsolete value and is not in the spec. It's also unsupported in Firefox. Doesn't seem like it needs to exist in Servo?

This comment has been minimized.

@jdm

jdm Dec 6, 2017

Member

Agreed. We can file a followup issue.

}
}
pub fn input_type(&self) -> InputType {

This comment has been minimized.

@KiChjang

KiChjang Dec 4, 2017

Member

I don't actually feel like this is THAT much of an improvement, since this essentially only saves us from typing .get. In fact, I think this may even hurt performance since we have to go through another function call... but since you've already made the change everywhere to use input_type(), the least we can do to mitigate this is to add a #[inline] annotation above.

This comment has been minimized.

@jonleighton

jonleighton Dec 4, 2017

Contributor

Thanks, done. Note that there need to be some public way to get the InputType from outside of the crate, which is part of the reason that I added this function (the other part of the reason is indeed that I think it's slightly nicer to not have to repeat input_type.get() everywhere.

@jonleighton jonleighton force-pushed the jonleighton:input-type branch 2 times, most recently from c3f9e0b to dded910 Dec 4, 2017

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 5, 2017

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

@jonleighton jonleighton force-pushed the jonleighton:input-type branch 2 times, most recently from 903033d to 9f7801d Dec 5, 2017

@jdm jdm assigned jdm and unassigned nox Dec 6, 2017

@jdm

jdm approved these changes Dec 6, 2017

Checkbox,
Color,
Date,
Datetime,

This comment has been minimized.

@jdm

jdm Dec 6, 2017

Member

Agreed. We can file a followup issue.

@jdm

This comment has been minimized.

Member

jdm commented Dec 6, 2017

@bors-servo: r+
This is a great improvement! Thanks!

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

📌 Commit 9f7801d has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

⌛️ Testing commit 9f7801d with merge 44a1e98...

bors-servo added a commit that referenced this pull request Dec 6, 2017

Auto merge of #19471 - jonleighton:input-type, r=jdm
Expand InputType to cover all possible types

This came out of a conversation with nox in IRC:
https://mozilla.logbot.info/servo/20171201#c13946454-c13946594

The code I was working on which motivated this change is here:
#19461

Previously, InputType::Text was used to represent several different
values of the type attribute on an input element.

If an input element doesn't have a type attribute, or its type attribute
doesn't contain a recognised value, then the input's type defaults to
"text".

Before this change, there were a number of checks in the code which
directly looked at the type attribute. If those checks matched against
the value "text", then they were potentially buggy, since an input with
type=invalid should also behave like an input with type=text.

Rather than have every conditional which cares about the input type also
have to deal with invalid input types, we can convert the type attribute
to an InputType enum once, and then match against the enum.

A secondary benefit is that the compiler can tell us whether we've
missed branches in a match expression. While working on this I
discovered that the HTMLInputElement::value_mode() method misses a case
for inputs with type=hidden (this resulted in a failing WPT test
passing).

I've also implemented the Default trait for InputType, so we now only
have one place in the code which knows that InputType::Text is the
default, where previously there were several.

<!-- 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/19471)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

💔 Test failed - linux-rel-wpt

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 6, 2017

I'll take a look at those failures

Expand InputType to cover all possible types
This came out of a conversation with nox in IRC:
https://mozilla.logbot.info/servo/20171201#c13946454-c13946594

The code I was working on which motivated this change is here:
#19461

Previously, InputType::Text was used to represent several different
values of the type attribute on an input element.

If an input element doesn't have a type attribute, or its type attribute
doesn't contain a recognised value, then the input's type defaults to
"text".

Before this change, there were a number of checks in the code which
directly looked at the type attribute. If those checks matched against
the value "text", then they were potentially buggy, since an input with
type=invalid should also behave like an input with type=text.

Rather than have every conditional which cares about the input type also
have to deal with invalid input types, we can convert the type attribute
to an InputType enum once, and then match against the enum.

A secondary benefit is that the compiler can tell us whether we've
missed branches in a match expression. While working on this I
discovered that the HTMLInputElement::value_mode() method misses a case
for inputs with type=hidden (this resulted in a failing WPT test
passing).

I've also implemented the Default trait for InputType, so we now only
have one place in the code which knows that InputType::Text is the
default, where previously there were several.

@jonleighton jonleighton force-pushed the jonleighton:input-type branch from 9f7801d to 05bfb8d Dec 6, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 6, 2017

Fixed. I needed to make InputType::from case-insensitive, so that <input type=password> matches the same as <input type=PASSWORD>.

@jdm

This comment has been minimized.

Member

jdm commented Dec 6, 2017

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

📌 Commit 05bfb8d has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

⌛️ Testing commit 05bfb8d with merge 6a6da9c...

bors-servo added a commit that referenced this pull request Dec 6, 2017

Auto merge of #19471 - jonleighton:input-type, r=jdm
Expand InputType to cover all possible types

This came out of a conversation with nox in IRC:
https://mozilla.logbot.info/servo/20171201#c13946454-c13946594

The code I was working on which motivated this change is here:
#19461

Previously, InputType::Text was used to represent several different
values of the type attribute on an input element.

If an input element doesn't have a type attribute, or its type attribute
doesn't contain a recognised value, then the input's type defaults to
"text".

Before this change, there were a number of checks in the code which
directly looked at the type attribute. If those checks matched against
the value "text", then they were potentially buggy, since an input with
type=invalid should also behave like an input with type=text.

Rather than have every conditional which cares about the input type also
have to deal with invalid input types, we can convert the type attribute
to an InputType enum once, and then match against the enum.

A secondary benefit is that the compiler can tell us whether we've
missed branches in a match expression. While working on this I
discovered that the HTMLInputElement::value_mode() method misses a case
for inputs with type=hidden (this resulted in a failing WPT test
passing).

I've also implemented the Default trait for InputType, so we now only
have one place in the code which knows that InputType::Text is the
default, where previously there were several.

<!-- 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/19471)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 6, 2017

@bors-servo bors-servo merged commit 05bfb8d into servo:master Dec 6, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

jonleighton added a commit to jonleighton/servo that referenced this pull request Dec 7, 2017

@jonleighton jonleighton referenced this pull request Dec 7, 2017

Merged

Remove support for <input type=datetime> #19509

3 of 5 tasks complete

bors-servo added a commit that referenced this pull request Dec 7, 2017

Auto merge of #19509 - jonleighton:remove-input-type-datetime, r=jdm
Remove support for <input type=datetime>

It has been removed from the spec: whatwg/html#336

See also #19471 (review)

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] 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/19509)
<!-- Reviewable:end -->

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 8, 2017

servo: Merge #19509 - Remove support for <input type=datetime> (from …
…jonleighton:remove-input-type-datetime); r=jdm

It has been removed from the spec: whatwg/html#336

See also servo/servo#19471 (review)

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 8e3056d0cc7caebc218d51373b3aa0ccd331fa20

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 76bd32c1d97be56bee2675857fcb5b20f2ed2d42

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Dec 8, 2017

servo: Merge #19509 - Remove support for <input type=datetime> (from …
…jonleighton:remove-input-type-datetime); r=jdm

It has been removed from the spec: whatwg/html#336

See also servo/servo#19471 (review)

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 8e3056d0cc7caebc218d51373b3aa0ccd331fa20

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Dec 19, 2017

servo: Merge #19509 - Remove support for <input type=datetime> (from …
…jonleighton:remove-input-type-datetime); r=jdm

It has been removed from the spec: whatwg/html#336

See also servo/servo#19471 (review)

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 8e3056d0cc7caebc218d51373b3aa0ccd331fa20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment