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

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

Open
annevk opened this issue Oct 31, 2019 · 56 comments
Assignees
Labels
Agenda feature may add new concept(s) to ARIA which will require implementations or APG changes
Milestone

Comments

@annevk
Copy link
Member

annevk commented Oct 31, 2019

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

cc @alice @domenic

@domenic
Copy link
Contributor

domenic commented Oct 31, 2019

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

  • long
  • long limited to only non-negative numbers
  • unsigned long
  • unsigned long limited to only non-negative numbers greater than zero
  • unsigned long limited only to non-negative numbers greater than zero with fallback
  • unsigned long clamped to the range [min, max]
  • double
  • unrestricted double
  • double limited to numbers greater than zero
  • unrestricted double limited to numbers greater than zero

Probably don't consider the long ones.

@alice
Copy link

alice commented Nov 1, 2019

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
Copy link

alice commented Nov 1, 2019

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

  • aria-atomic (true/false, i.e. empty string is invalid but missing value default is false)
  • aria-busy (true/false)
  • aria-colcount (integer >= -1, although zero would be weird)
  • aria-colindex (integer >= 1)
  • aria-colspan (integer >= 1 - this is weirdly inconsistent with aria-rowspan)
  • aria-disabled (true/false)
  • aria-level (integer >= 1)
  • aria-modal (true/false)
  • aria-multiline (true/false)
  • aria-multiselectable (true/false)
  • aria-posinset (integer >= 1)
  • aria-readonly (true/false)
  • aria-required (true/false)
  • aria-rowcount (integer >= -1)
  • aria-rowindex (integer >= 1)
  • aria-rowspan (integer >= 0 - weirdly inconsistent with aria-colspan)
  • aria-setsize (integer >= -1)
  • aria-valuemax (integer)
  • aria-valuemin (integer)
  • aria-valuenow (integer)

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

@annevk
Copy link
Member Author

annevk commented Nov 1, 2019

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
Copy link

alice commented Apr 7, 2020

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
Copy link
Contributor

Initial experimentation was in AOM threads.
WICG/aom#120

More meaty, long history thread inside ARIA tracker starts here:
#691 (comment)

Most relevant comment is this one:
#691 (comment)

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.
#834

@annevk
Copy link
Member Author

annevk commented Apr 9, 2020

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
Copy link
Contributor

domenic commented Apr 9, 2020

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
Copy link
Member Author

annevk commented Apr 9, 2020

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
Copy link
Contributor

@domenic wrote:

I agree that ARIA sticking to that would be better

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

@domenic
Copy link
Contributor

domenic commented Apr 20, 2020

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

@cookiecrook
Copy link
Contributor

cookiecrook commented Apr 20, 2020

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: Add ARIA property string reflection on Element #691 (comment)

  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
Copy link
Contributor

domenic commented Apr 20, 2020

  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
Copy link
Contributor

cookiecrook commented Apr 20, 2020

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
Copy link
Contributor

cookiecrook commented Apr 20, 2020

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
Copy link
Contributor

domenic commented Apr 20, 2020

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
Copy link
Contributor

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

@domenic
Copy link
Contributor

domenic commented Apr 20, 2020

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
Copy link
Contributor

cookiecrook commented Apr 20, 2020

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
Copy link
Contributor

cookiecrook commented Apr 20, 2020

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
Copy link
Contributor

domenic commented Apr 20, 2020

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

Correct.

@cookiecrook
Copy link
Contributor

cookiecrook commented Apr 20, 2020

Thanks @domenic.

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

@cookiecrook
Copy link
Contributor

cookiecrook commented Apr 24, 2020

@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?

@cookiecrook cookiecrook modified the milestone: ARIA 1.2 Apr 24, 2020
annevk added a commit to annevk/aria that referenced this issue Feb 20, 2023
This does not address most of the issues described in w3c#1110, but does provide a better base for ElementInternals support and tackling the issues in w3c#1110.

Corresponding HTML change: whatwg/html#8496.
@annevk annevk changed the title AriaAttributes has many integer attributes with string types ARIAMixin has many integer attributes with string types Feb 21, 2023
@annevk annevk changed the title ARIAMixin has many integer attributes with string types ARIAMixin has many integer attributes with string types and uses DOMString? incorrectly Feb 21, 2023
annevk added a commit to whatwg/html that referenced this issue Feb 22, 2023
Other changes:

* Remove reflection of unrestricted double as it is buggy and unused.
* The DOMString getter steps did not account for a missing attribute.
* The native accessibility semantics map was renamed to the internal content attribute map as it's now a more general reflection concept.

Corresponding ARIA PR: w3c/aria#1876.

Fixes #8442.

Follow-up:

* w3c/core-aam#152
* w3c/aria#1110
* #3238
* #8544
* #8545
* #8926
* #8927
* #8928
* #8930
@cookiecrook
Copy link
Contributor

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

@spectranaut
Copy link
Contributor

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

@nolanlawson
Copy link
Member

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
Copy link
Contributor

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

@cookiecrook cookiecrook removed their assignment Oct 10, 2023
@jcsteh
Copy link

jcsteh commented Oct 11, 2023

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.

@cookiecrook
Copy link
Contributor

Yes, sorry. Gecko is passing all non-element reflection tests, here: https://wpt.fyi/results/html/dom/aria-attribute-reflection.html?label=master&label=experimental&aligned&q=label%3Aaccessibility

@annevk
Copy link
Member Author

annevk commented Oct 16, 2023

I guess if we decide not to improve this, we still need to reconcile with HTML's reflect mechanism somehow as what's currently in the specification doesn't add up. And if we add a new ARIA attribute, we should be really deliberate about whether we should improve the setup from then on or not.

@domenic
Copy link
Contributor

domenic commented Jan 5, 2024

I've opened whatwg/html#10037 to discuss the remaining fixes on the HTML side. It looks like from the discussion over here ("I guess if we decide not to improve this") the current proposal is to not change any of the currently-shipped behaviors, which is my proposal 1A or 1B?

@spectranaut
Copy link
Contributor

Discussed briefly in the ARIA working group: https://www.w3.org/2024/01/11-aria-minutes#t07

thanks @domenic !

@spectranaut
Copy link
Contributor

Also discussed in today's working group meeting: https://www.w3.org/2024/03/07-aria-minutes.html#t08

@rahimabdi
Copy link
Contributor

@keithamus Removing myself as an assignee on this one but happy to help in any way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda feature may add new concept(s) to ARIA which will require implementations or APG changes
Projects
None yet
Development

No branches or pull requests