Add movementxy attribute test for pointerevents #4571

NavidZ · 2017-01-19T20:52:49Z

RByers · 2017-01-20T14:11:54Z

w3c-test:mirror (want to try the test out on w3c-test.org)
One initial nit: rather than a generic "interaction-with-other-specs" directory can you just use the name of the spec - "pointerlock"? That's the pattern we've already started with the "compat" sub-directory.

RByers · 2017-01-20T15:11:08Z

Test is now served here. I can't get it to complete on current Chrome dev channel -
the mouse one completes, but clicking 'skip' or using touch I still get "1 remain".

I was initially confused about the iframe and requirement that movement is kept to <10 pixels per event. This is because the events that land over the frame are lost, right? This is a same-origin iframe so it should be easy to also have listeners inside the frame which just call into some common code in the top document so that you can test both the events delivered to the top document and those delivered inside the frame.

Big picture, what I was wondering was - is it better to rely on relatively precise positioning like this (move from red to black), and/or should our test just verify that the movement sum exactly equals the change in client (or maybe screen) co-ordinates the way the pointerlock tests do?

/cc @scheib who may have input. From a quick glance I didn't see any movementX/Y tests there which look explicitly at the iframe case (but maybe it was only our originally flawed PointerEvents implementation that would even suggest that such a test was worthwhile).

NavidZ · 2017-01-20T15:47:09Z

We don't have any further sub-directory under compat though. I thought I just put all the related interaction with other spec tests under here like Shadow dom that will come later. If I were to create a folder for each some of those folders will have only one test. Having said that which one do you think is better? Create a separate subfolder under pointerevents for each other spec we would like to add test?

I do have a handler for the inner iframe as well and I also check the events from that (i.e. innerSumX vs sumX). The reason that I wanted the movements to be small is to avoid the jump when you cross the boundary of an iframe discussed in this issue on PointerLock. So I can actually sum things up inside the frame and outside the frame and be able to rely on the range of the result.

RByers · 2017-01-25T16:05:29Z

We don't have any further sub-directory under compat though

Right, compat is the name of the spec (compat.spec.whatwg.org). I only expect the single test there for the forseable future. We should have the spec name in the path somewhere, but as long as we're consistent with the pattern I don't really care. Eg: we could have:

compat/pointerevent_touch-action_two-finger_interaction-manual.html
pointerlock/pointerevent_movementxy-manual.html

or

interaction-with-other-specs/compat-pointerevent_touch-action_two-finger_interaction-manual.html
interaction-with-other-specs/pointerlock-pointerevent_movementxy-manual.html

I have a slight preference for the former, but as long as we're not mixing different patterns I don't really care.

RByers · 2017-01-25T16:12:05Z

I do have a handler for the inner iframe as well and I also check the events from that (i.e. innerSumX vs sumX). The reason that I wanted the movements to be small is to avoid the jump when you cross the boundary of an iframe discussed in this issue on PointerLock. So I can actually sum things up inside the frame and outside the frame and be able to rely on the range of the result.

I see, thanks. See my comment here. I don't think it makes sense to try to test the movementX/Y for the first event inside an iframe without communicating with the outer frame in order to measure the real delta.

RByers · 2017-01-25T16:18:56Z

w3c-test:mirror

RByers · 2017-01-25T16:38:16Z

pointerevents/pointerlock/pointerevent_movementxy-manual.html

