-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Assert pressure is 0 during pointerup #3794
Assert pressure is 0 during pointerup #3794
Conversation
One of these tests asserts that pressure is settable. In a real implementation, pressure shouldn't be set unless buttons is also set. Therefore, the test should also set buttons, to test a realistic state.
Reviewers for this pull request are: @EvgenyAgafonchikov, @RByers, @Steditor, @bethge, @jacobrossi, @plehegar, @scottgonzalez, and @staktrace. |
I don't think jquery polyfil is passing all the tests at the moment. @scottgonzalez might be able to help you with that. But looking at the code it does seem alright overall. Is there something particular you want me to test or something? |
@@ -30,6 +30,8 @@ | |||
var testHeight = 5; | |||
var testTiltX = -45; | |||
var testTiltY = 30; | |||
var testButton = 0; | |||
var testButtons = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although we have not setting this before and that is fine to set them this way. But there should be no expectation that when js itself creates a PointerEvent object it conforms with the spec. Meaning that it can have buttons=25 and pressure=2 as long as they fit in the data type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR I've opened to support w3c/pointerevents#146 in PEP sets pressure to 0 when buttons is 0. Thus, if someone tried to create a PointerEvent
with pressure
set but not buttons
, pressure would be 0.
This seems reasonable. pressure
without buttons
would be nonsense, and allowing a constructor implementation to validate those parameters makes it really easy to assert correctness. (Look at how simple the PEP PR is).
If allowing PointerEvent
arguments to be nonsensical is a requirement, that should be its own test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding and from the discussions we had so far the answer is yes. In other words, any combination of the field values are possible when js creates an untrusted event. But I'd like someone else to confirm this as well. Maybe @mustaqahmed @RByers @scottgonzalez can chime in here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion either way, but developers shouldn't be creating events with values that can't exist for real.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think there's a difference between "they technically can because an implementation doesn't validate" and "the spec requires that nonsense be passed-through unmolested."
I'm not asking that the spec require validation in the constructor, but I do think this PR should be merged. If we wanna specify that pressure
is always passed-through in the constructor, even if buttons
is unset, that should be a separate conversation (and a separate test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do feel the same about this PR that it can go in without further changes.
My first comment about this part was just something to bring our attentions to this subject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, constructors let you specify any value permitted by the interface even if such events aren't created by the browser in practice.
You mentioned you'd be down to be a reviewer, so I'd mostly like advice for changes and/or and LGTM and a merge. 😃 |
LGTM |
@NavidZ, can you please help me test that these are correct? I tried using PEP, but kept getting bit by jquery-archive/PEP#314