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 aria-reflection.html and virtual-accessibility-nodes.html #117

Merged
merged 2 commits into from
May 30, 2018

Conversation

alice
Copy link
Member

@alice alice commented May 29, 2018

aria-reflection.html has a stab at explaining the Element and ShadowRoot interfaces, largely taken from @cookiecrook's existing change to the ARIA spec.

virtual-accessibility-nodes.html is little more than a placeholder at this point.

@alice
Copy link
Member Author

alice commented May 29, 2018

@minorninth @cookiecrook @robdodson @domenic (first name club!) PTAL?

<html xmlns="http://www.w3.org/1999/xhtml" lang="en-US" xml:lang="en-US">

<head>
<title>AOM Phase 1 - ARIA reflection</title>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add links to these new files from at least index.html, if not from all phases?

Or maybe refactor index.html to be a roadmap of the other spec files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

<li>
<em>reflected</em> as DOM attributes on Elements</li>
<li>
<em>non-reflected</em>, via a ShadowRoot.</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about something like "providing default semantics for a custom element via a ShadowRoot (non-reflected)". I just feel like reflected vs non-reflected is more of an aspect, or a consequence, of the two places where you can set attributes - not the primary difference.

Copy link
Member Author

@alice alice May 30, 2018

Choose a reason for hiding this comment

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

I think the "default semantics" thing is a use case, but not necessarily the only one, so I don't want to state it that way.

I removed the emphasis on reflected/non-reflected.

</h2>
<p>If an
<code>Element</code> has an attached
<code>ShadowRoot</code>, and the author has access to the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Saying "if the author has access" is confusing to me. Some author always has access to the ShadowRoot.

Copy link
Member Author

@alice alice May 30, 2018

Choose a reason for hiding this comment

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

Not necessarily - if you attach a ShadowRoot in closed mode and don't keep a reference to it, you will not have access to it.

Also, if someone else has attached a ShadowRoot in closed mode, you will not have access to it.

<code>Element</code>.
</p>
<p>
If a property is set on
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 it'd be good to explicitly clarify what happens with the empty string vs null.

A reflected property can be set to null, which is the same as removing the attribute,
right? But setting it to the empty string overrides.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a mention of non-null value here.

- https://html.spec.whatwg.org/multipage/media.html#texttrack
- https://developer.mozilla.org/en-US/docs/Web/API/AudioNode

Do we need to have three+ AccessibleNode types?
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 we need AccessibleNode and AccessibleRoot, if we want to have the semantics where the AccessibleRoot is just a container for the children and doesn't have its own semantics.

I don't understand why we need both AccessibleNode and VirtualAccessibleNode. I thought we got rid of the use cases for a non-virtual AccessibleNode.

Copy link
Member Author

Choose a reason for hiding this comment

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

AccessibleNode is the common ancestor of VirtualAccessibleNode and AccessibleRoot.

