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.

Sign up for GitHub

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

Assert pressure is 0 during pointerup #3794

Merged
merged 4 commits into from
Sep 28, 2016
Merged

Assert pressure is 0 during pointerup #3794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2adc6f5
[pointerevents] Remove dead code in iframe
appsforartists Sep 21, 2016
68ba272
[pointerevents] Set event.buttons to match event.pressure
appsforartists Sep 21, 2016
100a484
[pointerevents] Add constructor tests for event.button and event.buttons
appsforartists Sep 21, 2016
f6f2c1c
[pointerevents] Assert event.pressure is 0 when event.type == pointerup
appsforartists Sep 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pointerevents/pointerevent_constructor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ <h4>Test Description: This test checks if PointerEvent constructor works properl
var testHeight = 5;
var testTiltX = -45;
var testTiltY = 30;
var testButton = 0;
var testButtons = 1;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Member

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.

Copy link
Contributor

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.

var testPressure = 0.4;
var testIsPrimary = true;

Expand All @@ -40,6 +42,8 @@ <h4>Test Description: This test checks if PointerEvent constructor works properl
["custom cancelable", event.cancelable, testCancelable],
["custom pointerId", event.pointerId, testPointerId],
["custom pointerType", event.pointerType, testPointerType],
["custom button", event.button, testButton],
["custom buttons", event.buttons, testButtons],
["custom width", event.width, testWidth],
["custom height", event.height, testHeight],
["custom clientX", event.clientX, testClientX],
Expand Down Expand Up @@ -80,6 +84,8 @@ <h4>Test Description: This test checks if PointerEvent constructor works properl
clientY: testClientY,
tiltX: testTiltX,
tiltY: testTiltY,
button: testButton,
buttons: testButtons,
pressure: testPressure,
isPrimary: testIsPrimary
});
Expand Down
3 changes: 3 additions & 0 deletions pointerevents/pointerevent_support.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ function check_PointerEvent(event, testNamePrefix) {
assert_greater_than_equal(event.pressure, 0, "pressure is greater than or equal to 0");
assert_less_than_equal(event.pressure, 1, "pressure is less than or equal to 1");

if (event.type === "pointerup") {
assert_equals(event.pressure, 0, "pressure is 0 during pointerup");
}

// TA: 1.7, 1.8
if (event.pointerType === "mouse") {
Expand Down
2 changes: 0 additions & 2 deletions pointerevents/resources/pointerevent_pointerId_scope-iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
<script>
function run() {
var target1 = document.getElementById("target1");
var pointerover_event;
var ponterId = null;

var eventList = ['pointerenter', 'pointerover', 'pointermove', 'pointerout', 'pointerleave'];

Expand Down