w3c / aria

Accessible Rich Internet Applications (WAI-ARIA)
https://w3c.github.io/aria/
Other
635 stars 120 forks source link

ARIAMixin has many integer attributes with string types and uses DOMString? incorrectly #1110

Open annevk opened 4 years ago

annevk commented 4 years ago

It would be nicer if these were reflected as numbers. E.g., aria-level.

cc @alice @domenic

domenic commented 4 years ago

If we do this (we probably should), then a choice needs to be made for each one, between the different types of reflection:

Probably don't consider the long ones.

alice commented 4 years ago

I have a vague memory of discussing this and concluding that string reflection was the way to go, but I don't remember any details. @cookiecrook may remember?

alice commented 4 years ago

Listing out non-string types per the ARIA value types (not including token types or true/false/undefined):

The true/false ones are interesting, since they're almost but not quite boolean.

annevk commented 4 years ago

From Domenic It sounded like there was a preference for true/false values to be enumerated values so they can be extended in the future. I kinda wish they would not have used true and false as values then, but so be it. So those probably have to remain strings, though limited to known values would be nice.

HTML's colSpan and rowSpan are unsigned long and colSpan there also has to be at least 1, whereas rowSpan is at least 0. I don't really know the background.

(It seems HTML uses long (for allowing -1) and unsigned long (when non-negative) pretty consistently.)

alice commented 4 years ago

Chatting with @cookiecrook about this just now - he is going to do some research and try to remember why we went with string reflection originally.

cookiecrook commented 4 years ago

Initial experimentation was in AOM threads. https://github.com/WICG/aom/pull/120

More meaty, long history thread inside ARIA tracker starts here: https://github.com/w3c/aria/issues/691#issuecomment-367878962

Most relevant comment is this one: https://github.com/w3c/aria/issues/691#issuecomment-385872627

Excerpt:

Changed all long values to DOMString, since long cannot be nullable. Changed all double values to DOMString, since double-to-string conversion is lossy and imprecise.

And we pulled a few more IDL props prior to shipping ARIA 1.2 to make way for element reflection. https://github.com/w3c/aria/issues/834

annevk commented 4 years ago

Why is long not nullable? Pretty sure it is.

And reflection of double to string is defined in HTML and used by a large number of features. The rationale given doesn't seem like a good reason to establish a new API pattern.

domenic commented 4 years ago

There is no reflection for nullable long defined in HTML. Instead, when the attribute is not present, a default value is used. (Usually 0 or -1.)

I agree that ARIA sticking to that would be better than inventing a new convention.

annevk commented 4 years ago

As I mentioned on IRC, there's no nullable string for reflection either (other than for enumerated attributes), so if that's the argument I'm confused.

cookiecrook commented 4 years ago

@domenic wrote:

I agree that ARIA sticking to that would be better

Sticking to what would be better? String reflection as-is?

domenic commented 4 years ago

No, sorry, sticking to using default values (like 0 or -1), like all other reflected numeric HTML attributes do.

cookiecrook commented 4 years ago

Ok then, let's work out the edge cases.

  1. As Alice mentioned, some ARIA attrs are true booleans (@aria-modal), others are true/false/mixed, and still others are true/false/undefined, where the unset value is different that either true or false. One example is @arial-pressed. When used on role="button" the existence of the attribute means it's a toggle button in a pressed (true) or unpressed (false) state. The undefined default (absence of the the attr) means it's a standard non-toggle button. (This might be superseded by the next item.)

  2. Several of the aria attributes are enumerable token values: such as @aria-invalid. Its values are currently true, false, spelling, and grammar though this list is intended to be expandable in future versions of ARIA. If I understand correctly, this would require defining new IDL value types as I attempted in the original issue here: https://github.com/w3c/aria/issues/691#issuecomment-367891305

  3. Many of the number values (@aria-valuenow, etc.) are not integers, but floating point doubles. Double-to-String reflection is lossy due to floating point math. Is this acceptable? Perhaps, but it will surprise some web authors.

  4. Most ARIA attributes can be adopted by the host language as providing equivalent semantics to the host language attribute. @required and @aria-required is an example. A conflicting value between the two results in UA resolution, based on some rules in the host language and ARIA. IIRC, a reflecting these as something other than String may result in both reflecting the same value... Is this acceptable, and if so, is there existing precedent in IDL?

domenic commented 4 years ago
  1. true/false/mixed and true/false/undefined sound like enumerated attributes to me, so yeah, subsumed into (2).

  2. We have a model for reflecting enumerated attributes. This doesn't require a new IDL value type; DOMString is good for that.

  3. Double-to-string reflection is well-established on the web platform with a number of existing examples in HTML. This is the main issue at hand, I think.

  4. I'm not sure I understand this, but I'll try to answer. The reflection from typed JS properties (aka IDL attributes) to HTML attribute strings is a bidirectional, mechanical process, that is performed independently for any two attributes. The reflection doesn't change behavior because two attributes are related. And the types of the JS properties (IDL attributes) being strings versus numbers or any such doesn't change this.

cookiecrook commented 4 years ago

The reflection doesn't change behavior because two attributes are related. And the types of the JS properties (IDL attributes) being strings versus numbers or any such doesn't change this.

Okay. I recall there were some cases where the existence of the ARIA attribute would "win" over the native, non-nullable boolean, but even if I recall that correctly, I suppose we could set those instances as true/false/undefined enumerables rather than true/false booleans. I think that may resolve any conflict.

cookiecrook commented 4 years ago

Also, authors may be confused that they'd need to use boolean values to set some DOM properties, and boolean-like strings for others.

el.ariaModal = true;
el.ariaInvalid = "true";

Presumably this is the fault of ARIA's value patterns, not IDL, but the change from DOMString to other values makes the author confusion more likely.

domenic commented 4 years ago

Yeah, that's unfortunate, but I don't see any way around it. There are two different syntaxes used on the HTML side, <el modal> vs. <el aria-invalid="true">. And so we need to use two different syntaxes on the JS side as well, el.modal = true vs. el.ariaInvalid = "true".

(Note that this is all off-topic. This issue is discussing integer attributes. I mention this because other threads where these things have been discussed previously have more context.)

cookiecrook commented 4 years ago

Note: I meant ariaModal above (corrected), but yes, the point is the same.

domenic commented 4 years ago

Oh, in that case, I think they would both use = "true". Because <el aria-modal> does not work, right? It has to be <el aria-modal="true">?

cookiecrook commented 4 years ago

I don't think it's off topic. If the ARIA WG decides to reflect as something other than DOMString, it should reflect as many specific relevant types as possible, and potential for author confusion should weigh into that decision.

cookiecrook commented 4 years ago

Oh, in that case, I think they would both use = "true". Because <el aria-modal> does not work, right? It has to be <el aria-modal="true">?

In that case, are you saying we'd need an enumerated boolean: "true"/"false" in addition to the tristate "true"/"false"/"undefined"?

domenic commented 4 years ago

In that case, are you saying we'd need an enumerated boolean: "true"/"false" in addition to the tristate "true"/"false"/"undefined"?

Correct.

cookiecrook commented 4 years ago

Thanks @domenic.

@jnurthen @joanmarie Should we discuss these changes in the Thursday ARIA call?

cookiecrook commented 4 years ago

@domenic and @annevk thanks for your help on this. @sinabahram reminded me yesterday that there are few numeric attributes where the absence of the attribute is meaningful. For example, a progress bar without aria-valuenow is considered an "indeterminate" progress indicator, equivalent to what some UI represents visually as a spinning gear.

<div role="progressbar" aria-valuemin="0" aria-valuemax="100">
  <!-- [sic, no aria-valuenow] -->
</div> 

Can you advise if it's possible to define aria-valuenow as a reflected nullable double?

domenic commented 4 years ago

It is not possible; in those cases there's a default value that is used when the attribute is absent. For example, for HTML's progressEl.position, that default value is -1 for indeterminate progress bars. For HTML's progressEl.value, that default value is 0 for indeterminate progress bars.

cookiecrook commented 4 years ago

So potentially the following could default to -1

aria-value* is a bit more complicated. ARIA has some other computed defaults in the case of authoring errors. Would this table need to change, or are each of these expressible in IDL, too?

Excerpt from: https://www.w3.org/TR/wai-aria-1.2/#authorErrorDefaultValuesTable