</p>
<pre class="idl">
partial interface Element {
AccessibleNode accessibleNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we were getting rid of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think I pasted it in from somewhere without really looking too closely at it, meaning to fix it up and forgetting to!
Removed until we have an idea what the API will be.

@domenic
Copy link

domenic commented May 30, 2018

Hmm, so seeing the reflection stuff written out here revealed a few complications. Sorry for not flagging them earlier :(. Most are pretty minor though. And it's exciting to see this moving!

Enumerated attributes and missing/invalid value defaults

The first is that the existing ARIA spec isn't clear on how these attributes behave, i.e. it does not specify the missing and invalid value defaults, or what the case-sensitivity of each value is, etc.

For example, https://w3c.github.io/aria/#aria-busy lists two values: "false (default)" and "true". A way to formalize this so that it works with the reflect infrastructure would be to add something like the following prologue before the definition of all attributes:

Various content attributes defined herein have a table listing their valid values. Such attributes are enumerated attributes, whose states and corresponding keywords are given by the first-column of the values table. One row in the value stable will have the suffix " (default)"; this suffix is to be excluded when determining the state/keyword name. The state given by the row marked as " (default)" is the invalid value default and missing value default for such an attribute.

The consequences of this would be that, for example, the browser must interpret aria-busy="TRuE" the same as aria-busy="true". And, aria-busy="", aria-busy="asdf", or no aria-busy="" attribute at all, must all be interpreted the same as aria-busy="false". Hopefully that is already how browsers behave, and it's just the case that nobody ever wrote it down?

Limited to only known values?

For the enumerated attribute cases, where there's a Values table, you can choose between three models: "limited to only known values", nullable, and default. These correspond to paragraphs 4, 5, and 6 of the reflection section. They differ in how they treat invalid, default, and null values.

Consider the element <div aria-invalid="asdf" aria-busy="false">. Note that aria-invalid's default is also "false", and aria-live's default is "off". The models then give:

  • Limited to only known values: div.ariaInvalid === "false", div.ariaBusy === "false", div.ariaLive === "off". Setting to null is the same as setting to "null".
  • Nullable: div.ariaInvalid === null, div.ariaBusy === null, div.ariaLive === null. Setting to null removes the attribute.
  • Default: div.ariaInvalid === "asdf", div.ariaBusy === "false", div.ariaLive === "". Setting to null is the same as setting to "null".

The most popular choice among existing reflected HTML enumerated attributes is "limited to only known values". Nullable seems to only be used for crossOrigin, and default seems to be used for older things like textarea.wrap and marquee.direction.

In this case, you'd want to remove the ?s from the IDL, and add something to the above-mentioned prologue stating that these are all limited to only known values.

Non-enumerated attributes

Not all of the reflected attributes are enumerated attributes.

For example, aria-valuemin's "Value" is described as a "number". Probably the type should be double, not DOMString?.

For the ones listed as "integer", it should probably be unsigned long. Also, for example, aria-posinset says authors must set it to something greater than or equal to 1; perhaps you should use limited to only non-negative numbers greater than zero? (Aside: why did we not call that "positive numbers"??)

For the ones whose value is "string", like aria-roledescription, it appears that we currently don't have the technology in HTML for reflecting as a nullable string, i.e. DOMString?. For example, given const u = document.createElement("ul"), there is no way to use u.type to distinguish between "has a type attribute with the empty string value" vs. "does not have a type attribute". This seems pretty lame, and I'm happy to add something to HTML to allow you to reflect as a DOMString? in the way you're probably expecting. But, it'd be good to confirm that this is worthwhile: are there string-valued ARIA attributes that have different behavior when you do e.g. aria-roledescription="" vs. just omitting aria-roledescription entirely? If empty-string is the same as omission for these cases, maybe we should just go with the flow.

@cookiecrook
Copy link
Collaborator

cookiecrook commented May 30, 2018

@domenic wrote:

The first is that the existing ARIA spec isn't clear on how these attributes behave…

Someone mentioned in response to your previous comments to the ARIA PR that these could/would be addressed as separate issues in the ARIA spec. However, since there are no active ARIA editors on this thread, could you post your ARIA Spec suggestions as a new issue on the ARIA repo? https://github.com/w3c/aria/issues/

@cookiecrook
Copy link
Collaborator

@domenic wrote:

For example, aria-valuemin's "Value" is described as a "number". Probably the type should be double, not DOMString?.

Due to floating point math, reflecting Double into String content attributes is lossy on every conversion. Do you have another example of reciprocal string-to-double reflection was implemented successfully?

@domenic
Copy link

domenic commented May 30, 2018

I'm hopeful someone else can help ferry my review into the appropriate channels so that it gets taken into account, and unblocks the work in this pull request (which doesn't really make sense without addressing it).

The attributes which are reflected as doubles in HTML include:

  • HTMLProgressElement's value and max
  • HTMLMeterElement's value, min, max, low, high, optimum

Other successfully-implemented string <-> double conversions on the web platform include Number.prototype.toString, Number.parseFloat, and the Number constructor.

Remember that these reflected IDL attributes are meant to be authoring conveniences; having appropriate types, without requiring the developer to do conversions themselves, is one of the key advantages over just using getAttribute().

</p>
<p>
If a
<code>role</code> or ARIA attribute property is set on
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am understanding you correctly, "role or ARIA attribute property" would be better expressed as "role IDL attribute or another ARIA IDL attribute."

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to better clarify.

<em>either</em> the
<code>Element</code>
<em>or</em> its associated
<code>ShadowRoot</code>, that property should be
Copy link
Collaborator

Choose a reason for hiding this comment

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

"that property" -> "the IDL attribute"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

@cookiecrook
Copy link
Collaborator

@domenic wrote:

I'm hopeful someone else can help ferry my review into the appropriate channels so that it gets taken into account,

It sounds like you asking one of us to repost your comments above into a new ARIA issue. Am I misunderstanding? Is there some reason you don't want to do it?

@domenic
Copy link

domenic commented May 30, 2018

Yes, it would be great if you all could help with such steps. I was just asked to provide a review here, and have limited time to engage. It's unfortunate and discouraging to provide a review, and then be asked to do more work in response.

@cookiecrook
Copy link
Collaborator

@domenic wrote:

It's unfortunate and discouraging to provide a review, and then be asked to do more work in response.

I did not intend to be discouraging. I needed to clarify because your review seemed to be focused on the part prefixed by: "This section is part of the ARIA 1.2 Specification, and is included here for informative purposes only."

@minorninth
Copy link
Collaborator

The consequences of this would be that, for example, the browser must interpret aria-busy="TRuE" the same as aria-busy="true". And, aria-busy="", aria-busy="asdf", or no aria-busy="" attribute at all, must all be interpreted the same as aria-busy="false". Hopefully that is already how browsers behave, and it's just the case that nobody ever wrote it down?

Unfortunately I suspect there are probably bugs in browser implementations here. I just glanced at the Chrome and WebKit code and see places in both where we test whether an ARIA attribute value is equal to "false" (ignoring case).

Limited to only known values?

My only concern here is that this now becomes a kind of feature detection, and I'm not sure that was something we wanted here.

For the ones whose value is "string", like aria-roledescription, it appears that we currently don't have the technology in HTML for reflecting as a nullable string, i.e. DOMString?. For example, given const u = document.createElement("ul"), there is no way to use u.type to distinguish between "has a type attribute with the empty string value" vs. "does not have a type attribute". This seems pretty lame, and I'm happy to add something to HTML to allow you to reflect as a DOMString? in the way you're probably expecting. But, it'd be good to confirm that this is worthwhile: are there string-valued ARIA attributes that have different behavior when you do e.g. aria-roledescription="" vs. just omitting aria-roledescription entirely? If empty-string is the same as omission for these cases, maybe we should just go with the flow.

From: https://w3c.github.io/aria/#state_property_processing

"""Sometimes states and properties are present in the DOM but have a zero-length string ("") as their value. This is equivalent to their absence. User agents SHOULD treat state and property attributes with a value of "" the same as they treat an absent attribute."""

It sounds like we don't need these to be nullable DOMStrings, then, for that reason.

@alice
Copy link
Member Author

alice commented May 30, 2018

Made changes to the specs here in response to comments, PTAL.

It seems like the discussion of @domenic's specific points needs to happen elsewhere, and I'm too tired right now to make that happen - @cookiecrook or @minorninth, if you feel like it perhaps you could start a thread in the right place tomorrow morning your time, or otherwise I'll do it tomorrow morning my time. Thanks!

@minorninth
Copy link
Collaborator

I filed w3c/aria#777 with @domenic's comments

@alice
Copy link
Member Author

alice commented May 30, 2018

Unless there are any objections, I'll land this patch for now, and create a separate patch to try out solutions to the issue @domenic raised, which we could then potentially also use to create a patch to the ARIA spec.

Copy link
Collaborator

@minorninth minorninth left a comment

Choose a reason for hiding this comment

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

Let's land this as-is to be in sync with the ARIA spec, then update it to address Domenic's changes - and then apply that same fix to the ARIA spec.

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.

4 participants