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

resolves #1058, enumerated values for DOMString reflection #1261

Merged
merged 14 commits into from
Jun 12, 2020
Merged

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook cookiecrook commented May 7, 2020

This prose addition should reconcile our use of DOMString reflection for enumerated values for #1058.


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on May 22, 2020, 9:31 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL


😭  Sorry, there was an error generating the HTML. Please report this issue!
�[36mSpecification: https://rawcdn.githack.com/w3c/aria/62603125ff5ada131c69644faa40cd8d26632ec2/index.html?isPreview=true&publishDate=2020-05-22�[39m
�[36mReSpec version: 25.6.0�[39m
�[36mFile a bug: https://github.com/w3c/respec/�[39m
�[36mError: Error: Evaluation failed: Timeout: document.respecIsReady didn't resolve in 5093ms.�[39m
�[36m    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/ExecutionContext.js:122:13)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/ExecutionContext.js:48:12)�[39m
�[36m    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:128:12)�[39m
�[36m    at async fetchAndWrite (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:95:18)�[39m
�[36m    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:20)�[39m
�[36m    at async generate (/u/spec-generator/server.js:91:29)�[39m
�[36m  -- ASYNC --�[39m
�[36m    at ExecutionContext.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:111:15)�[39m
�[36m    at DOMWorld.evaluate (/u/spec-generator/node_modules/puppeteer/lib/DOMWorld.js:112:20)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m  -- ASYNC --�[39m
�[36m    at Frame.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:111:15)�[39m
�[36m    at Page.evaluate (/u/spec-generator/node_modules/puppeteer/lib/Page.js:860:43)�[39m
�[36m    at Page.<anonymous> (/u/spec-generator/node_modules/puppeteer/lib/helper.js:112:23)�[39m
�[36m    at generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:128:23)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m    at async fetchAndWrite (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:95:18)�[39m
�[36m    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:20)�[39m
�[36m    at async generate (/u/spec-generator/server.js:91:29)�[39m
�[36m�[39m

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@cookiecrook cookiecrook added this to the ARIA 1.2 milestone May 7, 2020
@cookiecrook
Copy link
Contributor Author

@alice @annevk @domenic

@cookiecrook cookiecrook self-assigned this May 7, 2020
@carmacleod
Copy link
Contributor

You may want to link to https://w3c.github.io/aria/#propcharacteristic_value somewhere in that prose... perhaps link "allowed values" or just the word "values"?

@domenic
Copy link
Contributor

domenic commented May 7, 2020

This definitely helps, thanks!

What about when the table does not notate one of the values as "(default)"?

It would also be good to link:

@cookiecrook
Copy link
Contributor Author

What about when the table does not notate one of the values as "(default)"?

Good spot. There is only one: aria-relevant Filed as #1265.

@cookiecrook
Copy link
Contributor Author

It occurred to be that we may need to mention the difference between enumerated attributes that take a single value (token, tristate, etc) versus those that take multiple values (token list), including aria-relevant whose default is the two values "additions text"…

@domenic
Copy link
Contributor

domenic commented May 7, 2020

One additional thing, is that you should add a similar blanket statement making it clear what the states and their corresponding keywords are. Something like "Each value in the table is a keyword for the attribute, mapping to a state of the same name".

Stepping back a bit further, it would probably make sense to move this out of the "ARIA Attribute Reflection" section, since it has a broader impact on the processing model of these attributes. E.g. it implies that:

  • User agents must treat their values as ASCII case-insensitive
  • User agents must treat both missing and invalid values as the default (whereas before it wasn't clear what to do with invalid values). Note that unless you define otherwise somewhere, "" triggers the same default (since it's not in the table, and is thus invalid).

@domenic
Copy link
Contributor

domenic commented May 7, 2020

those that take multiple values (token list), including aria-relevant whose default is the two values "additions text"…

Hmm, yeah, this doesn't use the enumerated attribute framework in HTML then, and will need a different processing model and reflection support. A precedent in HTML is rel, which states (later):

To determine which link types apply to a link, a, area, or form element, the element's rel attribute must be split on ASCII whitespace. The resulting tokens are the keywords for the link types that apply to that element.

and

Keywords are always ASCII case-insensitive, and must be compared as such.

It sounds like you'd want to layer some additional processing on top of that, e.g. if the resulting set is empty, then set it to « "additions", "text" ».

How many such cases are there? Maybe we should make this the HTML editor's job, to create a common infrastructure (like enumerated attributes) which could be shared by the relevant ARIA attributes and <link>/<a>'s rel="" and probably some others.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented May 7, 2020

How many such cases (of token lists) are there? Maybe we should make this the HTML editor's job, to create a common infrastructure (like enumerated attributes) which could be shared by the relevant ARIA attributes and <link>/<a>'s rel="" and probably some others.

In ARIA 1.2 there are two attrs, but only one relevant in the IDL context:

  • aria-dropeffect (deprecated in ARIA 1.1, so not included in IDL reflection)
  • aria-relevant

@cookiecrook
Copy link
Contributor Author

it would probably make sense to move this out of the "ARIA Attribute Reflection" section, since it has a broader impact on the processing model of these attributes.

Yes. I think it makes sense to include this in the values section, rather than just linking to it from the IDL section. I'll think on this a bit more before updating the PR. Thanks for your help.

@domenic
Copy link
Contributor

domenic commented May 7, 2020

Hmm OK. That's a very interesting attribute.

I see two options:

IDL attribute directly manipulates and reflects the content attribute

This version would involve changing ariaRelevant to [SameObject, PutForwards=value] readonly attribute DOMTokenList ariaRelevant;, which will trigger the very last paragraph of https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes and take care of all the IDL stuff for you. It will allow authors to manipulate the result using the convenient features of DOMTokenList and the [PutForwards] annotation, e.g.:

el.ariaRelevant = "aDDitions removals"; // shortcut for el.ariaRelevant.value = "aDDitions removals"
el.ariaRelevant.remove("removals");
el.ariaRelevant.add("all");
console.assert(el.ariaRelevant.value === "aDDitions all");

If you do that, then the resulting .ariaRelevant property will not contain "additions" or "text" by default; it will reflect the value of the attribute exactly. (So e.g. el.ariaRelevant.contains("additions") will be false, if the attribute is not present.)

This follows a number of attributes in HTML, like link.relList, link.sizes, iframe.sandbox, etc.

IDL attribute is customized to reflect the data model better

This version would not use HTML's attribute reflection framework at all, and would not use DOMTokenList. Instead it would work to produce something that reflects the underlying data model, by defining custom getters and setters.

The setter would probably just set the attribute to the given value.

The getter would figure out the normalized list of tokens, and return a canonicalized form. For example, it could normalize "all" to "additions removals text", and would normalize invalid values by removing them. It could even sort them. So e.g. I'm imagining

el.ariaRelevant = "all      asdf additions !";
console.assert(el.ariaRelevant === "additions removals text");

el.ariaRelevant = "additions text REMOvALS   ";
console.assert(el.ariaRelevant === "addition removals text");

el.ariaRelevant = "";
console.assert(el.ariaRelevant === "additions text";

This kind of "custom reflection" has some precedent in HTML in attributes like el.dir, el.spellcheck, and and el.autocapitalize.

@cookiecrook
Copy link
Contributor Author

Okay. I think I'm going to file aria-relevant reflection as a separate issue and remove it from the 1.2 milestone in this PR, along with moving the line of prose to the values section.

@carmacleod
Copy link
Contributor

Just thought I'd mention that there's one more token list attribute that did not make it into ARIA 1.2, that may (or may not) go into ARIA 1.3.

Here's the preview for aria-textseparation from PR #996. The default value is a single token, style.

It's a particularly weird attribute because it can only take 1 or 2 tokens. :)

@annevk
Copy link
Member

annevk commented May 8, 2020

Looking at this PR I'm not sure how it solves the nullable problem? Or does it help with #1058, but not fully resolve it?

Also, role also seems to be defined as a list value or is that not considered an ARIA attribute? Either way it seems to have the same problem.

@carmacleod
Copy link
Contributor

carmacleod commented May 8, 2020

role is an ARIA attribute and does also need to be taken into account for attribute reflection. Thanks for the reminder. I have no idea about the history of why it's living separately in its own spec.

Edit: Oh right, it's because that makes it available to other markup languages/technologies.
https://www.w3.org/TR/role-attribute/

So maybe it's not technically an ARIA attribute, but the ARIA spec is specifying its reflection, so definitely needs to be part of the "token list" discussion. :)
https://w3c.github.io/aria/#AccessibilityRole

index.html Outdated Show resolved Hide resolved
@carmacleod carmacleod linked an issue May 14, 2020 that may be closed by this pull request
@cookiecrook
Copy link
Contributor Author

@annevk wrote:

Looking at this PR I'm not sure how it solves the nullable problem? Or does it help with #1058, but not fully resolve it?

@domenic's comments starting here in #1058 led me to believe this was sufficient given the current limitations of what is definable in WebIDL syntax today. If it's insufficient, I may need your help determining why or suggesting a resolution.

Also, role also seems to be defined as a list value or is that not considered an ARIA attribute? Either way it seems to have the same problem.

Issue #1058 refers directly to the AriaAttributes interface, which does not include role. That one is defined in the AccessibilityRole interface. We could address issues with that one too, but please file a separate issue if possible. Thank you.

@annevk
Copy link
Member

annevk commented May 15, 2020

@cookiecrook I guess it depends on whether all attributes with a nullable string are enumerated. I thought that some were not, such as ariaValueNow. (Filed #1272 for role.)

…g props are removed in another branch, so not modified here.
@cookiecrook
Copy link
Contributor Author

@annevk wrote:

I guess it depends on whether all attributes with a nullable string are enumerated. I thought that some were not, such as ariaValueNow.

Ah. Thanks. Yes, now that we have the prose, I think the original intention for for the IDL nullable ? flag is superseded entirely now. Added a commit to remove those from the DOMString props. Note: I left the others for Element and FrozenArray<Element> because those are pulled out in another branch.

@cookiecrook cookiecrook requested review from carmacleod and asurkov May 15, 2020 20:28
@cookiecrook
Copy link
Contributor Author

Recycling the review requests because this diff has grown substantially.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Looks good!
Just a few typos (I'll just push them in).
+1

(just curious... what is the missing value default for role? Is it "undefined"? Any chance we can talk the browsers into returning the implicit role for element.role if no explicit role was assigned? That would be so useful! 😄 )

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@cookiecrook
Copy link
Contributor Author

cookiecrook commented May 18, 2020

@carmacleod wrote:

Just a few typos (I'll just push them in).

Thanks !

(just curious... what is the missing value default for role? Is it "undefined"?

Empty string ""

Any chance we can talk the browsers into returning the implicit role for element.role if no explicit role was assigned? That would be so useful! 😄 )

That's pretty difficult to do without spinning up the accessibility code in most browsers now. TagName isn't the only factor that affects role. For example, a <td> outside of a table, doesn't get exposed as a cell or gridcell. Neither does a <td> in a layout table. From the implementation side, I don't think that change would be achievable in a minor point release.

The computedRole property in WebDriver #1439 may be a more feasible way to approach that use case in the meantime.

index.html Show resolved Hide resolved
Copy link
Contributor

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nit/addition. Thanks so much for working through this; it makes the processing model a lot clearer, both for reflection and ARIA behavior in general.

Probably the next step would be to tackle #1110, since that's simpler than element reflection and ariaRelevant.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
@jnurthen jnurthen merged commit 646f093 into master Jun 12, 2020
jnurthen pushed a commit that referenced this pull request Jul 10, 2020
* Enumerated values section now replaces superseded state_prop_values section, and includes examples per comment by @asurkov
* remove the nullable flag from most IDL properties; Note: the remaining props are removed in another branch, so not modified here.
Co-authored-by: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
@jnurthen jnurthen deleted the issue-1058 branch July 23, 2020 22:28
domenic added a commit to web-platform-tests/wpt that referenced this pull request Jan 5, 2024
This attribute was removed from the ARIA spec in 2020: see w3c/aria#1261 and w3c/aria#1267.
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.

AriaAttributes cannot be nullable
8 participants