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

Remove document.createEvent("touchevent") #27250

Merged
merged 4 commits into from
Jan 21, 2021
Merged

Conversation

LanWei22
Copy link
Contributor

Since in DOM spec, createEvent(interface) is not recommended and suggests "Event constructors ought to be used instead." Also, Chrome, Edge, Firefox and Safari do not support document.createEvent("touchevent"), we should just delete it.
DOM spec: https://dom.spec.whatwg.org/#dom-document-createevent.

@LanWei22
Copy link
Contributor Author

cynthia@ could you please take a look, thank you.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to remove TouchEvent from dom/nodes/Document-createEvent.js and add it to the not-supported list in dom/nodes/Document-createEvent.https.html.

Comment on lines 39 to 41
var touchEvent = document.createEvent("touchevent");
assert_false("initTouchEvent" in touchEvent,
"Should not be supported on the instance");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this test, but use the constructor to create touchEvent instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

@wpt-pr-bot wpt-pr-bot added the dom label Jan 20, 2021
@wpt-pr-bot wpt-pr-bot requested review from annevk and jdm January 20, 2021 21:15
Copy link
Contributor Author

@LanWei22 LanWei22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to remove TouchEvent from dom/nodes/Document-createEvent.js and add it to the not-supported list in dom/nodes/Document-createEvent.https.html.

Done, thank you.

Comment on lines 39 to 41
var touchEvent = document.createEvent("touchevent");
assert_false("initTouchEvent" in touchEvent,
"Should not be supported on the instance");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

@LanWei22 LanWei22 requested a review from Ms2ger January 20, 2021 22:51
@annevk
Copy link
Member

annevk commented Jan 21, 2021

This is still exposed on mobile as it happens. Properly specifying this in the DOM Standard depends on w3c/touch-events#64. I'm not sure how we would expose that boolean in tests though.

@Ms2ger Ms2ger merged commit b36a20a into master Jan 21, 2021
@Ms2ger Ms2ger deleted the toucheventhistoricaltest branch January 21, 2021 10:09
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 21, 2021

@annevk gah, didn't see your comment before merging. Lemme know if I should revert.

@annevk
Copy link
Member

annevk commented Jan 21, 2021

I think it's fine. Once that issue is fixed we'll need to have another look at this and for desktop these are the correct results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants