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

menus.create.accessKey - Probably wrong data, Firefox does not support this parameter/property #23521

Open
jobisoft opened this issue Jun 26, 2024 · 6 comments
Labels
data:webext Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions

Comments

@jobisoft
Copy link
Contributor

What type of issue is this?

Incorrect support data (example: BrowserX says "86" but support was added in "40")

What information was incorrect, unhelpful, or incomplete?

It is listed here as beeing supported since 63:

"version_added": "63"

The Firefox schema file does not know that parameter:
https://searchfox.org/mozilla-central/rev/2f48061aef8c8976b73749ee845e7b85751f5f2f/browser/components/extensions/schemas/menus.json#193-314

What browsers does this problem apply to, if applicable?

Firefox

What did you expect to see?

"firefox": {
   "version_added": false
 },

Notes:

  • The entire entry for menus.create seems to be not conforming to the official suggestions
  • There is currently no indication in BCD that the parameter accessKey (together with all the other parameters) is actually a property of the createProperties parameter. The linked document suggest to use createProperties_accessKey_parameter.
  • Other places in BCD use simple nested features
menus: {
 createProperties: {
   accessKey : {
   ...
   }
 }
}
  • I do prefer this notation over the suggested notation, since it is easier for clients to access and map the data to the actual schema (linter projects or typeScript definition generator projects).
  • It could be, that the Chrome also does not support this parameter. I have no knowledge if this parameter is at all specified. Where could this be checked?

Did you test this? If so, how?

No. Comparing with the schema file should be sufficient.

Can you link to any release notes, bugs, pull requests, or MDN pages related to this?

No response

Do you have anything more you want to share?

No response

MDN URL

No response

MDN metadata

No response

@Rob--W
Copy link
Member

Rob--W commented Jun 26, 2024

The "access key" feature means recognizing & in the title option and interpreting that as an access key: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/create#title

To reduce confusion it may make sense to move it under the title key in the schema data.

In general the BCD can list features that aren't API properties but references to special behavior. There are no established patterns for documenting that.

@Rob--W Rob--W added the data:webext Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions label Jun 26, 2024
@jobisoft
Copy link
Contributor Author

I offer to work on this, which will probably help me to get a deeper understanding of this matter.

If I am allowed to contribute, may I:

  1. Fix the notation of the properties in the same patch?
  2. Use the nested notation as seen here?

@Rob--W
Copy link
Member

Rob--W commented Jun 30, 2024

What do you mean by fixing the notation of the properties?

Previously I wrote "To reduce confusion it may make sense to move it under the title key in the schema data.", under the assumption that the "title" property was already listed in the BCD. Since it is not there, there is no separate "title" entry in the BCD and suggesting to move it there is not very meaningful.

The BCD already lists a separate description,

"accessKey": {
"__compat": {
"description": "<code>&amp;</code> in <code>title</code> sets access key",

... so the "accessKey" property name does not appear anywhere. Renaming it to something else doesn't add much value at this point.

2. Use the nested notation as seen here?

That was what I was thinking. In this specific case, it is not rendered in the BCD because the intermediate entry is missing the __compat key. I wish that we handled that better... At the least we could render items with compat data, and omit the compat info from intermediate rows (to signal that the compat is unknown).
And it would be nice if we have a unified convention for declaring methods vs properties/parameters (or marking something as a top-level method). Once we have that we could update the {{Compat}} macro to render the BCD with the right name (instead of the current practice which joins the key names together with a dot as separator).

@jobisoft
Copy link
Contributor Author

jobisoft commented Jul 1, 2024

My idea was to use the suggested notation for properties here, and thus do the following renaming:

  • command -> createProperties_command_parameter
  • icons -> createProperties_icons_parameter
  • visible -> createProperties_visible_parameter

I then would add the missing createProperties_title_parameter entry and move the accessKey to become a child of createProperties_title_parameter

My alternative suggestion would be to not use the <paramName>_<propName>_parameter notation, but instead create a new createProperties entry and move all the listed properties as children (without any special name convention), as it is done here. I would then propose to officially add that notation as a suggested notation of properties as well.

The currently used notation makes it difficult for clients to work with BCD, there is no indication that command, icons and visible are properties of the createProperties parameter. The client can check for different notations, but that makes everything very difficult.

@dotproto
Copy link
Collaborator

Might it be best to open an issue about establishing a for documenting browser-specific support for the interpretation of specific fields in objects and documenting that pattern in the BCD Data guidelines?

@Rob--W
Copy link
Member

Rob--W commented Jul 29, 2024

This issue was filed because the accessKey was present in the BCD despite there not being a corresponding API entry. As noted in #23521 (comment), the data itself is not incorrect, so this issue could be closed.

Also noted in the same comment is the issue that many BCD entries are not rendered in the Browser Compatibility table on MDN because of missing __compat keys. I'm not sure if this is an issue in the data itself (i.e. al data should list __compat) or in the macro that renders the compat table. To answer this question, we'd need to agree on what the data should look like.

And in order to have a discussion on the desired format of the BCD data, it would be helpful to describe common patterns in the extension APIs, especially when different from what's usual on the web platform. And also the use cases that we're trying to optimize for (just the "Browser Compatibility" table? or something more?).

Note that the original report here questioned the presence of the accessKey because the property is not present in the extension API. An equally valid question could also have been the question of whether menus.create.accessKey is a method, a property, or whether menus is a method and create.accessKey a property of the option named create.

And even if we end up agreeing on some convention for distinguishing properties from method names, there is also the question on how to account for types. For example, Chrome commonly uses single-use named types to describe objects. These names are not visible in the extension API itself (except when it is an enum), but do appear in their documentation. E.g. contextMenus.CreateProperties (https://developer.chrome.com/docs/extensions/reference/api/contextMenus#type-CreateProperties). Would the name then be contextMenus.create.options.title.accessKey? contextMenus.create.options_parameter.title.accessKey? contextMenus.create.accessKey? contextMenus.CreateProperties.accessKey? etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:webext Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions
Projects
None yet
Development

No branches or pull requests

3 participants