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

Update EventHandler.webidl #11158

Closed
KiChjang opened this issue May 12, 2016 · 7 comments
Closed

Update EventHandler.webidl #11158

KiChjang opened this issue May 12, 2016 · 7 comments

Comments

@KiChjang
Copy link
Member

@KiChjang KiChjang commented May 12, 2016

The WebIDL has more events defined on it now. Also, there is a new DocumentAndElementEventHandlers interface that is supposed to be defined on Document and HTMLElement.

Spec: https://html.spec.whatwg.org/multipage/webappapis.html#globaleventhandlers, https://html.spec.whatwg.org/multipage/webappapis.html#windoweventhandlers, https://html.spec.whatwg.org/multipage/webappapis.html#documentandelementeventhandlers
Code: components/script/dom/webidls/EventHandler.webidl, components/script/dom/webidls/HTMLElement.webidl, components/script/dom/webidls/Document.webidl

@KiChjang KiChjang changed the title Update GlobalEventHandler Update EventHandler.rs May 12, 2016
@KiChjang KiChjang changed the title Update EventHandler.rs Update EventHandler.webidl May 12, 2016
@s-baldrick
Copy link
Contributor

@s-baldrick s-baldrick commented May 12, 2016

@KiChjang
I'd like to have a go at this (though I'm unlikely to get started before Saturday).

Greetings,
Piotr

@jdm jdm added the C-assigned label May 12, 2016
@jdm
Copy link
Member

@jdm jdm commented May 12, 2016

Great!

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented May 13, 2016

Should we implement onfoo attributes where we never dispatch a foo event?

@jdm
Copy link
Member

@jdm jdm commented May 13, 2016

On one hand it's not great for feature detection. On the other hand, it's really easy to forget to add them when implementing code that dispatches the appropriate event, and it leads to frustrating and confusing debugging sessions. I lean towards adding the handlers for that reason.

@s-baldrick
Copy link
Contributor

@s-baldrick s-baldrick commented May 18, 2016

Hi, I've done a couple patches for this - https://github.com/s-baldrick/servo/tree/11158, and now I am trying to come up with some tests, although I am not sure what will be a sensible way of going about it since not all of these events will be triggered. I guess asserting if setting an event handler does not fail could be one way, but then they all get set in the same way, so testing for this might not bring much to the table. Do you have some suggestions about this?

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented May 18, 2016

You can actually open a PR right now, and I'll do a try run and see which tests it'll break. Some of our WPT tests are marked as FAILING, but after defining these events, I'd expect plenty of them to PASS.

@s-baldrick
Copy link
Contributor

@s-baldrick s-baldrick commented May 18, 2016

Cool, I'll open it this evening, as I am at work right now.

@s-baldrick s-baldrick mentioned this issue May 18, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue May 19, 2016
11158 - add event handlers

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 --faster` does not report any errors
- [X] These changes fix #11158 (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ____

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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11255)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 20, 2016
11158 - add event handlers

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 --faster` does not report any errors
- [X] These changes fix #11158 (github issue number if applicable).

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ____

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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11255)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 20, 2016
11158 - add event handlers

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 --faster` does not report any errors
- [X] These changes fix #11158 (github issue number if applicable).

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ____

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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11255)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 21, 2016
11158 - add event handlers

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 --faster` does not report any errors
- [X] These changes fix #11158 (github issue number if applicable).

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ____

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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11255)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 21, 2016
11158 - add event handlers

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 --faster` does not report any errors
- [X] These changes fix #11158 (github issue number if applicable).

Either:
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ____

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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11255)
<!-- Reviewable:end -->
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.

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