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

Handle cases where selection API doesn't apply #19461

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
6 participants
@jonleighton
Contributor

jonleighton commented Dec 2, 2017

The selection API only applies to certain types:

https://html.spec.whatwg.org/multipage/#do-not-apply

This commit ensures that we handle that correctly.

Some notes:

  1. TextControl::set_dom_selection_direction now calls
    set_selection_range(), which means that setting selectionDirection will
    now fire a selection event, as it should per the spec.

  2. There is a test for the firing of the select event in
    tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
    however the test did not run due to this syntax error:

    (pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"

    This happens due to the us of the "for (const foo of ...)" construct.
    Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
    this should actually work, so it's somewhat unsatisfying to have to
    change the test.

  3. I removed tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application-textarea.html
    because it doesn't seem to add any extra value - the selection API
    always applies to textarea elements, and the API is tested elsewhere.

  4. If an <input>'s type is unset, it defaults to a text, and the
    selection API applies. Also, if an <input>'s type is set to an
    invalid value, it defaults to a text too. This second case doesn't
    currently work, and I'll need to do more restructuring of the code in
    a future commit. See discussion with nox in IRC:
    https://mozilla.logbot.info/servo/20171201#c13946454-c13946594


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Dec 2, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmltextareaelement.rs, components/script/dom/textcontrol.rs, components/script/dom/webidls/HTMLInputElement.webidl, components/script/dom/webidls/HTMLTextAreaElement.webidl, components/script/dom/htmlinputelement.rs and 1 more
  • @fitzgen: components/script/dom/htmltextareaelement.rs, components/script/dom/textcontrol.rs, components/script/dom/webidls/HTMLInputElement.webidl, components/script/dom/webidls/HTMLTextAreaElement.webidl, components/script/dom/htmlinputelement.rs and 1 more
  • @KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/textcontrol.rs, components/script/dom/webidls/HTMLInputElement.webidl, components/script/dom/webidls/HTMLTextAreaElement.webidl, components/script/dom/htmlinputelement.rs and 1 more
@nox

This comment has been minimized.

Member

nox commented Dec 2, 2017

Please remove my name from the commit message, I get a notification every time it's pushed on GH (just make it nox instead of @nox to fix that issue).

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-4 branch from 6688224 to b9377ec Dec 2, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 2, 2017

Sorry about that, done. Do you want to still be assigned? Not sure if you want to be involved with this, @KiChjang has reviewed several of my previous patches but this obviously relates somewhat to what we discussed in IRC.

(Also not sure if you got assigned because I mentioned you or whether it was just luck of the draw, I don't really know how the bot works, but anyway, feel free to tell me what you prefer.)

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

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:
servo#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

This comment has been minimized.

Contributor

jonleighton commented Dec 3, 2017

I've now submitted #19471 which cleans up the handling of the type attribute. If that gets merged, I can make this change a bit cleaner and the tests for <input type=invalidvalue> will pass.

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

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:
servo#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 added a commit to jonleighton/servo that referenced this pull request Dec 4, 2017

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:
servo#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 added a commit to jonleighton/servo that referenced this pull request Dec 5, 2017

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:
servo#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 added a commit to jonleighton/servo that referenced this pull request Dec 5, 2017

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:
servo#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.

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

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

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:
servo#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.

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

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

servo: Merge #19471 - Expand InputType to cover all possible types (f…
…rom jonleighton:input-type); r=jdm

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:
servo/servo#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.

Source-Repo: https://github.com/servo/servo
Source-Revision: 6a6da9c2a4805d28365961c6ecd1e8dc7559b0b1

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

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-4 branch from b9377ec to a3da184 Dec 7, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 7, 2017

@highfive highfive assigned KiChjang and unassigned nox Dec 7, 2017

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

servo: Merge #19471 - Expand InputType to cover all possible types (f…
…rom jonleighton:input-type); r=jdm

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:
servo/servo#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.

Source-Repo: https://github.com/servo/servo
Source-Revision: 6a6da9c2a4805d28365961c6ecd1e8dc7559b0b1
@KiChjang

Great work! I don't see much problems with the code, but I have some comments about the tests.

