Treat 'no orphan pointerup' as a pass in the event timing tests #51630
Conversation
|
Hey @mmocny, this PR changes the test to treat 'no pointerup event' as a pass, like we discussed. Could you take a look at it? (can't mark you as a reviewer here) |
|
I equally dont love this but I think its the most sensible immediate solution. Other options are to remove the WPT for this case, as arguably this Chrome behaviour might itself be against UIEvents/PointerEvents spec expectations -- but perhaps we can followup after exploring that. Cheers. |
|
Thanks for the review! Yes, let's explore other options after landing this. @npm1, It looks like @mmocny is not part of the wpt org and this patch still needs a review. Could you take a look as well? |
| // timeout mechanism. If no events are fired in 2 seconds, treat 'no | ||
| // pointerup event' as a pass. 2 seconds is an arbitrary time we picked, | ||
| // consider changing if it's flaky. | ||
| new Promise(resolve => t.step_timeout(resolve, 2000)) |
There was a problem hiding this comment.
This will be a source of flakiness. Maybe add some other event that you know is going to be processed after or something?
There was a problem hiding this comment.
Oh, that's a great idea! It should work well with both of the browsers. I'm updating the PR in a bit.
|
@npm1 Thanks for the suggestion, I've updated the patch. I tested it with both Firefox and Chrome and both of them pass (with the correct condition). Could you take a look at it again? |
| await interactAndObserve('orphan-pointerup', document.getElementById('testButtonId'), observerPromise); | ||
| assert_equals(map.get('pointerup'), 0, 'Should have a trivial interactionId for orphan pointerup event.'); | ||
|
|
||
| // This test passes when: |
There was a problem hiding this comment.
nit: 'when either' to make it clearer
|
Hmm weird. CI is failing because of Firefox failures but it's from a test that I don't touch in this PR and it was already failing before. |
This is something we came up while implementing it in Firefox. We don't fire an orphan pointerup event at all when there is only a pointerup and no pointerdown. So this test was failing. This change adds a timeout mechanism for browsers like Firefox, so we can treat 'no pointerup entry' as a pass. As a solution, we are sending a non-pointer related event right after the pointerup event, to make sure at least one event is being handled by the test so it doesn't timeout. It fixes [Bug 1956598](https://bugzilla.mozilla.org/show_bug.cgi?id=1956598).
canova
force-pushed
the
orphan-pointerup
branch
from
c61f89a to
519947a
Compare
…ng tests r=sefeng This patch was approved in web-platform-tests/wpt#51630. But unfortunately I couldn't manage to land it there due to some intermittent Firefox and Chrome test failures that are unrelated to the changes I was making. Differential Revision: https://phabricator.services.mozilla.com/D247327
…ng tests r=sefeng This patch was approved in web-platform-tests/wpt#51630. But unfortunately I couldn't manage to land it there due to some intermittent Firefox and Chrome test failures that are unrelated to the changes I was making. Differential Revision: https://phabricator.services.mozilla.com/D247327
This patch was approved in #51630. But unfortunately I couldn't manage to land it there due to some intermittent Firefox and Chrome test failures that are unrelated to the changes I was making. Differential Revision: https://phabricator.services.mozilla.com/D247327 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1956598 gecko-commit: 77a4f757045738f11a562db9fdc6b420024ccc3e gecko-reviewers: sefeng
This patch was approved in #51630. But unfortunately I couldn't manage to land it there due to some intermittent Firefox and Chrome test failures that are unrelated to the changes I was making. Differential Revision: https://phabricator.services.mozilla.com/D247327 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1956598 gecko-commit: 77a4f757045738f11a562db9fdc6b420024ccc3e gecko-reviewers: sefeng
|
I directly landed it on mozilla-central and let it auto upstream instead because of the CI issues. Closing this. |
…ng tests r=sefeng This patch was approved in web-platform-tests/wpt#51630. But unfortunately I couldn't manage to land it there due to some intermittent Firefox and Chrome test failures that are unrelated to the changes I was making. Differential Revision: https://phabricator.services.mozilla.com/D247327
4 participants
This is something we came up while implementing it in Firefox.
We don't fire an orphan pointerup event at all when there is only a pointerup and no pointerdown. So this test was failing. This change adds a timeout mechanism for browsers like Firefox, so we can treat 'no pointerup entry' as a pass.
As a solution, we are sending a non-pointer related event right after the pointerup event, to make sure at least one event is being handled by the test so it doesn't timeout.
It fixes Bug 1956598.