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

Some ways of adding, getting, and removing HTMLSelectElement options don't work #25003

Closed
pshaughn opened this issue Dec 2, 2019 · 6 comments
Closed

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 2, 2019

WPT custom-elements/reactions/HTMLSelectElement.html adds and removes <option> elements from a <select> various ways. select[0]=option and select.add(option) fail to add, and select[0] = null fails to remove. select.length = 0 and select.remove(0) do successfully remove. (appendChild and removeChild are not tested by this test)

@jdm jdm added the A-content/dom label Dec 2, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 2, 2019
@pshaughn pshaughn changed the title Some ways of adding and removing HTMLSelectElement options don't work Some ways of adding, getting, and removing HTMLSelectElement options don't work Dec 3, 2019
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 3, 2019

WebIDL/ecmascript-binding/legacy-platform-object.html is trying to Object.getOwnPropertyDescriptor(object,"0") a select and getting undefined instead of a property descriptor with an option in it. Much of what's happening here might relate to incomplete "legacy platform object" implementation; https://heycam.github.io/webidl/#es-legacy-platform-objects describes various things legacy platform objects are supposed to be able to do.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 25, 2019

html/semantics/forms/the-select-element/select-add.html is also having trouble here.

@teapotd teapotd mentioned this issue Jan 6, 2020
4 of 4 tasks complete
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 6, 2020

The indexed setter is commented out in HTMLSelectElement.webidl, and while that doesn't account for every one of these failures, it accounts for several!

@pshaughn pshaughn self-assigned this Jan 7, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 7, 2020

Both add() and the indexed setter just need to forward to already-existing methods in HTMLOptionsCollection, and maybe that will fix it all.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 7, 2020

uncommenting the indexed setter from HTMLSelectElement.webidl produces this:

error: failed to run custom build command for `script v0.0.1 (/home/pshaughn/servo/components/script)`

Caused by:
  process didn't exit successfully: `/home/pshaughn/servo/target/debug/build/script-f05a965ea2abfd61/build-script-build` (exit code: 1)
--- stderr
Traceback (most recent call last):
  File "dom/bindings/codegen/run.py", line 119, in <module>
    main()
  File "dom/bindings/codegen/run.py", line 52, in main
    module = CGBindingRoot(config, prefix, filename).define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 6772, in define
    return stripTrailingWhitespace(self.root.define())
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in define
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in join
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in <genexpr>
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in <genexpr>
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 6349, in define
    return self.cgRoot.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in define
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in join
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in <genexpr>
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in <genexpr>
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 1915, in define
    defn = self.child.define()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in define
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in join
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2348, in <genexpr>
    return self.joiner.join(s for s in iterable if len(s) > 0)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2351, in <genexpr>
    return self.join(child.define() for child in self.children if child is not None)
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 2578, in define
    body = self.definition_body()
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 5277, in definition_body
    return CGGeneric(self.getBody())
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 5268, in getBody
    CGIndenter(CGProxyNamedGetter(self.descriptor)).define() +
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 5087, in __init__
    CGProxySpecialOperation.__init__(self, descriptor, 'NamedGetter')
  File "/home/pshaughn/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 5004, in __init__
    assert len(operation.signatures()) == 1
AttributeError: 'NoneType' object has no attribute 'signatures'

Build FAILED in 0:00:12

It looks kind of like it's trying to find a NamedGetter, despite that not being necessary here at all.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 7, 2020

possibly

needs to be an elif like
elif self.descriptor.operations['IndexedGetter']:
and it just hasn't come up because this is the first time it's had an indexed setter thrown at it without also having a named getter.

bors-servo added a commit that referenced this issue Jan 7, 2020
Implement HTMLSelectElement.add() and indexed setter

HTMLSelectElement.add and its indexed setter just needed to forward to matching methods in HTMLOptionsCollection, which they now do. It was also necessary to change codegen slightly; it had accidentally assumed that if an indexed setter existed, a named getter or setter also would.