@@ -408,6 +408,17 @@ impl TextControl for HTMLInputElement {
fn textinput(&self) -> &DomRefCell<TextInput<ScriptToConstellationChan>> {
&self.textinput
}
fn selection_api_applies(&self) -> bool {

This comment has been minimized.

@KiChjang

KiChjang Dec 7, 2017

Member

Can you link to the relevant section of the spec that defines this behaviour?

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

Yep

[input type password: setRangeText()]
expected: FAIL

This comment has been minimized.

@KiChjang

KiChjang Dec 7, 2017

Member

Why are there so many expected test failures?

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

These are just things that haven't yet been implemented:

  • select() method
  • setRangeText() method
  • Code to avoid firing the select event when the selection hasn't actually moved
const elLabel = el.localName === "textarea" ? "textarea" : "input type " + el.type;
for (const action of actions) {
actions.forEach((action) => {

This comment has been minimized.

@KiChjang

KiChjang Dec 7, 2017

Member

Why the change?

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

This is explained in the commit message:

There is a test for the firing of the select event in
tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
however the test did not run due to this syntax error:

(pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"

This happens due to the us of the "for (const foo of ...)" construct.
Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
this should actually work, so it's somewhat unsatisfying to have to
change the test.

Definitely not ideal to have to change this, so I'm open to other suggestions! But without changing this, the changes here are not fully tested (specifically, setting selectionDirection will now call set_selection_range and fire the select event, and this test file verifies that behaviour).

This comment has been minimized.

@KiChjang

KiChjang Dec 8, 2017

Member

Does let instead of const work? If not, then this looks like it's a problem with our implementation, and not the tests. To reiterate: these tests are shared across all other browsers, so we cannot simply change the tests as we like to make it pass.

This comment has been minimized.

@jdm

jdm Dec 8, 2017

Member

Our spidermonkey version doesn't support for (const foo of bar). We modify tests to use for (let foo of bar) instead when we want to be able to run them.

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

I think I tried let but was having trouble for some reason. I'll have another go.

This comment has been minimized.

@jonleighton

jonleighton Dec 9, 2017

Contributor

Or maybe put the modified version in tests/wpt/mozilla/ for the time being? Seems a shame not to have it tested in any way, as then we won't be sure we're not introducing regressions.

This comment has been minimized.

@jonleighton

jonleighton Dec 9, 2017

Contributor

Though as far as I can see those tests (in tests/wpt/mozilla/) don't actually get run by the CI? So maybe that wouldn't particularly help.

This comment has been minimized.

@jdm

jdm Dec 10, 2017

Member

No, tests/wpt/mozilla/ definitely get run by CI. Adding a modified version to that directory is a good idea.

This comment has been minimized.

@nox

nox Dec 10, 2017

Member

15:24 <annevk> nox: I think it’s considered acceptable to rewrite if not too much of a concession in readability

We could just use the forEach method in that test.

This comment has been minimized.

@jonleighton

jonleighton Dec 10, 2017

Contributor

@jdm are you happy to do as @nox suggests (if so, this can be merged as it is), or shall I add the modified test in tests/wpt/mozilla?

@@ -1,29 +0,0 @@
<!DOCTYPE html>

This comment has been minimized.

@KiChjang

KiChjang Dec 7, 2017

Member

Why is this test file removed?

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

Also explained in the commit message:

I removed tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application-textarea.html
because it doesn't seem to add any extra value - the selection API
always applies to textarea elements, and the API is tested elsewhere.

This comment has been minimized.

@KiChjang

KiChjang Dec 8, 2017

Member

I'd rather not remove this haphazardly without contacting the authors or upstream W3C about this. Note that this is not a servo-specific test - any changes you make under tests/wpt/web-platform-tests are shared across all browsers.

This comment has been minimized.

@jonleighton

jonleighton Dec 8, 2017

Contributor

Fair enough, I'll reinstate it.

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-4 branch from a3da184 to b75b939 Dec 8, 2017

Handle cases where selection API doesn't apply
The selection API only applies to certain <input> types:

https://html.spec.whatwg.org/multipage/#do-not-apply

This commit ensures that we handle that correctly.

Some notes:

1. TextControl::set_dom_selection_direction now calls
   set_selection_range(), which means that setting selectionDirection will
   now fire a selection event, as it should per the spec.

2. There is a test for the firing of the select event in
   tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
   however the test did not run due to this syntax error:

   (pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"

   This happens due to the us of the "for (const foo of ...)" construct.
   Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
   this should actually work, so it's somewhat unsatisfying to have to
   change the test.

4. If an <input>'s type is unset, it defaults to a text, and the
   selection API applies. Also, if an <input>'s type is set to an
   invalid value, it defaults to a text too. I've expanded the tests
   to account for this second case.

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-4 branch from b75b939 to 71a013d Dec 8, 2017

var types = ["hidden", "email", "datetime-local", "date", "month", "week", "time", "number", "range", "color", "checkbox", "radio", "file", "submit", "image", "reset", "button"]; //types for which the API doesn't apply
var types2 = ["text", "search", "tel", "url", "password"]; //types for which the API applies
var nonApplicableTypes = ["hidden", "email", "datetime-local", "date", "month", "week", "time", "number", "range", "color", "checkbox", "radio", "file", "submit", "image", "reset", "button"];
var applicableTypes = ["text", "search", "tel", "url", "password", "aninvalidtype", null];

This comment has been minimized.

@KiChjang

KiChjang Dec 8, 2017

Member

Why are null and "aninvalidtype" accepted? Is it because they're both going to be treated as "text"?

This comment has been minimized.

@jonleighton

jonleighton Dec 9, 2017

Contributor

Correct. My first implementation only worked with the former, but now since I've done the InputType refactoring, both are correctly treated as "text".

@KiChjang

This comment has been minimized.

Member

KiChjang commented Dec 11, 2017

@jonleighton This should be good enough; thanks for your work!

@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 11, 2017

📌 Commit 71a013d has been approved by KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 11, 2017

⌛️ Testing commit 71a013d with merge db41cc0...

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

Auto merge of #19461 - jonleighton:issue-19171-4, r=KiChjang
Handle cases where selection API doesn't apply

The selection API only applies to certain <input> types:

https://html.spec.whatwg.org/multipage/#do-not-apply

This commit ensures that we handle that correctly.

Some notes:

1. TextControl::set_dom_selection_direction now calls
   set_selection_range(), which means that setting selectionDirection will
   now fire a selection event, as it should per the spec.

2. There is a test for the firing of the select event in
   tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
   however the test did not run due to this syntax error:

   (pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"

   This happens due to the us of the "for (const foo of ...)" construct.
   Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
   this should actually work, so it's somewhat unsatisfying to have to
   change the test.

3. I removed tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application-textarea.html
   because it doesn't seem to add any extra value - the selection API
   always applies to textarea elements, and the API is tested elsewhere.

4. If an `<input>`'s type is unset, it defaults to a text, and the
   selection API applies. Also, if an `<input>`'s type is set to an
   invalid value, it defaults to a text too. This second case doesn't
   currently work, and I'll need to do more restructuring of the code in
   a future commit. See discussion with nox in IRC:
   https://mozilla.logbot.info/servo/20171201#c13946454-c13946594

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

This comment has been minimized.

Contributor

bors-servo commented Dec 11, 2017

@bors-servo bors-servo merged commit 71a013d into servo:master Dec 11, 2017

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
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 11, 2017

Hooray, thanks

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

servo: Merge #19471 - Expand InputType to cover all possible types (f…
…rom jonleighton:input-type); r=jdm

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:
servo/servo#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.

Source-Repo: https://github.com/servo/servo
Source-Revision: 6a6da9c2a4805d28365961c6ecd1e8dc7559b0b1

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jan 4, 2018

Handle cases where selection API doesn't apply
The selection API only applies to certain <input> types:

https://html.spec.whatwg.org/multipage/#do-not-apply

This commit ensures that we handle that correctly.

Some notes:

1. TextControl::set_dom_selection_direction now calls
   set_selection_range(), which means that setting selectionDirection will
   now fire a selection event, as it should per the spec.

2. There is a test for the firing of the select event in
   tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
   however the test did not run due to this syntax error:

   (pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"

   This happens due to the us of the "for (const foo of ...)" construct.
   Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
   this should actually work, so it's somewhat unsatisfying to have to
   change the test.

4. If an <input>'s type is unset, it defaults to a text, and the
   selection API applies. Also, if an <input>'s type is set to an
   invalid value, it defaults to a text too. I've expanded the tests
   to account for this second case.

Upstreamed from servo/servo#19461 [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment