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

Updating ARIA 1.2 due to IDL implementations (exit and re-enter CR?) #1598

Closed
joanmarie opened this issue Aug 13, 2021 · 41 comments · Fixed by #1611 or #1633
Closed

Updating ARIA 1.2 due to IDL implementations (exit and re-enter CR?) #1598

joanmarie opened this issue Aug 13, 2021 · 41 comments · Fixed by #1611 or #1633
Assignees
Labels
Milestone

Comments

@joanmarie
Copy link
Contributor

As described in #1585 (comment) as well as in ARIA WG meetings 1 and 2, what implementors have done for ARIA reflection does not appear to match what is in the spec.

In particular, default/implicit values for missing attributes are not being reflected, and no validation is taking place. When an attribute is not present in the DOM, the reflected value is null. Thus as it currently stands, I believe we have 0 implementations for the IDL interface as spec'ed in ARIA 1.2.

What is currently implemented seems consistent with the properties being a nullable DOMString. Therefore, unless there is a strong belief that what is in the spec now (attributes are a non-nullable DOMString) is correct and that the implementations are wrong, the thing to do might be to update ARIA 1.2 to reflect (pun intended) reality.

@cyns cyns self-assigned this Aug 19, 2021
@jnurthen jnurthen self-assigned this Aug 26, 2021
@jnurthen jnurthen added this to the ARIA 1.2 milestone Sep 1, 2021
@cookiecrook
Copy link
Contributor

@jongund
Copy link
Contributor

jongund commented Sep 7, 2021

Does the group need a test page need to be developed for IDL testing?

@cyns
Copy link
Contributor

cyns commented Sep 9, 2021

Here's the PR #1611

@saschanaz
Copy link
Member

Therefore, unless there is a strong belief that what is in the spec now (attributes are a non-nullable DOMString) is correct and that the implementations are wrong, the thing to do might be to update ARIA 1.2 to reflect (pun intended) reality.

Probably worth to ping @annevk who raised #1058.

@annevk
Copy link
Member

annevk commented Sep 20, 2021

Was an effort made to get implementations to change here? Are there tests? This change was made only a year ago too.

Furthermore, it's not clear to me that the current text works as reflecting only supports nullable strings if the attribute is an enumerated attribute and meets the requirements thereof. See also the discussion in the issue @saschanaz referenced.

@cookiecrook
Copy link
Contributor

cookiecrook commented Sep 23, 2021

Google WG members looked into changing the implementations, but it's a non-trivial amount of work with no clear user benefit.

As such, we don't believe the engines need to change, so we'd like to take the WHATWG-recommended approach that you and others helped develop: update the spec to match what is already working in the implementations.

@annevk
Copy link
Member

annevk commented Sep 27, 2021

But what exactly is that? The PR doesn't appear to do that as far as I can tell. It relies on reflect, but reflect doesn't handle this type for numerous ARIA attributes.

@cookiecrook
Copy link
Contributor

cookiecrook commented Sep 27, 2021

Update: linked #1611 in the sidebar, which is the correct PR.

…but the only substantive IDL change in this PR is to add the nullable ? to the existing DOMString entries like ariaLabel which matches current browser behavior.

The diff won't change any of the other still-commented FrozenArray<Element> attributes that it seems like you're concerned about, like ariaLabelledByElements. The diff also adds some examples. HTH. (@cyns please verify I recall correctly)

@annevk
Copy link
Member

annevk commented Sep 28, 2021

It doesn't really, ariaLabel isn't an enumerated attribute as far as I know, so DOMString? doesn't work if you say "reflect".

@annevk
Copy link
Member

annevk commented Sep 28, 2021

cc @domenic

@cookiecrook
Copy link
Contributor

@annevk wrote:

as far as I know, so DOMString? doesn't work if you say "reflect".

When we first proposed adding IDL to ARIA, the standard version of IDL didn't include reflect at all. Since it's been a moving target, we appreciate your help in this area.

For the following HTML element:

<div aria-label="foo"><!-- No role. This is just an example element for the output below. --></div>

This is the console output in WebKit, and IIRC, it matches other engines.

> $0.ariaLabel
< "foo"
> $0.getAttribute("aria-label")
< "foo"
> $0.setAttribute("aria-label", "")
< undefined
> $0.getAttribute("aria-label")
< ""
> $0.ariaLabel
< ""
> $0.removeAttribute("aria-label")
< undefined
> $0.ariaLabel
< null

IIRC @joanmarie and @cyns were attempting to match the existing implementations, which treat these attributes like nullable, reflected DOMStrings, as demonstrated above. If there is a better syntax to express this in the current version of IDL, please advise. Thanks.

@saschanaz
Copy link
Member

We looked into changing the implementations, but it's a non-trivial amount of work with no clear user benefit.

I wonder what caused such amount of work, were there web compat issues?

@cookiecrook
Copy link
Contributor

cookiecrook commented Sep 28, 2021

@cyns looked into this more for Chrome, but IIRC, the issue was that in every web engine, the backing accessibility implementation already handled validation, so duplicating that behavior in reflected enumerated DOM attributes was a reasonable amount of effort with no end-user impact. These sections were added to document how the implementations work today.

6.3.1 IDL Reflection of ARIA attributes

All ARIA attributes reflect in IDL as DOMString attributes. This includes the boolean-like enumerated true/false type, and all other ARIA enumerated attributes.

Default values from the ARIA values tables MUST NOT reflect to IDL as the missing value default or the invalid value default for the attribute. On getting, a missing ARIA attribute will return null. ARIA attributes are not validated on get. If an ARIA value is invalid, on getting, it will return its set value as a literal string, and will not return an invalid value default.

6.3.2 Operating System Accessibility API mapping of ARIA enumerated attributes

Unlike IDL reflection, operating system accessibility API mappings of ARIA enumerated attributes do have defaults. The default values from the ARIA values tables are exposed to the operating system accessibility API as described in section 5.2.3 Supported States and Properties of this specification, and in the CORE-AAM specification.

@saschanaz
Copy link
Member

saschanaz commented Sep 28, 2021

It sounds like Chrome couldn't do something like this:

DOMString AriaMixin::AriaLabel() {
  Nullable<DOMString> nullable = WhateverTheBackingAccessibilityImplDoes();
  if (nullable.is_null()) {
    return "";
  }
  return nullable.inner_value();
}

... which doesn't make sense to me, or I probably still lack the background here.

@annevk
Copy link
Member

annevk commented Sep 29, 2021

"Reflect" has been a concept defined by the HTML standard for well over a decade so I think there might be some misunderstanding here. At least per my understanding this feature is a lot newer than that.

@cookiecrook
Copy link
Contributor

Clarifying that I was referring to the ~[reflect] syntax WebKit and Blink use(d?).

@annevk
Copy link
Member

annevk commented Sep 30, 2021

At the moment that's just an implementation detail. There's been some talk about standardizing it, but not a lot of progress. (It would still build upon "reflect" though so the issues would remain the same.)

@domenic
Copy link
Contributor

domenic commented Sep 30, 2021

To be a bit clear, since nobody has stated it this way: right now your spec has a (conceptual) null pointer exception, since you apply the concept "reflect", but in spec-land that concept is totally inapplicable to DOMString? attributes. So anyone trying to implement based on the spec you have written (plus the HTML spec) is unable to do so.

@cookiecrook
Copy link
Contributor

cookiecrook commented Sep 30, 2021

After some additional digging and discussion with colleagues, I think there is a path forward.

ARIA 1.2

In order to get ARIA 1.2 through its long-overdue CR, remove the reference to HTML reflection and replace it with something like "Return the value of the attribute if present and null otherwise." This way the 1.2 spec still matches the shipping implementations. It can include in a Note the similarity/difference between this and HTML's concept of reflection.

ARIA 1.3 or later

Consider changing the implementations to non-nullable reflected DOMStrings... (@cyns, @joanmarie, and @aleventhal to comment, since IIRC, it was one of them who mentioned an implementation, interop, or scheduling concern.)

On the surface, it seems like this may be as simple as removing the ? from the WebKit IDL, Chromium IDL, and Gecko IDL, but I'll defer to the others on the potential concerns.

@jnurthen
Copy link
Member

jnurthen commented Oct 5, 2021

In HTML crossOrigin on img is defined as
[CEReactions] attribute DOMString? crossOrigin;

with the following:
The crossOrigin IDL attribute must reflect the crossorigin content attribute, limited to only known values.

I'm not sure what is different for our usage of reflect than this.

@annevk
Copy link
Member

annevk commented Oct 5, 2021

@jnurthen see #1598 (comment). crossorigin is an enumerated attribute, aria-label is not.

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 7, 2021

@annevk What about alt in HTML? There's an important Accessibility API distinction between an empty string alt="" and a missing attribute. <img alt=""> means "ignore this image" and <img> means "this is an inaccessible image; attempt to repair through machine vision or another means."

@saschanaz
Copy link
Member

saschanaz commented Oct 7, 2021

new Image().alt returns "" (and I think we can't change it as the behavior has been there for decades).

@jnurthen
Copy link
Member

jnurthen commented Oct 7, 2021

@saschanaz I agree this would be hard to change for alt now. However, we don't want to introduce similar broken behaviour for aria attributes.

@saschanaz
Copy link
Member

Yeah and I don't think people are asking for the non-nullable behavior, it's just that the spec is broken and you need to define an actual algorithm for nullable non-enumerated string attributes.

@saschanaz
Copy link
Member

(And thanks, I think #1598 (comment) is the background I wanted as I wasn't sure nullable string is the way people actually want to go or just a webcompat quirk.)

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 12, 2021

@saschanaz wrote:

Yeah and I don't think people are asking for the non-nullable behavior, it's just that the spec is broken and you need to define an actual algorithm for nullable non-enumerated string attributes.

Modifying my comment above to clarify a potential permanent solution rather than just a stop gap. I wrote:

…remove the reference to HTML reflection [for the non-enumerated attributes] and replace it with something like "Return the value of the attribute if present, or null otherwise." This way the 1.2 spec still matches the shipping implementations. It can include in a Note the similarity/difference between this and HTML's concept of reflection.

Note: The enumerated attributes in ARIA may be 1:1 with the §6.7 list whose short description start with the word "Indicates…" (minus aria-keyshortcuts b/c of #1625)

I think these are the new choices based on @saschanaz's comment:

  1. Spec could state that the nullable enumerated attributes reflect (HTML definition), and the other nullable attributes, ~"Return the value of the attribute if present, or null otherwise." Downside to this path is that none of the implementations match the enumerated attributes IDL now, so those would need to be removed from the PR, or ARIA 1.2 CR would need to wait on two implementations for the enumerated validations. Note: validation for default missing value and default invalid value is currently handled by the backing accessibility implementation in the engines, and not by the IDL implementation.

  2. Spec could remove the reflect verbiage entirely, and define this algorithm for all nullable DOMStrings, ~"Return the value of the attribute if present, or null otherwise." Downside to this path is that we risk breaking webcompat for any we expect might be nullable enumerated attributes in the future. However, upside is that this matches the implementations today, so it's a very low risk that this path would cause additional harm now or later.

@annevk
Copy link
Member

annevk commented Oct 13, 2021

I don't think solution 2 is adequate as that would only address getter behavior and that exact language doesn't establish a relationship between the name of the getter and the name of the attribute.

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 14, 2021

@annevk wrote:

that would only address getter behavior and that exact language doesn't establish a relationship between the name of the getter and the name of the attribute.

Is there a modification to ARIA Section §10.2 that would address your concern with the relationship between the ~reflected DOM property and respective content attributes?

@jnurthen
Copy link
Member

@annevk @saschanaz @domenic Will you be able to attend our meeting at TPAC so we can progress this? https://www.w3.org/events/meetings/e8be2735-d90b-4619-a25c-d25c0abdd965

@domenic
Copy link
Contributor

domenic commented Oct 21, 2021

I can probably attend, but it's unclear what a meeting will help with here. What is necessary is for someone to write a proposal with spec text and get it reviewed.

@annevk
Copy link
Member

annevk commented Oct 22, 2021

I have a conflict for the first hour and it's rather late after that, but I can probably make that work. Agreed though that what's really needed here is for someone to write a proposal.

@cookiecrook
Copy link
Contributor

TPAC ARIA Meeting has started.

@jnurthen
Copy link
Member

@annevk @domenic is this the type of text you are looking for?
“If a reflecting IDL attribute is a nullable DOMString attribute, then, on getting, if the corresponding content attribute is in its missing value default state then the IDL attribute must return null, otherwise, the IDL attribute must get the value in a transparent, case-preserving manner. On setting, if the new value is null, the content attribute must be removed, and otherwise, the content attribute must be set to the specified new value in a transparent, case-preserving manner.”

@cookiecrook
Copy link
Contributor

And @annevk FWIW we believe your relationship comment is already addressed.

@domenic
Copy link
Contributor

domenic commented Oct 28, 2021

That's along the right lines. An issue is that "missing value default state" only applies to enumerated attributes, and nullable DOMString reflection is already defined for enumerated attributes in https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes

As @annevk pointed out in #1598 (comment) , the attributes we're dealing with here are not enumerated attributes. So that condition doesn't hold, and thus the spec text you propose doesn't really work as written.

When do you want null to be returned?

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 28, 2021

@domenic wrote:

the attributes we're dealing with here are not enumerated attributes

I previously proposed "Return the value of the attribute if present and null otherwise" but I don't believe it's been answered why that simpler phrasing wouldn't work for these non-enumerated reflected strings.

@annevk implied there's no relationship established between the name of the getter and the name of the attribute, but I believe that's already addressed by ARIA Section §10.2

If one or both of you could join the call (going on now) it may be easier to work out the disconnect.

@domenic
Copy link
Contributor

domenic commented Oct 28, 2021

"Return the value of the attribute if present and null otherwise" would work. @annevk's response was that you failed to define the setter behavior, but if you combine that with the rest of #1598 (comment) it would be fine.

Again, I don't think a call is necessary here. Proposing spec text and reviewing it, as we're doing asynchronously, works best for me. It would be even better if it was done in pull request form so we could use the normal code review tools.

@jnurthen
Copy link
Member

revising the proposed language:
"If a reflecting IDL attribute is a nullable DOMString attribute, then, on getting, if the corresponding content attribute is not present then the IDL attribute must return null, otherwise, the IDL attribute must get the value in a transparent, case-preserving manner. On setting, if the new value is null, the content attribute must be removed, and otherwise, the content attribute must be set to the specified new value in a transparent, case-preserving manner."

@domenic
Copy link
Contributor

domenic commented Oct 28, 2021

LGTM with a slight modification: "If a reflecting IDL attribute is a nullable DOMString attribute" should be "If a reflecting IDL attribute is a nullable DOMString attribute whose content attribute is not an enumerated attribute", so as to not conflict with the existing definition in https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 28, 2021

if you combine that with the rest of [jnurthen's draft] it would be fine.

Thanks for clarifying. I think this was the bit we were blocked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants