Skip to content

/ servo

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

Jump to bottom

Properly use [PutForwards] like we should in WebIDL interfaces #9990

Closed
nox opened this issue Mar 14, 2016 · 11 comments
Closed

Properly use [PutForwards] like we should in WebIDL interfaces #9990

nox opened this issue Mar 14, 2016 · 11 comments
Labels
A-content/dom B-high-value C-needs-test

Comments

@nox
Copy link
Member

@nox nox commented Mar 14, 2016

See #9350.
@jdm
Copy link
Member

@jdm jdm commented Mar 14, 2016

Specifically, the PR looks good but is missing tests that the attributes using PutForwards do what they're supposed to.
@tbu-
Copy link

@tbu- tbu- commented Mar 15, 2016

I'd like to try this.
@jdm
Copy link
Member

@jdm jdm commented Mar 15, 2016

@tbu- Please do! Ask questions here if anything's unclear :)
@jdm jdm added the C-assigned label Mar 15, 2016
@tbu- tbu- mentioned this issue Mar 24, 2016
Closed
@jdm jdm removed the C-assigned label Apr 9, 2016
@cbrewster cbrewster mentioned this issue Jul 3, 2017
Closed
4 of 4 tasks complete
@nox
Copy link
Member Author

@nox nox commented Oct 1, 2017

This still hasn't been done.
@jdm jdm mentioned this issue Mar 31, 2018
Closed
@pshaughn pshaughn mentioned this issue Dec 12, 2019
Closed
@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 12, 2019

#25037 and #25038 were me not realizing that ElementCSSInlineStyle.webidl has the PutForwards part of HTMLElement.style commented out entirely.
@nox nox closed this Jan 6, 2020
@nox
Copy link
Member Author

@nox nox commented Jan 6, 2020

Oops, misclick.
@nox nox reopened this Jan 6, 2020
@jdm jdm added the B-high-value label Jan 6, 2020
@jdm
Copy link
Member

@jdm jdm commented Jan 6, 2020

Marking this as high-value because code like foo.style = "display: none" just silently does not work in Servo right now, which likely leads to web compat issues.
@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 7, 2020

I've tried to find every location that defines a PutForwards and note its current status. The only implemented attributes with a missing PutForwards appear to be ElementCSSInlineStyle.cssText and multiple instances of .relList, although it's possible I missed a relevant spec.

from WHATWG HTML:

  • Document.location - already PutForwards in Servo
  • Window.location - already PutForwards in Servo
  • HTMLIFrameElement.sandbox - already PutForwards in Servo
  • HTMLAnchorElement.relList - attribute exists in Servo without PutForwards
  • HTMLAreaElement.relList - attribute exists in Servo without PutForwards
  • HTMLLinkElement.relList - attribute exists in Servo without PutForwards
  • HTMLFormElement.relList - attribute is entirely absent in Servo
  • HTMLLinkElement.sizes - attribute is commented out in Servo
  • HTMLOutputElement.htmlFor - attribute is commented out in Servo

From CSSOM:

  • CSSStyleRule.cssText - already PutForwards in Servo
  • ElementCSSInlineStyle.cssText - attribute exists in Servo with commented-out PutForwards
  • CSSImportRule.mediaText - attribute is commented out in Servo
  • Stylesheet.mediaText - attribute is commented out in Servo
  • CSSPageRule.cssText - no IDL in servo
  • CSSMarginRule.cssText - no IDL in servo

From CSS Fonts Module:

  • CSSFontFaceRule.style - not PutForwards in spec, attribute with PutForwards is commented out in Servo (probably just cut and paste from other CSS*Rule IDL)
@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 7, 2020

The ElementCSSInlineStyle.cssText case has tests for it (and pass when the PutForwards is included); the .relList cases seem to be missing test coverage for PutForwards behavior.
@pshaughn pshaughn mentioned this issue Jan 7, 2020
Merged
3 of 4 tasks complete
bors-servo added a commit that referenced this issue Jan 7, 2020
@bors-servo
Auto merge of #25450 - pshaughn:putforwards, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
371674f 
Add PutForwards to implemented attributes that lacked it

Added PutForwards to three relLists and one style. The style already had a test; I added tests for relLists, including one relList that we have commented out right now.

---
<!-- 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 advance #9990 to be caught up with the attributes we have so far; many unimplemented attributes will need PutForwards when implemented.

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

@pshaughn pshaughn commented Jan 7, 2020

As far as I can tell, assuming the above PR merges, we will be up-to-date on [PutForwards] with respect to attributes that currently exist in Servo's IDL, and remaining lack of PutForwards behavior is because the entire attribute is unimplemented.
bors-servo added a commit that referenced this issue Jan 23, 2020
@bors-servo
Auto merge of #25450 - pshaughn:putforwards, r=jdm,Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
dec6d41 
Add PutForwards to implemented attributes that lacked it

Added PutForwards to three relLists and one style. The style already had a test; I added tests for relLists, including one relList that we have commented out right now.

---
<!-- 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 advance #9990 to be caught up with the attributes we have so far; many unimplemented attributes will need PutForwards when implemented.

<!-- 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 23, 2020
@bors-servo
Auto merge of #25450 - pshaughn:putforwards, r=jdm,Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8bc54fd 
Add PutForwards to implemented attributes that lacked it

Added PutForwards to three relLists and one style. The style already had a test; I added tests for relLists, including one relList that we have commented out right now.

---
<!-- 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 advance #9990 to be caught up with the attributes we have so far; many unimplemented attributes will need PutForwards when implemented.

<!-- 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 23, 2020
@bors-servo
Auto merge of #25450 - pshaughn:putforwards, r=jdm,Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d0d3aaa 
Add PutForwards to implemented attributes that lacked it

Added PutForwards to three relLists and one style. The style already had a test; I added tests for relLists, including one relList that we have commented out right now.

---
<!-- 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 advance #9990 to be caught up with the attributes we have so far; many unimplemented attributes will need PutForwards when implemented.

<!-- 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 23, 2020

This should be fixed now!
@jdm jdm closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/dom B-high-value C-needs-test
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@jdm @nox @pshaughn @tbu-
You can’t perform that action at this time.