+                });
+                ['pointerenter', 'pointerleave'].forEach(function(eventName) {
+                  on_event(document.getElementById('innerFrame').contentDocument, eventName, function (event) {
+                    isFirstMove = true;


I see, so this means we're not testing the movementX/movementY values for the event that enters/leaves the iframe. That's OK but not ideal (misses some interesting test coverage).

The simplest way to get this additional test coverage is to use screenX/Y instead of clientX/Y (and remove all the isFirstMove logic). That would also better match the spec, since movementX/Y is defined in terms of screen co-ordinates. Eg. if the browser content area happened to move relative to the physical screen during the test (such as when top controls show or hide in mobile browsers) then the test should still pass when using screen co-ordinates but should fail when using client (assuming we've implemented according to the spec).

If there's some reason why using client co-ordinates is preferable (eg. perhaps @schieb has a reason why the existing pointerlock tests do that), you could still get this additional test coverage as follows:

specify the iframe border size explicitly (with either the 'border=' HTML attribute or a CSS border rule on the iframe element) rather than relying on the browser default (not sure that's standardized).

on test start, compute the offset of the iframe within the parent frame by using getBoundingClientRect on innerFrame plus the border size you specified

in your pointermove handler, if the event target is inside the iframe, add the above offset to the client co-ordinates

In that way you'd be doing all your math in the "root frame" coordinate space (instead of in the client / local-frame co-ordinate space). And then the math should work out in all cases.

screenX/Y are good, the use of clientX/Y in other tests was an oversight. I've filed an issue to fix the pointerlock tests #4630.

Perfect, thanks! Sorry for the mis-spell ;-)

NavidZ · 2017-01-26T16:52:42Z

Now it looks a lot better using screenX/Y. I was able to get rid of all that isFirstMove/enter/leave logic. Thanks. ptal.

RByers · 2017-01-26T17:03:08Z

w3c-test:mirror

RByers · 2017-01-26T17:53:46Z

Yeah it looks great - thanks! Just wanted to try it out again myself.

On Chrome 55 desktop (the only laptop I have handy in the meeting I'm sitting in) the mouse case fails the ASSERT. Is there a known bug for mouse in Chrome 55? I thought the only known bug was with touch?

I also tried Chrome 57 android but it seems the test won't work on a phone - it doesn't fit and you apparently can't scroll the page. It's probably not worth a big effort now (eg. could be a mobile-friendly-cleanup we do later), but do you think it's worth seeing if you can tweak the layout / touch-action a bit to make this test passable on a phone?

NavidZ · 2017-01-26T19:56:04Z

w3c-test:mirror

NavidZ · 2017-01-26T20:23:38Z

Layout is fixed now. Chrome 55 does have the bug. That is the intention of this test. Hopefully it will be fixed in the latest version.

Ms2ger · 2017-01-31T13:22:02Z

@RByers is this good to go now?

RByers · 2017-02-03T05:01:42Z

w3c-test:mirror

RByers · 2017-02-03T05:02:54Z

LGTM, thanks!

smaug---- · 2017-10-02T11:52:39Z

So this was wrong. PointerLock spec explicitly states that "movementX/Y must be zero for all mouse events except mousemove." We need a spec change, as far as I see.

Add movementxy attribute test for pointerevents

fe546e5

NavidZ mentioned this pull request Jan 20, 2017

movementX/Y when moving out of or into an iframe w3c/pointerlock#16

Open

use clientX/Y for testing

4343fd1

wpt-pr-bot added pointerevents wg-pointerevents labels Jan 23, 2017

RByers reviewed Jan 25, 2017

View reviewed changes

scheib mentioned this pull request Jan 25, 2017

pointerlock movementX/Y tests should use screenX/Y, not clientX/Y #4630

Open

use screenX/Y

69e2394

Fix layout of the test for small screens

a000320

RByers merged commit b6125d6 into web-platform-tests:master Feb 3, 2017

RByers mentioned this pull request Feb 8, 2017

Auto-mirror "safe" tests #4636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add movementxy attribute test for pointerevents #4571

Add movementxy attribute test for pointerevents #4571

NavidZ commented Jan 19, 2017 •

edited

RByers commented Jan 20, 2017

RByers commented Jan 20, 2017 •

edited

NavidZ commented Jan 20, 2017

RByers commented Jan 25, 2017

RByers commented Jan 25, 2017

RByers commented Jan 25, 2017

RByers Jan 25, 2017

schieb Jan 25, 2017

scheib Jan 25, 2017

RByers Jan 26, 2017

NavidZ commented Jan 26, 2017

RByers commented Jan 26, 2017

RByers commented Jan 26, 2017

NavidZ commented Jan 26, 2017

NavidZ commented Jan 26, 2017

Ms2ger commented Jan 31, 2017

RByers commented Feb 3, 2017

RByers commented Feb 3, 2017

smaug---- commented Oct 2, 2017

Add movementxy attribute test for pointerevents #4571

Add movementxy attribute test for pointerevents #4571

Conversation

NavidZ commented Jan 19, 2017 • edited

RByers commented Jan 20, 2017

RByers commented Jan 20, 2017 • edited

NavidZ commented Jan 20, 2017

RByers commented Jan 25, 2017

RByers commented Jan 25, 2017

RByers commented Jan 25, 2017

RByers Jan 25, 2017

Choose a reason for hiding this comment

schieb Jan 25, 2017

Choose a reason for hiding this comment

scheib Jan 25, 2017

Choose a reason for hiding this comment

RByers Jan 26, 2017

Choose a reason for hiding this comment

NavidZ commented Jan 26, 2017

RByers commented Jan 26, 2017

RByers commented Jan 26, 2017

NavidZ commented Jan 26, 2017

NavidZ commented Jan 26, 2017

Ms2ger commented Jan 31, 2017

RByers commented Feb 3, 2017

RByers commented Feb 3, 2017

smaug---- commented Oct 2, 2017

NavidZ commented Jan 19, 2017 •

edited

RByers commented Jan 20, 2017 •

edited