-
Notifications
You must be signed in to change notification settings - Fork 24
createTouch has all the params as required, should probably have them optional #58
Comments
Both the test and the spec use 7 arguments, so I'm not seeing the issue there. But still they should perhaps be optional. They're certainly optional in blink, but I haven't tested other engines. @choniong WDYT? Also the "magic" clientX/Y behavior is how this API has always worked on at least Safari and Chrome AFAIK. There is some precedent for this sort of thing (eg. MouseEvent.pageX/pageY), but we ultimately decided not to follow that pattern for the constructors: #28 |
Concretely, if Safari, Firefox and Edge already treat all params as optional, we should just update the spec to match reality (and perhaps add a test - though being deprecated I'm not sure it's worthwhile). |
oh, I was looking at Gecko's createTouch which has more param. So ok, the test is fine per current spec (except that is assumes some magical undefined pageX/Y -> clientX/Y conversion) |
The spec assumes this magical conversion as well (or at least fails to define how one can initialize clientX/Y using this API). But I agree the spec doesn't quite fully define what should happen here (though it's somewhat implicit from the definitions of clientX/clientY and pageX/pageY). My feeling is that since these APIs are now deprecated, it's not worth trying to fix anything here (we should instead just focus on the constructors). But I'm willing to add something to the spec for this if you think it's valuable. WDYT? |
From my tests https://output.jsbin.com/xeviri
Since the behavior is so different I guess it's not worth trying to fix it? PS: It also means Firefox won't pass the create-touch-touchlist test for |
Thanks Chong. Since Safari requires these 7, I don't think we should change the spec to mark them |
Safari on iOS has 7 required arguments, but the first two can be null, i.e. |
in light of us wanting to potentially deprecate #80 is this still relevant? perhaps for clarity/historical reasons? |
Blink now has 7 required arguments as well: https://codereview.chromium.org/2747333003/ |
is there anything we want to add to the spec in light of this? even just as a non-normative note? |
There's still the issue that Blink has additional optional arguments, but that wasn't the initial bug here. How good is the test coverage for the corner cases here? I'm pretty sure there are differences if one passes in null for some of these arguments. |
https://github.com/w3c/web-platform-tests/blob/master/touch-events/create-touch-touchlist.html#L124 should per spec throw, since not enough params are passed to the method. But instead that test assumes different behavior and it also assumes clientX/Y somehow magically get their values from other coordinates.
The text was updated successfully, but these errors were encountered: