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

Missing MouseEvent attributes defined by CSSOM View #24248

Closed
jdm opened this issue Sep 20, 2019 · 9 comments
Closed

Missing MouseEvent attributes defined by CSSOM View #24248

jdm opened this issue Sep 20, 2019 · 9 comments
Labels
A-content/dom A-webcompat C-assigned E-candidate-for-mentoring

Comments

@jdm
Copy link
Member

@jdm jdm commented Sep 20, 2019 •
edited

https://drafts.csswg.org/cssom-view/#extensions-to-the-mouseevent-interface

In particular, the pageX/pageY attributes are used by three.js code in http://rawcdn.githack.com/mrdoob/three.js/r105/examples/js/controls/FirstPersonControls.js . This is likely to be a source of surprising web compat issues!

This will likely have an impact on #20931.
@tigleym
Copy link
Contributor

@tigleym tigleym commented Oct 6, 2019

I'd like to try working on this.

@highfive assign me
@highfive
Copy link

@highfive highfive commented Oct 6, 2019

Hey @tigleym! Thanks for your interest in working on this issue. It's now assigned to you!
@highfive highfive added the C-assigned label Oct 6, 2019
@tigleym tigleym mentioned this issue Oct 9, 2019
Merged
4 of 5 tasks complete
@tigleym
Copy link
Contributor

@tigleym tigleym commented Oct 9, 2019

@jdm the above pull request addresses specifically the pageX and pageY attributes, although I realize that offsetX and offsetY are missing as well (https://drafts.csswg.org/cssom-view/#dom-mouseevent-offsetx).

As for tests, I added a simple one in tests/wpt/web-platform-tests/css/cssom-view/mouseEvent.html. I'd like to also test for the existence of the attribute on the interface itself, and it's currently set to fail here:

servo/tests/wpt/metadata/css/cssom-view/idlharness.html.ini

Lines 32 to 36 in b0b01b8

[MouseEvent interface: attribute pageX]
expected: FAIL
[MouseEvent interface: attribute pageY]
expected: FAIL

I believe I found the corresponding test at

servo/tests/wpt/web-platform-tests/css/cssom-view/idlharness.html

Line 37 in b0b01b8

// MouseEvent: ['new MouseEvent("foo")'],

Uncommenting this line still doesn't seem to run the test at all. Perhaps I'm missing something here?
@jdm
Copy link
Member Author

@jdm jdm commented Oct 9, 2019

Great question about the idlharness test! I dug into it, and it turns out the cssom-view.idl is failing the WebIDL validation tests because the partial interface for MouseEvent that it contains (https://github.com/web-platform-tests/wpt/blob/0d9fecceac869018c2b54b6251600f3d184d98c0/interfaces/cssom-view.idl#L141-L152) duplicates some properties from uievents.webidl (https://github.com/web-platform-tests/wpt/blob/0d9fecceac869018c2b54b6251600f3d184d98c0/interfaces/uievents.idl#L28-L31), and this prevents the MouseEvent interface from being tested.
@jdm
Copy link
Member Author

@jdm jdm commented Oct 9, 2019 •
edited

In a delightful twist, there is a known issue in that the CSSOM View spec redefines existing members that is called out at the top of https://drafts.csswg.org/cssom-view/#extensions-to-the-mouseevent-interface (w3c/csswg-drafts#4084).
@tigleym
Copy link
Contributor

@tigleym tigleym commented Oct 10, 2019

Thank you for looking into this! Glad to see the appropriate fix for testing these MouseEvent attributes was merged in: web-platform-tests/wpt#19611
@jdm
Copy link
Member Author

@jdm jdm commented Oct 10, 2019

We should pick those changes up automatically in tomorrow's sync.
bors-servo added a commit that referenced this issue Oct 17, 2019
@bors-servo
Auto merge of #24409 - tigleym:mouse_event_cssom_attributes, r=jdm
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…
68f16c7 
Implements pageX and pageY attributes on MouseEvent interface

<!-- Please describe your changes on the following line: -->
Implements missing pageX and pageY on MouseEvent interface.

---
<!-- 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 #24248 (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/24409)
<!-- Reviewable:end -->
@tigleym
Copy link
Contributor

@tigleym tigleym commented Oct 17, 2019

@jdm for the rest of the missing attributes, would you prefer if I created another issue as a follow-up? Or can I make another PR against this same issue?
@jdm
Copy link
Member Author

@jdm jdm commented Oct 17, 2019

Another PR against this one should be fine.
@tigleym tigleym mentioned this issue Oct 22, 2019
Merged
3 of 5 tasks complete
bors-servo added a commit that referenced this issue Oct 22, 2019
@bors-servo
Auto merge of #24521 - tigleym:missing_cssom_attributes, r=<try>
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…
b3119be 
Implement MouseEvent's x/y and offsetX/offsetY attributes

<!-- Please describe your changes on the following line: -->
Implement MouseEvent's x/y and offsetX/offsetY attributes

---
<!-- 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 #24248 (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. -->
bors-servo added a commit that referenced this issue Nov 5, 2019
@bors-servo
Auto merge of #24521 - tigleym:missing_cssom_attributes, r=jdm
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…
f6a906d 
Implement MouseEvent's x/y and offsetX/offsetY attributes

<!-- Please describe your changes on the following line: -->
Implement MouseEvent's x/y and offsetX/offsetY attributes

---
<!-- 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 #24248 (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. -->
bors-servo added a commit that referenced this issue Nov 5, 2019
@bors-servo
Auto merge of #24521 - tigleym:missing_cssom_attributes, r=jdm
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…
28dbbda 
Implement MouseEvent's x/y and offsetX/offsetY attributes

<!-- Please describe your changes on the following line: -->
Implement MouseEvent's x/y and offsetX/offsetY attributes

---
<!-- 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 #24248 (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. -->
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 A-webcompat C-assigned E-candidate-for-mentoring
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Implements pageX and pageY attributes on MouseEvent interface
Implement MouseEvent's x/y and offsetX/offsetY attributes
3 participants
@jdm @highfive @tigleym
You can’t perform that action at this time.