Skip to content

Gamepad: Implement gamepad buttons to be array, removing GamepadList#44357

Merged
Gae24 merged 3 commits into
servo:mainfrom
rovertrack:issue-44333
Apr 24, 2026
Merged

Gamepad: Implement gamepad buttons to be array, removing GamepadList#44357
Gae24 merged 3 commits into
servo:mainfrom
rovertrack:issue-44333

Conversation

@rovertrack
Copy link
Copy Markdown
Contributor

@rovertrack rovertrack commented Apr 19, 2026

Replaced GamepadButtonList interface with a frozen array of gamepad buttons to match the gamepad webidl specification.

Testing: No idlharness tests possible since the values can't be accessed without a real gamepad.
Fixes: #44333

@rovertrack rovertrack requested a review from gterzian as a code owner April 19, 2026 15:58
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 19, 2026
@rovertrack rovertrack changed the title Implement gamepad buttons to be array removing GamepadList Gamepad: Implement gamepad buttons to be array, removing GamepadList Apr 19, 2026
@mrobinson
Copy link
Copy Markdown
Member

@rovertrack The Testing field should not contain a screenshot, but describe the automated tests that test this change.

Comment thread components/script/dom/gamepad/gamepad.rs Outdated
Comment thread components/script/dom/gamepad/gamepad.rs Outdated
Comment thread components/script/dom/gamepad/gamepad.rs Outdated
Comment thread components/script/dom/gamepad/gamepad.rs
@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 19, 2026
@rovertrack
Copy link
Copy Markdown
Contributor Author

Thanks @Gae24 , all suggested improvements have been made
now the init_button returns buttons and the conversion to inline with rooted_vec! happens in the constructor itself!

Copy link
Copy Markdown
Contributor

@Gae24 Gae24 left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread components/script/dom/gamepad/gamepad.rs Outdated
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 20, 2026
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 20, 2026
@rovertrack rovertrack requested a review from Gae24 April 20, 2026 09:13
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 20, 2026
@rovertrack
Copy link
Copy Markdown
Contributor Author

is there any changes to be done on this @jdm?

@Gae24
Copy link
Copy Markdown
Contributor

Gae24 commented Apr 22, 2026

is there any changes to be done on this @jdm?

Actually could you check if tests/wpt/tests/gamepad/idlharness-manual.html cover these changes?

readonly attribute DOMString mapping;
readonly attribute Float64Array axes;
[SameObject] readonly attribute GamepadButtonList buttons;
readonly attribute any buttons;
Copy link
Copy Markdown
Contributor Author

@rovertrack rovertrack Apr 23, 2026

Choose a reason for hiding this comment

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

I think instead of the suggested "any" this must be FrozenArray<GamepadButton>so it checks the type of the buttons in

if (type.generic == "sequence" || type.generic == "FrozenArray") {
return this.is_json_type(idlType[0]);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Our webidl codegen doesn't support FrozenArray types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarity 🙂

@jdm
Copy link
Copy Markdown
Member

jdm commented Apr 23, 2026

Replaced the Gamepadbuttonlist type with any to implement Frozenarray - components/script_bindings/webidls/Gamepad.webidl
made the buttons usage in components/script/dom/gamepad/gamepad.rs be implemented with CachedFrozenArray so that the buttons are cached frozen_buttons. replacing dependency of Gamepadbuttonlist
Implemented a init_buttons same as the one with Gamepadlist.rs to remove, Removed Gampadbuttonlist completely

Tested with a physical controller
servo:

Screenshot 2026-04-19 211908

I'm editing the original description to include a more clear commit message (since it will be used for the squashed commits). Here's the original message for posterity.

@jdm jdm added this pull request to the merge queue Apr 23, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 23, 2026
@rovertrack
Copy link
Copy Markdown
Contributor Author

Replaced the Gamepadbuttonlist type with any to implement Frozenarray - components/script_bindings/webidls/Gamepad.webidl
made the buttons usage in components/script/dom/gamepad/gamepad.rs be implemented with CachedFrozenArray so that the buttons are cached frozen_buttons. replacing dependency of Gamepadbuttonlist
Implemented a init_buttons same as the one with Gamepadlist.rs to remove, Removed Gampadbuttonlist completely

Tested with a physical controller
servo:

Screenshot 2026-04-19 211908 I'm editing the original description to include a more clear commit message (since it will be used for the squashed commits). Here's the original message for posterity.

Thanks @jdm.

@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 23, 2026
@servo-highfive servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 23, 2026
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 23, 2026
@rovertrack
Copy link
Copy Markdown
Contributor Author

@jdm Merge Failed due to GamePadButtonList being present in tests while this PR completely removes it, I have updated the PR to remove the test expectations!

@servo-highfive servo-highfive added the S-needs-rebase There are merge conflict errors. label Apr 23, 2026
@jdm
Copy link
Copy Markdown
Member

jdm commented Apr 24, 2026

Excellent, thanks! This just needs to be rebased against main and run ./mach update-manifest again.

Signed-off-by: Rover track <rishan.pgowda@gmail.com>
Signed-off-by: Rover track <rishan.pgowda@gmail.com>
@servo-highfive servo-highfive removed the S-needs-rebase There are merge conflict errors. label Apr 24, 2026
Signed-off-by: Rover track <rishan.pgowda@gmail.com>
@rovertrack rovertrack requested a review from jdm April 24, 2026 08:03
@rovertrack
Copy link
Copy Markdown
Contributor Author

Excellent, thanks! This just needs to be rebased against main and run ./mach update-manifest again.

Rebased and updated the manifest!

@Gae24 Gae24 added this pull request to the merge queue Apr 24, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 24, 2026
Merged via the queue into servo:main with commit d812d2f Apr 24, 2026
30 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 24, 2026
@rovertrack
Copy link
Copy Markdown
Contributor Author

Thanks @Gae24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gamepad: buttons should be Array, but is GamepadList instead

5 participants