Gamepad: Align closer to spec and implement missing slots #31385

msub2 · 2024-02-20T03:17:39Z

This follows up from the initial Gamepad implementation and addresses some TODOs around gamepad.[[exposed]] and navigator.[[hasGamepadGesture]], as well as some missing spec steps. It also properly gates gamepad handling behind the dom.gamepad.enabled pref, as previously the gamepadconnected and gamepaddisconnected events would fire regardless.

This also allows one more WPT test to pass, gamepad/not-fully-active.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

github-actions · 2024-02-23T08:48:50Z

🔨 Triggering try run (#8016745639) for Linux WPT

github-actions · 2024-02-23T09:33:24Z

Test results for linux-wpt-layout-2020 from try job (#8016745639):

Flaky unexpected result (15)

TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-with-non-reserved-words.html (#16216)
- NOTRUN [expected PASS] subtest: Overall test
TIMEOUT [expected OK] /_webgl/conformance/uniforms/out-of-bounds-uniform-array-access.html (#26225)
- NOTRUN [expected PASS] subtest: Overall test
OK /css/cssom-view/MediaQueryList-addListener-removeListener.html (#24569)
- PASS [expected FAIL] subtest: listeners are called correct number of times
OK /css/cssom-view/scroll-behavior-smooth-navigation.html (#29564)
- FAIL [expected PASS] subtest: Smooth scrolling while doing history navigation. assert_not_equals: Shouldn't be scrolled to top anymore. got disallowed value 0
TIMEOUT [expected OK] /html/browsers/browsing-the-web/history-traversal/srcdoc/consecutive-srcdoc.html (#29084)
- TIMEOUT [expected FAIL] subtest: changing srcdoc to about:srcdoc#yo then another srcdoc does two push navigations and we can navigate back Test timed out
OK /html/browsers/browsing-the-web/navigating-across-documents/empty-iframe-load-event.html (#29066)
- FAIL [expected PASS] subtest: Check execution order from nested timeout assert_equals: Expected nested setTimeout to run second expected true but got false
- FAIL [expected PASS] subtest: Check execution order on load handler assert_equals: Expected onload to run first expected false but got true
TIMEOUT [expected OK] /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-referrer.window.html (#29081)
- TIMEOUT [expected PASS] subtest: no-referrer referrer policy used to create the starting page Test timed out
FAIL [expected PASS] /html/rendering/non-replaced-elements/the-page/iframe-scrolling-attribute-values.html
CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
- PASS [expected FAIL] subtest: text/plain: 0x00 in value (normal form)
OK [expected ERROR] /html/semantics/scripting-1/the-script-element/defer-script/async-script.html?reload (#29054)
TIMEOUT /html/webappapis/scripting/events/compile-event-handler-settings-objects.html (#24246)
- TIMEOUT [expected FAIL] subtest: The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document Test timed out
OK [expected TIMEOUT] /webmessaging/with-ports/017.html (#24486)
- PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, about:blank
TIMEOUT [expected OK] /webmessaging/without-ports/018.html (#24485)
- TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, javascript: Test timed out
ERROR [expected OK] /workers/baseurl/alpha/import-in-moduleworker.html (#21315)

Stable unexpected results that are known to be intermittent (17)

TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
- NOTRUN [expected PASS] subtest: Overall test
TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
- TIMEOUT [expected PASS] subtest: background-image sec-fetch-site - HTTPS downgrade (header not sent) Test timed out
TIMEOUT /fetch/metadata/generated/element-img-environment-change.sub.html (#30111)
- PASS [expected FAIL] subtest: sec-fetch-site - Not sent to non-trustworthy same-origin destination, no attributes
OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html (#28681)
- PASS [expected FAIL] subtest: load & pageshow events do not fire on contentWindow of <iframe> element created with src='about:blank'
OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-window-open.html (#28691)
- FAIL [expected PASS] subtest: load event does not fire on window.open('about:blank') assert_unreached: load should not be fired Reached unreachable code
OK /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling-dynamic.html (#28066)
- FAIL [expected PASS] subtest: 0041 set in href="" targeting a frame and clicked assert_equals: expected "A" but got ""
- FAIL [expected PASS] subtest: 0080 00FF set in href="" targeting a frame and clicked assert_equals: expected "�ÿ" but got ""
- FAIL [expected PASS] subtest: 0080 00FF 0100 set in href="" targeting a frame and clicked assert_equals: expected "�ÿĀ" but got ""
- FAIL [expected PASS] subtest: D83D DE0D set in href="" targeting a frame and clicked assert_equals: expected "😍" but got ""
OK /html/browsers/history/the-history-interface/traverse_the_history_4.html (#21383)
- PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
TIMEOUT [expected OK] /html/infrastructure/urls/base-url/document-base-url-window-initiator-is-not-opener.https.window.html (#30970)
CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
- FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
FAIL [expected PASS] /html/semantics/embedded-content/the-img-element/image-loading-lazy-slow.html (#30684)
TIMEOUT [expected OK] /html/semantics/forms/form-submission-0/reparent-form-during-planned-navigation-task.html (#29724)
- TIMEOUT [expected PASS] subtest: reparent-form-during-planned-navigation-task Test timed out
OK /html/semantics/forms/historical.html (#28568)
- FAIL [expected PASS] subtest: <input name=isindex> should not be supported assert_regexp_match: expected object "/?isindex=x$/" but got "about:blank"
TIMEOUT /resource-timing/test_resource_timing.html (#25720)
- PASS [expected FAIL] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (img)
OK [expected TIMEOUT] /webmessaging/with-ports/018.html (#24485)
- PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, javascript:
TIMEOUT [expected OK] /webmessaging/without-ports/017.html (#24486)
- TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, about:blank Test timed out

github-actions · 2024-02-23T09:33:36Z

✨ Try run (#8016745639) succeeded.

components/script/dom/globalscope.rs

Taym95 · 2024-02-25T16:48:18Z

components/script/dom/globalscope.rs

+                                                }),
+                                                window.upcast(),
+                                            )
+                                            .unwrap();


maybe you should check if gamepad_task_source is not None to avoid unwrap

This is about the queuing of the task, but we should expect with a clear message.

components/script/dom/navigator.rs

components/script/dom/gamepad.rs

- Fixes TODO's from initial gamepad implementation - Adds some missing spec steps

gterzian

Great start! Couple of comments...

components/script/dom/navigator.rs

components/script/dom/gamepadlist.rs

components/script/dom/globalscope.rs

gterzian · 2024-03-06T13:42:13Z

components/script/dom/globalscope.rs

+                                                }),
+                                                window.upcast(),
+                                            )
+                                            .unwrap();


This is about the queuing of the task, but we should expect with a clear message.

components/script/dom/globalscope.rs

- Change should_notify to has_gesture - Add spec links and TODO to navigator - Remove unneeded clone from GamepadList::list - Move gamepadconnected event firing into has_gesture block

gterzian

LGTM with one nit.

components/script/dom/globalscope.rs

gterzian

Ok thanks for the refactor, just one last thing and then good to go!

components/script/dom/globalscope.rs

gterzian

LGTM, one nit.

gterzian · 2024-03-11T16:17:01Z

components/script/dom/gamepad.rs

+        GamepadUpdateType::Button(_, value) => {
+            return value > BUTTON_PRESS_THRESHOLD;
+        },
+    };
 }


Let's just add a newline here.

A second one? It ends at 297

Ah ok, should be fine then...

components/script/dom/gamepad.rs

gterzian

Thank you!

msub2 mentioned this pull request Feb 20, 2024

Pass WPT tests for Gamepad and catch up to spec #31362

Open

4 tasks

msub2 force-pushed the gamepad branch from eea1c55 to e76cf18 Compare February 21, 2024 04:48

mrobinson added the T-linux-wpt-2020 Do a try run of the WPT label Feb 23, 2024

github-actions bot removed the T-linux-wpt-2020 Do a try run of the WPT label Feb 23, 2024

Taym95 reviewed Feb 25, 2024

View reviewed changes

components/script/dom/globalscope.rs Outdated Show resolved Hide resolved

Taym95 reviewed Feb 25, 2024

View reviewed changes

components/script/dom/navigator.rs Show resolved Hide resolved

Taym95 reviewed Feb 25, 2024

View reviewed changes

components/script/dom/gamepad.rs Show resolved Hide resolved

msub2 added 7 commits March 2, 2024 20:23

Implement missing gamepad slots, align to spec more

1fbe07e

- Fixes TODO's from initial gamepad implementation - Adds some missing spec steps

Only handle gamepad events when pref is enabled

d68dcd6

Return empty list in getGamepads if document not active

a5ae14e

./mach fmt

c6dac32

Update getGamepads to return an array instead of GamepadList

05cb3d5

Add spec link for [[exposed]] slot

b1c015b

Remove failing test expectations for not-fully-active

21f565d

msub2 force-pushed the gamepad branch from f1db2bb to 21f565d Compare March 3, 2024 06:26

gterzian self-requested a review March 5, 2024 09:51

gterzian requested changes Mar 6, 2024

View reviewed changes

gterzian reviewed Mar 6, 2024

View reviewed changes

components/script/dom/globalscope.rs Outdated Show resolved Hide resolved

msub2 added 3 commits March 6, 2024 15:17

A few fixes

8c24672

- Change should_notify to has_gesture - Add spec links and TODO to navigator - Remove unneeded clone from GamepadList::list - Move gamepadconnected event firing into has_gesture block

Use queue_with_canceller for tasks and add expects

5e97ecd

Explicitly check for gamepad user gesture

c7bcf0f

msub2 requested a review from gterzian March 7, 2024 06:59

gterzian approved these changes Mar 7, 2024

View reviewed changes

components/script/dom/globalscope.rs Outdated Show resolved Hide resolved

Move user gesture check into separate function

ddce1c3

gterzian requested changes Mar 8, 2024

View reviewed changes

components/script/dom/globalscope.rs Outdated Show resolved Hide resolved

Change contains_user_gesture to be a gamepad function

ff67a7e

mach fmt

5041099

msub2 requested a review from gterzian March 9, 2024 03:01

gterzian approved these changes Mar 11, 2024

View reviewed changes

gterzian reviewed Mar 11, 2024

View reviewed changes

components/script/dom/gamepad.rs Outdated Show resolved Hide resolved

Change axis/button threshold constants to be private to module

bea5f15

gterzian approved these changes Mar 12, 2024

View reviewed changes

gterzian added this pull request to the merge queue Mar 12, 2024

Merged via the queue into servo:main with commit 48fa77d Mar 12, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gamepad: Align closer to spec and implement missing slots #31385

Gamepad: Align closer to spec and implement missing slots #31385

msub2 commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

Taym95 Feb 25, 2024 •

edited

Loading

gterzian Mar 6, 2024

gterzian left a comment

gterzian Mar 6, 2024

gterzian left a comment

gterzian left a comment

gterzian left a comment

gterzian Mar 11, 2024

msub2 Mar 11, 2024

gterzian Mar 11, 2024

gterzian left a comment

Gamepad: Align closer to spec and implement missing slots #31385

Gamepad: Align closer to spec and implement missing slots #31385

Conversation

msub2 commented Feb 20, 2024 • edited Loading

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

Taym95 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

gterzian Mar 6, 2024

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian Mar 6, 2024

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian Mar 11, 2024

Choose a reason for hiding this comment

msub2 Mar 11, 2024

Choose a reason for hiding this comment

gterzian Mar 11, 2024

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

msub2 commented Feb 20, 2024 •

edited

Loading

Taym95 Feb 25, 2024 •

edited

Loading