I expect this and #25446 will combine to pass more tests than either alone 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 #25003

<!-- Either: -->
- [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. -->
@bors-servo bors-servo closed this in 1e7c206 Jan 8, 2020
web-platform-test failures automation moved this from To do to Done Jan 8, 2020
bors-servo added a commit that referenced this issue Apr 1, 2020
Form constraint validation

It's almost done, there are few things remaining:

- ~Range underflow, range overflow and step mismatch implementation require #25405~
- ~There are some test failures due to missing DOM parts (#25003)~
- ~`pattern` attribute uses JS regexp syntax. Currently I used regex crate, but it's probably incompatible. Should we use SpiderMonkey's regexp via jsapi?~
- Currently validation errors are reported using `println!`. Are there any better options?
- ~["While the user interface is representing input that the user agent cannot convert to punycode, the control is suffering from bad input."](https://html.spec.whatwg.org/multipage/#e-mail-state-(type%3Demail)%3Asuffering-from-bad-input)~

r? @jdm

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11444
- [x] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 1, 2020
Form constraint validation

It's almost done, there are few things remaining:

- ~Range underflow, range overflow and step mismatch implementation require #25405~
- ~There are some test failures due to missing DOM parts (#25003)~
- ~`pattern` attribute uses JS regexp syntax. Currently I used regex crate, but it's probably incompatible. Should we use SpiderMonkey's regexp via jsapi?~
- Currently validation errors are reported using `println!`. Are there any better options?
- ~["While the user interface is representing input that the user agent cannot convert to punycode, the control is suffering from bad input."](https://html.spec.whatwg.org/multipage/#e-mail-state-(type%3Demail)%3Asuffering-from-bad-input)~

r? @jdm

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11444
- [x] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 1, 2020
Form constraint validation

It's almost done, there are few things remaining:

- ~Range underflow, range overflow and step mismatch implementation require #25405~
- ~There are some test failures due to missing DOM parts (#25003)~
- ~`pattern` attribute uses JS regexp syntax. Currently I used regex crate, but it's probably incompatible. Should we use SpiderMonkey's regexp via jsapi?~
- Currently validation errors are reported using `println!`. Are there any better options?
- ~["While the user interface is representing input that the user agent cannot convert to punycode, the control is suffering from bad input."](https://html.spec.whatwg.org/multipage/#e-mail-state-(type%3Demail)%3Asuffering-from-bad-input)~

r? @jdm

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11444
- [x] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 2, 2020
Form constraint validation

It's almost done, there are few things remaining:

- ~Range underflow, range overflow and step mismatch implementation require #25405~
- ~There are some test failures due to missing DOM parts (#25003)~
- ~`pattern` attribute uses JS regexp syntax. Currently I used regex crate, but it's probably incompatible. Should we use SpiderMonkey's regexp via jsapi?~
- Currently validation errors are reported using `println!`. Are there any better options?
- ~["While the user interface is representing input that the user agent cannot convert to punycode, the control is suffering from bad input."](https://html.spec.whatwg.org/multipage/#e-mail-state-(type%3Demail)%3Asuffering-from-bad-input)~

r? @jdm

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11444
- [x] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 2, 2020
Form constraint validation

It's almost done, there are few things remaining:

- ~Range underflow, range overflow and step mismatch implementation require #25405~
- ~There are some test failures due to missing DOM parts (#25003)~
- ~`pattern` attribute uses JS regexp syntax. Currently I used regex crate, but it's probably incompatible. Should we use SpiderMonkey's regexp via jsapi?~
- Currently validation errors are reported using `println!`. Are there any better options?
- ~["While the user interface is representing input that the user agent cannot convert to punycode, the control is suffering from bad input."](https://html.spec.whatwg.org/multipage/#e-mail-state-(type%3Demail)%3Asuffering-from-bad-input)~

r? @jdm

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #11444
- [x] There are tests for these changes
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.

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