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

Add extended buttons to support trackpad (#191) #196

Open
wants to merge 4 commits into
base: gh-pages
Choose a base branch
from

Conversation

xingri
Copy link

@xingri xingri commented Mar 5, 2024

Closes #191

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

extensions.html Outdated
@@ -649,6 +649,168 @@ <h2>
</p>
</dd>
</section>
<section>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's define extended buttons in the main spec document (index.html). The extensions document is not officially part of the spec and we want to eventually remove it.

The spec PR should extend GamepadButton to add a new attribute (perhaps called GamepadButton.name) and an algorithm for the getter. The type of the new attribute will be a new enum (perhaps called GamepadButtonName). The enum type should be defined in the spec along with all its values.

Add steps to initialize the new attribute when initializing buttons. The steps should define what to do if a gamepad is missing a button defined in the Standard Gamepad. I think it makes sense to use a special value for these buttons so applications can identify which buttons are not actually mapped. (Ideally we'd set the button to null, but this would break existing apps which don't check for null.)

Also, initialization steps should define what to do if a gamepad has a button that doesn't have a predefined Standard Gamepad index or enum value. Do we enumerate it? If so, what should the new attribute's value be? Perhaps we can add a special "unrecognized" value for this.

extensions.html Outdated
</h2>
<p>
This section defines extended gamepad buttons mapping beyonds the Standard Gamepads layout,
but are commonly found on some gamepad models .The <code>_ext</code> suffix is used to indicate
Copy link
Collaborator

Choose a reason for hiding this comment

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

The _ext suffix comes from OpenXR's input subpaths specification:

https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#_adding_input_sources_via_extensions

We should decide whether this attribute uses OpenXR input subpaths or something new that we want to define for ourselves. If we're using OpenXR subpaths then we should explicitly link to the OpenXR spec which explains how to extend it. If we're only using it for inspiration then I think we should drop the _ext suffix.

extensions.html Outdated
<td>Standard: Center button in center cluster</td>
</tr>
<tr>
<td>buttons[17]</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This suggests the "trackpad" button can only appear at buttons[17]. My thinking is that we should not continue the current pattern of assigning meaning to button indices, otherwise the buttons array will be very sparse since most gamepads do not share the same extra buttons. Instead of using button indices to identify buttons, apps can check the new enum attribute.

For this non-normative section, I think it will be clearer if we provide a separate table for each gamepad instead of trying to combine them all into a single table.

Copy link
Author

Choose a reason for hiding this comment

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

@nondebug Thanks for the review. Will update the PR based on the feedback shortly.

Copy link
Author

Choose a reason for hiding this comment

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

@nondebug I have updated the PR based on your feedback. I have added the enum for identifying the gamepad buttons. Please review this changes and share your opinions.

@xingri xingri force-pushed the trackpad-button-support branch 6 times, most recently from 5e9abce to ab39929 Compare March 13, 2024 22:07
index.html Outdated
</pre>
<dl>
<dt>
<dfn>`"extended"`</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Introducing a new value for GamepadMappingType will break applications that currently check for "standard". I don't think it's necessary to add a new enum value for this, applications can discover whether the gamepad supports extra buttons by inspecting the buttons array.

index.html Outdated
</p>
<pre class="idl">
partial interface GamepadButton {
readonly attribute GamepadButtonType type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move the definition to the main interface GamepadButton IDL section.

index.html Outdated
</tr>
<tr>
<td>
<dfn data-dfn-for="GamepadButton">[[\type]]</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need an internal slot.

For other attributes we use a slot because the attribute is read-only but the implementation needs to be able to change the value. In this case the value should never change after initialization.

index.html Outdated
</table>
<dl data-dfn-for="GamepadButton">
<dt>
<dfn>type</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

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

<dfn>type</dfn> attribute

index.html Outdated
<dfn>GamepadButtonType</dfn> Enum
</h3>
<p>
This enum defines the set of possible botton types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

botton -> button

index.html Outdated
"", /* unknown, or not applicable */
"trackpad",
"share",
"..."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit "..." since it is not a valid enum value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The diagrams are difficult to read. For clarity I think we should provide this information as a table instead of a diagram.

index.html Outdated
<dfn>type</dfn>
</dt>
<dd>
An enumeration, {{GamepadButtonType}}, that indicates which button types the controller
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description for the type attribute needs to be updated

Copy link
Author

Choose a reason for hiding this comment

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

@nondebug Again thank you for the review. I have no objections and will update this PR based on your feedbacks.

index.html Outdated
`undefined`
</td>
<td>
Indicates the button type the controller is held in.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Indicates the button type the controller is held in.
The button type of the controller.

index.html Outdated
</p>
<pre class="idl">
enum GamepadButtonType {
"", /* unknown, or not applicable */
Copy link
Member

Choose a reason for hiding this comment

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

Better to make it explicit.

Suggested change
"", /* unknown, or not applicable */
"unknown",

And if not applicable, maybe we should make . type nullable?

index.html Show resolved Hide resolved
<dd>
An enumeration, {{GamepadButtonType}}, that indicates which button types the controller
is being held in.
</dd>

Choose a reason for hiding this comment

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

Looks like we're still missing getter definitions, no?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, though it's implied... but yeah, if the others have the getter steps (what it was initialized to) then so should this.

index.html Outdated
</dt>
<dd>
An enumeration, {{GamepadButtonType}}, that indicates which button types the controller
is being held in.

Choose a reason for hiding this comment

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

Is this phrasing in line with other specs? There's an open spec issue regarding consistency in enum phrasing which makes me think this would be better phrased as:

An enumerated {{GamepadButtonType}} attribute that classifies the current button's type in relation to an extended mapping

Copy link
Author

Choose a reason for hiding this comment

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

updated.

index.html Outdated
Comment on lines 1988 to 2071
<th scope="col">Gamepad Models</th>
</tr>

Choose a reason for hiding this comment

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

It feels like there's some disconnect with this table and the available GamepadButtonType enums. Are we missing a possible third column which maps each of these button to a respective GamepadButtonType?

index.html Show resolved Hide resolved
index.html Outdated
</tr>
<tr>
<td>home</td>
<td>Nvidia Shield Gamepad</td>

Choose a reason for hiding this comment

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

The Xbox and PlayStation controllers have their own respective home buttons. Are we intending to add those to this list or is there some special reason why we're classifying this separately for the Shield gamepad?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. updated.

index.html Outdated
<dfn>unknown</dfn>
</dt>
<dd>
This indicates button type is unknown or not applicable.

Choose a reason for hiding this comment

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

Is there a tangible benefit for User Agents to populate all the GamepadButtons with unknown for any buttons that can't be classified as one of the other types? Would it be better to simply have the attribute as undefined when that's the case?

Copy link
Author

Choose a reason for hiding this comment

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

@mrmcpowned Thanks for the review. I agree with your feedbacks. Will update this changes shortly.

Copy link
Author

Choose a reason for hiding this comment

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

Agree with your feedback @mrmcpowned, updated.

@xingri xingri force-pushed the trackpad-button-support branch 3 times, most recently from 94f73da to 64b7a26 Compare March 28, 2024 23:49
index.html Outdated
<tbody>
<tr>
<td>trackpad/touchpad</td>
<td>Sony DualSense, DualShock 4</td>
Copy link
Member

Choose a reason for hiding this comment

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

As I imagine these products will change a lot over time, so these should probably be in Wiki page (or if we want to formalize it, and actual registry). We can point the spec to the wiki page.

Copy link
Author

Choose a reason for hiding this comment

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

@marcoscaceres Thanks for the feedback. I have updated the rest of the comments.
I agree with you that it will keep changing and it would be great to have the link inside of the gamepad working group wiki. If this is not a hard requirement, I would like to follow up creating the wiki page on the different button mappings of the game controllers mentioned in this table as a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm, let's remove the table from this PR and follow up with the wiki page.

index.html Outdated
<dfn>trackpad</dfn>
</dt>
<dd>
This button is being assigned to trackpad button.
Copy link
Member

Choose a reason for hiding this comment

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

Probably just ok to say:

Suggested change
This button is being assigned to trackpad button.
A trackpad button.

Same for the rest.

index.html Outdated
This section introduces an extended gamepad button mapping beyond the Standard Gamepad layout.
These additional buttons are commonly found on certain gamepad models.
The following table defines the extended buttons used by some gamepad models in the following diagrams.
It’s important to note that this list is not exhaustive, and user agents may utilize different
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Its important to note that this list is not exhaustive, and user agents may utilize different
It's important to note that this list is not exhaustive, and user agents may utilize different

index.html Outdated
The following table defines the extended buttons used by some gamepad models in the following diagrams.
It’s important to note that this list is not exhaustive, and user agents may utilize different
or additional buttons for these or other gamepad models.
Consequently, the number of buttons on the Gamepad is not limited to the standard mapping of 17 buttons.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Consequently, the number of buttons on the Gamepad is not limited to the standard mapping of 17 buttons.
Consequently, the number of buttons on the {{Gamepad}} is not limited to the standard mapping of 17 buttons.

index.html Outdated
</p>
<p>
To accommodate extended gamepads, we have incorporated an "extended" mapping enumeration
into the GamepadMappingType. Additionally, we have defined an enumeration for the various
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
into the GamepadMappingType. Additionally, we have defined an enumeration for the various
into the {{GamepadMappingType}}. Additionally, we have defined an enumeration for the various

index.html Outdated
<p>
To accommodate extended gamepads, we have incorporated an "extended" mapping enumeration
into the GamepadMappingType. Additionally, we have defined an enumeration for the various
button types in the table, termed "GamepadButtonType", and have expanded the "GamepadButton"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
button types in the table, termed "GamepadButtonType", and have expanded the "GamepadButton"
button types in the table, termed {{GamepadButtonType}}, and have expanded the {{GamepadButton}}

index.html Outdated
@@ -952,6 +953,17 @@ <h2>
.. 1.0]
</td>
</tr>
<tr>
<td>
<dfn data-dfn-for="GamepadButton">[[\type]]</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the internal slot [[type]] isn't needed and can be removed.

Normally we use internal slots for read-only attributes that the user agent needs to modify. We expect a GamepadButton created with a button type will always have that type, so we don't need a way to modify it.

index.html Outdated
<tbody>
<tr>
<td>trackpad/touchpad</td>
<td>Sony DualSense, DualShock 4</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm, let's remove the table from this PR and follow up with the wiki page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add steps to the "initialize buttons" steps to initialize the button type.

https://w3c.github.io/gamepad/#dfn-initializing-buttons

Currently we initialize buttons in two phases. First, determine how many buttons are needed. Then, create all the buttons.

We'll need to remember the button type from the first phase so we can initialize the GamepadButton in the second phase. We also need to handle some special cases:

A GamepadButton may be created without a corresponding button input on the gamepad. (For instance, a gamepad may not have all the Standard Gamepad buttons; the button array entry for a missing button will still have a GamepadButton object but its value should never change.) We can initialize GamepadButton.type to null to represent this case.

A gamepad may have buttons that don't have a canonical button type. We should create a special GamepadButtonType enum value to signal that the button exists but doesn't have a canonical name yet.

Copy link
Author

Choose a reason for hiding this comment

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

@nondebug Thanks for the feedback. Added steps for considering the button types.

index.html Outdated
<p>
This section introduces an extended gamepad button mapping beyond the Standard Gamepad layout.
These additional buttons are commonly found on certain gamepad models.
The following table defines the extended buttons used by some gamepad models.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The following table defines the extended buttons used by some gamepad models.
The following definition list defines the <dfn>extended gamepad buttons</dfn> used by some gamepad models.

index.html Outdated
Comment on lines 823 to 827
<li>Determine the type of the button at index |rawInputIndex|
[=represents a Standard Gamepad button=] or Extended button
types as described in the {{ExtendedMapping}} using the provided
buttonTypes array. If the button does not have a canonical type,
use GamePadButtonType.non_canonical.
Copy link
Member

Choose a reason for hiding this comment

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

This should be specified as:

Suggested change
<li>Determine the type of the button at index |rawInputIndex|
[=represents a Standard Gamepad button=] or Extended button
types as described in the {{ExtendedMapping}} using the provided
buttonTypes array. If the button does not have a canonical type,
use GamePadButtonType.non_canonical.
<li>Let |button| be the button at index |rawInputIndex|.</li>
<li>Let |type| be the result of determining if the |button|'s type represents a [=Standard Gamepad button|represents a Standard Gamepad button=] or an [=Extended Gamepad button|represents a Extended Gamepad button=]. If it's neither, use {{GamePadButtonType/"non_canonical"}}.
</li>

And <dfn>represents a Standard Gamepad button</dfn> should be defined underneath where "represents a Standard Gamepad button" is defined/

@xingri xingri force-pushed the trackpad-button-support branch 3 times, most recently from 8966102 to 7bcd588 Compare May 9, 2024 22:44
@xingri
Copy link
Author

xingri commented May 9, 2024

Some suggested changes.

@marcoscaceres Thanks for the feedback. Applied most of the change requests.

Regarding the extended mapping, based on the working group discussion on last March with @nondebug while you were away,
we discussed that we will not have the additional mapping type of "extended" for those gamepads that have a mapping that is "standard" plus extended bits. So, we'd keep "standard" but just have more controls.

Let's discuss a little bit during the call. Also I will fix the syntax error soon.

@xingri xingri force-pushed the trackpad-button-support branch 8 times, most recently from 05f7d5d to 23e192c Compare May 10, 2024 22:48
@xingri
Copy link
Author

xingri commented May 10, 2024

@marcoscaceres, @nondebug Please review the updated sentences.
Also linked the webkit & chromium issues.

@xingri xingri requested a review from marcoscaceres May 10, 2024 22:58
index.html Outdated
<li>Let |button| be the button at index |rawInputIndex|.
</li>
<li>Let |type| be the result of determining if the |button| [=represents
a Standard Gamepad button=] or represents [=Additional gamepad buttons=]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use singular in both cases and add a period at the end of the sentence:

a Standard Gamepad button=] or represents an [=additional gamepad button=].

I'm not sure if respec will link the definition correctly if we don't use "Additional gamepad buttons"; if not, we can add a data-lt attribute to the tag below:

<dfn data-lt="additional gamepad button">Additional gamepad buttons</dfn>

index.html Outdated
|buttonsSize| − 1:
<ol>
<li>Determine the |type| of the button from the provided {{GamepadButtonType}}
array, defaulting to null if not specified.
Copy link
Collaborator

Choose a reason for hiding this comment

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

`null`

index.html Outdated
<li>Determine the |type| of the button from the provided {{GamepadButtonType}}
array, defaulting to null if not specified.
</li>
<li>[=list/Append=] a [=new=] {{GamepadButton}} to |buttons|
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should reference the |type| variable we defined above:

<li>Let |button:GamepadButton| be a [=new=] {{GamepadButton}} with its {{GamepadButton/type}} attribute initialized to |type|.
</li>
<li>[=list/Append=] |button| to |buttons|.
</li>

index.html Outdated
|buttons|.
|buttonsSize| − 1:
<ol>
<li>Determine the |type| of the button from the provided {{GamepadButtonType}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Determine the |type| of the button from the provided {{GamepadButtonType}} array

This should be phrased "Let |type| be..." so it's clear we're defining a variable. Also, it's unclear where the GamepadButtonType array came from or how it's being used. If it's provided then it should be passed into this algorithm and given a name, for instance: "To initialize buttons for a gamepad with an array |types| of GamepadButtonType values"

Copy link
Author

Choose a reason for hiding this comment

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

@nondebug thanks for the review and detailed guide. Updated this PR to reflect the feedbacks.

@xingri
Copy link
Author

xingri commented May 17, 2024

@marcoscaceres could you please review the changes and let me know if you need anything further.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@xingri
Copy link
Author

xingri commented Jul 11, 2024

@marcoscaceres , @nondebug As we discussed today, I have removed the table in the section 12.1.
Please review this PR and let me know your feedback.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add trackpad button support
4 participants