-
Notifications
You must be signed in to change notification settings - Fork 59
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
Conversation
@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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
spec/aria-reflection.html
Outdated
<li> | ||
<em>reflected</em> as DOM attributes on Elements</li> | ||
<li> | ||
<em>non-reflected</em>, via a ShadowRoot.</li> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
spec/aria-reflection.html
Outdated
<code>Element</code>. | ||
</p> | ||
<p> | ||
If a property is set on |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 defaultsThe 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:
The consequences of this would be that, for example, the browser must interpret 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 Consider the element
The most popular choice among existing reflected HTML enumerated attributes is "limited to only known values". Nullable seems to only be used for In this case, you'd want to remove the Non-enumerated attributesNot all of the reflected attributes are enumerated attributes. For example, For the ones listed as "integer", it should probably be For the ones whose value is "string", like |
@domenic wrote:
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/ |
@domenic wrote:
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? |
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:
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 |
spec/aria-reflection.html
Outdated
</p> | ||
<p> | ||
If a | ||
<code>role</code> or ARIA attribute property is set on |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to better clarify.
spec/aria-reflection.html
Outdated
<em>either</em> the | ||
<code>Element</code> | ||
<em>or</em> its associated | ||
<code>ShadowRoot</code>, that property should be |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks.
@domenic wrote:
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? |
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. |
@domenic wrote:
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." |
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).
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.
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. |
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! |
I filed w3c/aria#777 with @domenic's comments |
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. |
There was a problem hiding this 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.
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.