WAI-ARIA role Required Attribute Fallback value
scrollbar aria-valuenow If missing or not a number,(aria-valuemax - aria-valuemin) / 2. If present but less than aria-valuemin, the value of aria-valuemin. If present but greater than aria-valuemax, the value of aria-valuemax.
slider aria-valuenow If missing or not a number,(aria-valuemax - aria-valuemin) / 2. If present but less than aria-valuemin, the value of aria-valuemin. If present but greater than aria-valuemax, the value of aria-valuemax.
spinbutton aria-valuemax A value indicating that the spinbutton has no upper bound (accessibility API dependent).
spinbutton aria-valuemin A value indicating that the spinbutton has no lower bound (accessibility APIdependent).
annevk commented 4 years ago

The equivalent attributes on input (min, max, value) are strings, though they can also handle date values and such.

I think it would make sense to let reflection handle your situation (e.g., perhaps default could be a value or algorithm; note that we haven't formalized this to the level of IDL yet in the specification: https://github.com/whatwg/html/issues/3238#issuecomment-601383619) or failing that define the logic for those IDL attributes yourself in prose. It seems like a more useful API for web developers if these return numeric values.

sinabahram commented 4 years ago

I think there is a meta-issue here, so hopefully there is a 1-to-1 mapping to a meta-solution so we don’t need to treat things individually, and we can zoom out and address the entire problem space, or at least enough of it to cover what we care about. Alternatively, I could be completely off, so just let me know if that’s the case.

For any numeric type, where I’m loosely defining numeric to be int, float, double, decimal, whatever you like. If numbers are used to represent it, we have 2 cases:

1: We have an unsigned range e.g. aria-posinset, aria-setsize, etc.

2: We have a signed range e.g. tabindex. See note 1.

Now, across both 1 and 2 above, one of the following conditions must also be true:

Conditions:

A: A specific value, almost always either -1 or 0, has the same meaning as the absence of the attribute.

B: the absence of the attribute is not mappable to a numeric representation, but instead must be represented by null e.g. -1 or 0 are meaningful, so they can’t be used as default values. See note 1 again. Also, see note 2.

Some questions:

  1. So, my first series of questions are, and please forgive my lack of having read the IDL spec, because I haven’t read it, but does this mapping even support the idea of nullable numerics? What does that even mean? I could see a representation in some programming languages, but in most of them, especially those inherited from c/pascal, primitive types do not take null, right? So, int, float, double in c, c++, java, etc. all have a 0-like value as their default, no? So, I just literally don’t understand what a nullable int is. I know I want it to exist, but what does it mean? Is this an implementation detail that’s just made the problem of whomever is tasked with implementing this mapping e.g. if a value exists, reflect, otherwise …. Uhh, “don’t?” or something. Again, having trouble understanding how this would be represented. Obviously in loosely-typed languages, this is not as much of a concern, and maybe that’s all we care about? just let me know.

Oh, and let me be clear, I’m not saying I couldn’t see how to code it. I could see something silly like a pointer being set to 0 to represent null, and if it is non-zero, then and only then it is dereferenced, and obviously once ddereferenced, it maps to the 32-bit or 64-bit representation of whatever numeric it represents. I get how to code it, and I’m sure there’s better ways, but is that what’s done? Forcing a conditional per dereference seems pretty suboptimal.

  1. My next question is, will we do a proper treatment of default mappings as part of this discussion? See note 1. That’s super-important. Or, is that all out of scope? For example, when only condition A is true above, then 0 or -1 can be used to represent the equivalence of null, which is fine, but obviously that doesn’t work when condition B is true.

Notes:

  1. Fun fact, it was brought up on the ARIA WG call that some browsers have a default value for tabindex, the property, when there is no tabindex, the attribute, present, which seems …. Quite surprising, in my opinion. If we’re going to do that, then why in the world are we forcing thousands of developers to slap a useless tabindex of -1 on components just so skip links and scriptable focus can be achieved? This simple default actually being realized across all browsers would save thousands of hours of development and tens of thousands of hours of user frustration for people who need it the most, arguably switch-users. So, I really hope we resolve this ASAP. The suggestion was made to just make anything focusable by a script, to which I say, that I totally agree with that. No clue why that wasn’t the default from day 1. Maybe there’s problems, though?

  2. I am completely avoiding the empty string, epsilon, because that’s meaningless since everything starts off as a string, and then is actually reflected by the browser and then again presumably for our purposes, so really the empty string is not helpful, because these are numeric types once we wish to reason about them, and empty string is not valid for anything that is numeric e.g. tabindex=”” …. Maybe that means something. I have no idea what it means/I think it’s invalid, or at least should be.

I am absolutely positive I have missed something above, but that’s just how I’m thinking about this problem.

Thoughts?

sinabahram commented 4 years ago

I’m unclear how aria-required, a Boolean, is being mapped to -1 instead of a third state in an enumerated type?

Per my last message, the same conditions are true here, no?

For Boolean fields, either they are absence=false or not, if their absence is not the same as false, then they aren’t actually Booleans. They are trinary state fields e.g. aria-expanded has three states: null, false, and true, so an enumerated type is needed, not a -1 representation, right?

annevk commented 4 years ago

For the web platform we only (have to) care about how IDL maps to JavaScript. How other languages want to implement those types is up to them.

The tabindex content attribute can be in a state you cannot discern from the tabIndex IDL attribute, that might be why it's confusing. As to how it generally works and how browsers are supposed to parse it (including the empty string), I recommend reading https://html.spec.whatwg.org/multipage/interaction.html#the-tabindex-attribute.

E.g., in the example below the first div can be focused with a mouse, despite both div elements returning the same value for their tabIndex IDL attribute.

<style>:focus { background:red }</style>
<div tabindex=-1>test</div>
<div>test</div>
<script>
alert(document.querySelectorAll("div")[0].tabIndex === document.querySelectorAll("div")[1].tabIndex)
</script>

I don't think we should copy that. It's useful if the IDL attribute can represent the full set of states.

sinabahram commented 4 years ago

Thanks for the additional info/context.

Yeah, if we can’t distinguish between those two divs, then I don’t see the point. this has profound implications for the user at the end of the day. it’s not just a pedantic or abstract point in a spec somewhere, so I hope we can get this right e.g. distinguish between these states.

cookiecrook commented 4 years ago

@sinabahram wrote:

I’m unclear how aria-required, a Boolean, is being mapped to -1 instead of a third state in an enumerated type?

Not sure where you see that suggested. I think only the numeric attrs would reflect -1 from a missing content attribute. As I understand it, a missing aria-required would reflect "undefined" from the enumerated true/false/undefined type.

sinabahram commented 4 years ago

@cookiecrook sorry if I got this wrong, but you said "So potentially the following could default to -1", and then you listed seven items, one of which was aria-required. Maybe I'm misreading that?

cookiecrook commented 4 years ago

You must be reading from a stale email rather than the live page. IIRC, there were a couple in the copy/paste buffer that I edited out immediately. Sorry for the confusion.

sinabahram commented 4 years ago

@cookiecrook, my apologies. You identified the situation perfectly.

domenic commented 4 years ago

Turning to this issue, since #1261 is wrapping up and IMO this would make sense to figure out next.

My attempt to summarize some of the above, mixing in a few minor suggestions of my own:

Easier cases

(Side note: "non-negative numbers greater than 0" is a pretty silly way to say "positive numbers". Maybe we can clean that up on the HTML side.)

Harder cases

aria-value* have "computed defaults" as discussed in https://github.com/w3c/aria/issues/1110#issuecomment-619285883. It sounds like we want to reflect those into IDL.

Their types should all be double, at least.

It might be simplest to just define these manually, without using the reflection framework. That is what HTML does for <meter>.

mrego commented 2 years ago

Just to update the issue, as last comment is from 2 years ago.

This has been shipped as DOMString in Chromium and WebKit. It has also been implemented in Firefox behind a runtime flag, but hasn't shipped yet because of concerns with this issue (and https://github.com/w3c/aria/issues/1239): https://bugzilla.mozilla.org/show_bug.cgi?id=1628418

annevk commented 1 year ago

I'm interested in fixing this still as well as some of the other adjacent issues at https://github.com/w3c/aria/projects/11.

This work is currently blocked on https://github.com/whatwg/html/issues/8442 (filed here as #1843), which I hope to get to reasonably shortly after the plan has been reviewed. After which tackling this should be relatively straightforward (specification-wise, anyway).

nolanlawson commented 1 year ago

Since string reflection has been shipped in Chromium and WebKit for almost 5 years, I would like to raise the question of whether this change would be web-compatible. For instance:

if (element.posInSet === '1') { // false if `posInSet` is a number instead of a string
  /* ... */
}

Or for an element with role="button":

element.ariaPressed = 0

// `'0'` today, i.e. pressed
console.log(element.ariaPressed)

// `false` with this proposal, since `0` is falsy?
console.log(element.ariaPressed)

The fact that Firefox has not shipped yet is not a guarantee that web authors are not relying on the current Chromium/WebKit behavior. Even ignoring websites that eschew Firefox support, there may be polyfills. For example, we at Salesforce have a polyfill that has shipped on our platform for almost 5 years.

cyns commented 1 year ago

Added to AOM agenda, to see if there's consensus with Nolan there. If so, cyns will follow up with Domenic and Anne on whether a won't fix is ok.

cookiecrook commented 1 year ago

Actions from today's AOM meeting.

  1. @nolanlawson will follow up with @jcsteh: does Mozilla have a strong preference, or just want it resolved either way?
  2. @cookiecrook will follow up with @annevk: please add comment on the compelling use cases for changing the types. General group feeling was was that either was fine, so slight preference to keep what's shipped.
cookiecrook commented 1 year ago

I also took a silent action to trace back why we originally changed these all back to DOMString in the first place. That answer is here, near the bottom of a very, very long thread. https://github.com/w3c/aria/issues/691#issuecomment-385872627

cookiecrook commented 1 year ago

@mrego made a case in https://github.com/w3c/aria/issues/1732#issuecomment-1172563241 for keeping the Element based reflection (FrozenArray<Element>? ariaControlsElements;) but keeping a DOMString reflector with a different DOM property name (DOMString? ariaControls;)... Maybe something similar could be done with the other ARIA attributes that would not affect backwards compatibility with the existing DOMString values.

jcsteh commented 1 year ago
1. @nolanlawson will follow up with @jcsteh: does Mozilla have a strong preference, or just want it resolved either way?

I originally raised this because I worry about author confusion here. I understand the history of having ARIA attributes as strings. However, since we're now exposing them as element properties (largely to make things easier/cleaner for authors), I figured it'd make sense to make the values easier/cleaner/more consistent with the rest of the DOM for authors as well. I've seen authors get confused about ARIA attributes being strings when they intuitively seem like they should be booleans, for example. I figured that confusion would extend to these properties, perhaps more so because they seem like they're integrated more directly into DOM elements.

That said, I acknowledge the web compat risk here and that definitely needs to be considered alongside everything else.

annevk commented 1 year ago

To build on what @jcsteh said:

  1. Consistency with how HTML does its attributes would be less surprising for web developers.
  2. It gives all these properties more of a tangible benefit over using setAttribute().
  3. For enumerated attributes it also allows for feature testing, whereby if you set it to an unknown enumeration (say from a future ARIA) the implementation would ignore it and continue returning the previous value. This is a proven extensibility path that all new enumerated attributes make use of. (This is probably something we can do regardless as these would still be strings so there should be negligible impact. It's just that not all set values remain honored.)
  4. ARIA is already embracing non-string types with element(s).
annevk commented 1 year ago

I rediscovered that the status quo is broken as well as using DOMString? in combination with reflection requires the attribute to be an enumerated attribute and to be limited to known values. So as currently specified we hit several asserts in HTML and end up with spec-UB.

cookiecrook commented 1 year ago

Added Agenda label so @annevk could join to discuss. Perhaps next week, March 9th at 10 AM Pacific?

spectranaut commented 1 year ago

Discussed in ARIA working group meeting: https://www.w3.org/2023/03/09-aria-minutes#t02

nolanlawson commented 1 year ago

Based on the ARIA notes and today's AOM discussion, it sounds like the conclusion is that UAs might move forward with the breaking change of changing the types for the existing ARIA reflected properties.

Just to make my position clear: this sounds fine to me, as long as we have enough of a heads-up in advance and are able to feature-detect the new behavior (which seems possible). This allows us to either 1) tweak our polyfill to preserve the old behavior, or 2) proactively notify customers of a potential breakage.

/cc @gregwhitworth

cookiecrook commented 9 months ago

Gecko shipped reflection recently in 119 to match the shipping behavior of WebKit and Chromium, which makes a breaking change a little less desirable. Agenda+?

It's still failing most WPT tests in 120 though... https://wpt.fyi/results/html/dom/aria-element-reflection.html?label=master&label=experimental&aligned&q=label%3Aaccessibility

jcsteh commented 9 months ago

It's still failing most WPT tests in 120 though... https://wpt.fyi/results/html/dom/aria-element-reflection.html?label=master&label=experimental&aligned&q=label%3Aaccessibility

Gecko shipped ARIA reflection for non-idref attributes. We did not ship element reflection though, which is why those tests are failing.