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.

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

Move movementX/Y calculation for aura pointer-locked state to Blink. #17295

Merged
merged 3 commits into from Jul 11, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Jun 12, 2019

This change moves movementX/Y calculation for aura pointer-locked
state to Blink.
On aura, pointer lock use "move to center" to make cursor stays in
the window. With the calculation done in blink side, the move to
center event is marked as synthesize move, so that we can update
the blink side states and do not dispatch the event.

The change is under content feature flag kConsolidatedMovementXY

Bug: 802067
Change-Id: I05360dadd18a2f41481a0de9ef78a05199493857
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1618306
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#674237}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Move aura pointer locked state movementX/Y calculate to blink Move movementX/Y calculation for aura pointer-locked state to Blink. Jun 13, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1618306 branch 7 times, most recently from ce943b1 to 9cfca30 Jun 13, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1618306 branch 6 times, most recently from 1dd98fe to b9bba2b Jun 21, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1618306 branch from b9bba2b to ef1c152 Jul 2, 2019
This change moves movementX/Y calculation for aura pointer-locked
state to Blink.
On aura, pointer lock use "move to center" to make cursor stays in
the window. With the calculation done in blink side, the move to
center event is marked as synthesize move, so that we can update
the blink side states and do not dispatch the event.

The change is under content feature flag kConsolidatedMovementXY

Bug: 802067
Change-Id: I05360dadd18a2f41481a0de9ef78a05199493857
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1618306
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#674237}
@lukebjerring

This comment has been minimized.

Copy link
Contributor

lukebjerring commented Jul 4, 2019

Chrome instability:

Test Subtest Results Messages
/pointerevents/pointerevent_touch-action-keyboard.html CRASH: 1/10, OK: 9/10
/pointerevents/pointerevent_touch-action-keyboard.html touch-action attribute test PASS: 9/10, MISSING: 1/10
/pointerevents/pointerlock/pointerevent_coordinates_when_locked.html CRASH: 1/10, OK: 9/10
/pointerevents/pointerlock/pointerevent_coordinates_when_locked.html mouse Test pointerevent coordinates when pointer is locked PASS: 9/10, MISSING: 1/10
@lukebjerring

This comment has been minimized.

Copy link
Contributor

lukebjerring commented Jul 4, 2019

FF instability:

Unstable results

Test Subtest Results Messages
/pointerevents/extension/pointerevent_getCoalescedEvents_when_pointerlocked.html mouse pointermove getCoalescedEvents when lock test FAIL: 6/10, PASS: 4/10 assert_true: document.pointerLockElement should have coalesced events. expected true got false
/pointerevents/pointerevent_setpointercapture_inactive_button_mouse.html OK: 9/10, TIMEOUT: 1/10
/pointerevents/pointerevent_setpointercapture_inactive_button_mouse.html setPointerCapture + inactive button state FAIL: 9/10, MISSING: 1/10 InvalidPointerId: Invalid pointer id.
/pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html mouse pointerevent movementX/Y with pointerlock test FAIL: 1/10, PASS: 9/10 assert_equals: expected 408 but got 640
@EiraGe

This comment has been minimized.

Copy link
Contributor

EiraGe commented Jul 4, 2019

I wonder how would my patch cause 3 unrelated test failing on FF?
And the chrome result is failing the test that is removed in this patch?

Can we re-run this?

@lukebjerring lukebjerring reopened this Jul 5, 2019
@lukebjerring

This comment has been minimized.

Copy link
Contributor

lukebjerring commented Jul 5, 2019

Sure, closed and reopened to trigger a re-run.

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jul 8, 2019

There's a bunch of unrelated tests here because of the change to a commonly used support file.

The only relevant flakiness seems to be: mouse pointerevent movementX/Y with pointerlock test
https://tools.taskcluster.net/groups/N1TFFgzeRiyxiWDBwMQMGw/tasks/DtaXTG4-Tm6vg-miXwjkdw/runs/0/logs/public%2Flogs%2Flive.log#L46551 @EiraGe

@EiraGe

This comment has been minimized.

Copy link
Contributor

EiraGe commented Jul 8, 2019

I think I figure out the reason why the added test failing on FF. How should I make the change? Can I do the change with a chromium CL?

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jul 8, 2019

@EiraGe if it's a substantial change, you can send a follow-up Chromium CL and I'll force-merge this one; if it's just a simple fix, you can propose the change directly in this PR on GitHub.

Co-Authored-By: Ella Ge <eirage@chromium.org>
@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jul 8, 2019

@EiraGe

This comment has been minimized.

Copy link
Contributor

EiraGe commented Jul 8, 2019

ChromeDriver is now complaining about "invalid input state":

Yes, that's the same result as Luck pointed out before the change. It's not the test changed in this CL (sorry for the similar name)

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jul 8, 2019

Ahh sorry I didn't look at it closely enough.

But it seems we haven't fixed the Firefox issue :( https://tools.taskcluster.net/groups/bnj45Fo0RC6rrcMtRGHPWQ/tasks/TTP8iKO1QPOhd2V25njlrA/runs/0/logs/public%2Flogs%2Flive.log#L46674

…lock.html

Co-Authored-By: Ella Ge <eirage@chromium.org>
@KyleJu KyleJu closed this Jul 11, 2019
@KyleJu KyleJu reopened this Jul 11, 2019
@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jul 11, 2019

I spent some time trying to reproduce the flaky results of pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html locally, and was only able to do so with the headless mode disabled and constantly moving the mouse while running the tests. This might be another window focus issue and/or related to #14485 . Regardless, it seems to me that without other input interference, the test itself is stable; and input interference is likely an infra issue (Xvfb, fluxbox, etc.). Therefore, I'm going to manually squash-merge this test.

@Hexcles Hexcles merged commit 2931433 into master Jul 11, 2019
6 of 12 checks passed
6 of 12 checks passed
manifest-build-and-tag manifest-build-and-tag
Details
website-build-and-publish website-build-and-publish
Details
Taskcluster (pull_request) TaskGroup: failure
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20190711.70 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
@Hexcles Hexcles deleted the chromium-export-cl-1618306 branch Jul 11, 2019
@EiraGe

This comment has been minimized.

Copy link
Contributor

EiraGe commented Jul 11, 2019

Thanks Robert for looking into this

natechapin added a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
…eb-platform-tests#17295)

This change moves movementX/Y calculation for aura pointer-locked
state to Blink.
On aura, pointer lock use "move to center" to make cursor stays in
the window. With the calculation done in blink side, the move to
center event is marked as synthesize move, so that we can update
the blink side states and do not dispatch the event.

The change is under content feature flag kConsolidatedMovementXY

Bug: 802067
Change-Id: I05360dadd18a2f41481a0de9ef78a05199493857
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1618306
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#674237}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.