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

Jump to bottom

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

Merged
merged 3 commits into from Jul 11, 2019
Merged

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

merged 3 commits into from Jul 11, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Jun 12, 2019 •
edited

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}

wpt-pr-bot
wpt-pr-bot approved these changes Jun 12, 2019
View reviewed changes
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Compare June 19, 2019 17:04
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1618306 branch 6 times, most recently from 1dd98fe to b9bba2b Compare June 27, 2019 14:57
@EiraGe @chromium-wpt-export-bot
Move movementX/Y calculation for aura pointer-locked state to Blink.
e83be16 
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
Copy link
Contributor

lukebjerring commented Jul 4, 2019 •
edited

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
Copy link
Contributor

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
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
Copy link
Contributor

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

@Hexcles
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
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
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.

EiraGe
EiraGe suggested changes Jul 8, 2019
View reviewed changes
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html Outdated Show resolved Hide resolved
@Hexcles @EiraGe
Apply suggestions from code review
480443e 
Co-Authored-By: Ella Ge <eirage@chromium.org>
@Hexcles
Copy link
Member

Hexcles commented Jul 8, 2019

ChromeDriver is now complaining about "invalid input state": https://tools.taskcluster.net/groups/bnj45Fo0RC6rrcMtRGHPWQ/tasks/UcyccTzdRImzE09Lx3nVDQ/runs/0/logs/public%2Flogs%2Flive.log#L7428

@EiraGe
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
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

@Hexcles @EiraGe
Update pointerevents/pointerlock/pointerevent_movementxy_with_pointer…
183b4a0 
…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
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
@Hexcles Hexcles deleted the chromium-export-cl-1618306 branch July 11, 2019 18:01
@EiraGe
Copy link
Contributor

EiraGe commented Jul 11, 2019

Thanks Robert for looking into this

natechapin pushed a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
@chromium-wpt-export-bot @natechapin
Move movementX/Y calculation for aura pointer-locked state to Blink. (w…
38318ef 
…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
Reviewers

@EiraGe EiraGe EiraGe requested changes

@wpt-pr-bot wpt-pr-bot wpt-pr-bot approved these changes

Assignees
No one assigned
Labels
chromium-export pointerevents wg-pointerevents
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@chromium-wpt-export-bot @lukebjerring @EiraGe @Hexcles @wpt-pr-bot @KyleJu