Validating `accessibleNode` properties

[alice]

Validation options

Note: these are by no means mutually exclusive, and we may want a combination of several.

1. No validation

   el.accessibleNode.role = "banana";
   el.accessibleNode.role; // "banana"
   

   Mental model: this is a "POJO" where I can read out whatever I set. Pro: Easy to understand model; low implementation cost. Con: Hides bugs caused by invalid values.

2. Validate keyword values on set (like .style)

   el.accessibleNode.role = "banana";
   el.accessibleNode.role; // null
   
   el.accessibleNode.role = "button";
   el.accessibleNode.role; // "button"
   

   Mental model: type safety - I can only set valid enum values for keywords. Pro: Can depend on getting values from accessibleNode to return valid value. Con: Greater implementation cost; potentially confusing.

3. Provide an explicit hook for feature detection, along the lines of CSS.supports()

   if (AOM.supports("role", "switch"))  // TODO: bikeshed where this method should live
       el.accessibleNode.role = "switch";
   else
       el.accessibleNode.role = "button";
   

   Mental model: Can explicitly check what is and isn't valid. Pro: Allows "louder" (warning or exception) validation failures since we have a way to avoid them; don't need to modify an accessibleNode object to validate (e.g. watching ARIA attributes and manually applying in the case of a custom element). Con: More lines of code required to check valid values.

4. (Maybe ruled out? Note that CSS OM has gone with this choice after much deliberation.) In conjuction with .supports() or similar, have an explicit setter method which throws an exception if an invalid value is used (would definitely want a way to check validity without a try/catch):

   el.accessibleNode.set("role", "banana");  // throws an exception
   // or
   el.accessibleNode.setRole("banana");  // throws an exception
   
   // explicit getter?
   el.accessibleNode.role();  // null, also this line is never reached because it already died
   
   // ES6 getter with no setter?
   el.accessibleNode.role; // as above
   
   // Generic getter?
   el.accessibleNode.get("role");  // same again
   

   Mental model: More explicit type safety: if code doesn't throw an exception, I can rely on it being valid. Pro: Typo bugs show up immediately. Con: Need to code defensively to avoid exceptions, especially in the case of feature detection.

5. In conjunction with any of the above we may want to provide enum constants for all keyword values (note: this is currently in the spec):

   if ("ROLE_SWITCH" in AOM)  // TODO: bikeshed where enums should live
       el.accessibleNode.role = AOM.ROLE_SWITCH;  // TODO: bikeshed enum structure
   else
       el.accessibleNode.role = AOM.ROLE_BUTTON;
   

   Mental model: I should never need to explicitly pass a string to a keyword value. Pro: No strings; much harder to make typo errors; can use enum values for feature detection. Con: Bloats API.

Addendum:

• (2a.) Validate keyword values on set (like .style) plus log a warning if attempting to set an invalid value.

  el.accessibleNode.role = "banana";  // Warning: attempting to set invalid value for "role".
  el.accessibleNode.role; // null
  

  Mental model: Same as [2] plus console warning. Pro: Some development time feedback when things aren't right. Con: Console warning noise.
• (0.) No getter at all (throw on get). Cf. #71 Punt the getter part to a new phase between Phase 1 and Phase 2.

Validation use cases

Runtime

• Feature detection
• (With reflection from ARIA attributes) validating third party ARIA

Development time

• Console "playing" with the API
• Console log debugging to understand existing code
• Type safety (i.e. code to avoid exceptions)

[alice]

My slight preference is still (2):

• Since we already have some type safety for things like boolean and numeric values, it would be consistent to validate keyword values as well.

• It would make code like this trivial:

  // In custom-element.html
  
  // Set role if there is no valid author-provided ARIA role:
  
  // Without validation:
  if (!this.accessibleNode.role ||  
      !libraryMethodValidatingRoleWithBrowserSniffing(this.accessibleNode.role)) {
      this.accessibleNode.role = "checkbox";
  }
  
  // With .supports():
  if (!this.accessibleNode.role || !AOM.supports(this.accessibleNode.role))
      this.accessibleNode.role = "checkbox";
  
  // With validation on set:
  if (!this.accessibleNode.role)
      this.accessibleNode.role = "checkbox";
  

  [EDIT: custom elements don't have any attributes at construction time; this would need to either happen at connectedCallback time, or else custom element authors would need to explicitly watch relevant ARIA attributes.]

  I think we all agree that getting the computed role is not appropriate for this use case since it will, even in a minimal version, require a layout pass.

I'm prepared to go with (1) and (3) if there is otherwise consensus around this, but I think we should carefully consider all our options.

[robdodson]

Without at least a console warning or an error I think 2 will lead to a lot of developer confusion and time spent debugging typos. Something will work in 3 browsers and be null in the fourth and they'll wonder if AOM is just buggy. Or they'll make a typo and spend a ton of time reading through all of their code, only to smack their head 10 minutes later when they realize the mistake. I imagine in that moment they'll say "why doesn't this thing warn me?"

Because it's not explicit that it's doing validation, it requires the developer have extra knowledge of implicit behavior. My personal experience is folks tend to use new APIs without full knowledge of all their edge cases. So if something is null they may not jump to "ah I must have used an invalid role", instead they'll just go "hm, why is this null?"

I think my vote is for 1 + 3, and if that's not feasible, then 4

[alice]

Or they'll make a typo and spend a ton of time reading through all of their code, only to smack their head 10 minutes later when they realize the mistake. I imagine in that moment they'll say "why doesn't this thing warn me?"

Right, but the flip side is that not doing any validation may give people a false confidence that their code is working when it's not, meaning that errors won't be picked up until much later in the process when someone is doing actual screen reader testing. The error is there either way.

I think of this as a kind of runtime type safety. Obviously .style sets the precedent, though I don't know how widely that's used or understood, and it's marginally different because it reflects out to the attribute so it's obvious sooner that it's happened. Otherwise, it's the exact same philosophy.

[robdodson]

That's why I think any solution should log a warning or throw an error. If I also had an API to detect the presence of a role I could avoid filling my console with warnings

[alice]

I'm not wild about the console warning idea.

• If we do (1) with a warning, we're just contributing to warning spam without any guarantee that developers will see the warning in all of their existing warning spam.
• If we do (2) with a warning, then using it as a shortcut for the feature detection case will create warning spam. At least (2) and (3) with a warning give you a way to avoid creating warnings while also providing some kind of consequence for ignoring them other than just creating warning spam.

One thing we could do is instead of a console warning, provide some kind of specialised warning in the devtools pane with a link to the line number which generated it.

[robdodson]

yeah that's why I suggested using supports() to avoid the warning spam.

One thing we could do is instead of a console warning, provide some kind of specialised warning in the devtools pane with a link to the line number which generated it.

Would that be guaranteed to show up in all devtools panels or would this be Chrome specific? Couldn't you have a role that's supported in Chrome but not yet supported in other browsers?

[alice]

supports() doesn't solve the warning spam issue, since you have to know to use it before you can avoid the warning spam, and because of warning spam it's easy to ignore warnings, so even if the warnings advise you to use supports(), there's a good chance you'll never see it.

Yes the devtools thing would be chrome specific but the warnings are advised only anyway so still browser specific.

[robdodson]

As you mentioned, the warning could instruct folks to use supports() so it's at least somewhat self correcting and we can say we tried to teach folks the right way. Failing silently, on the other hand, doesn't feel like a better alternative to me. It assumes that everyone does screen reader testing in every browser configuration, or that they use an accessibility inspection tool of some kind.

[alice]

Failing silently, on the other hand, doesn't feel like a better alternative to me. It assumes that everyone does screen reader testing in every browser configuration, or that they use an accessibility inspection tool of some kind.

Agreed: this is why I don't like (1). (1) with a warning is still kind of just (1), since I am really not confident that many people will ever look at the warning.

While (2) doesn't fully address this because it's still "silent", you at least have some feedback that you're not doing something valid. (2) with a warning (while I'm still not in favour of the warning at all) at least gives you a reason to look at the warnings.

[asurkov]

We possibly should keep feature detection and validation separated, at least I don't see a good way to wrap them into one method. Compare: check whether 'gridcell' role is supported by the browser (feature detection) vs setting 'gridcell' role out of role 'grid' context (validation).

I like supports() method as a feature detection, it looks similar to DOMImplementation.hasFeature. Not sure however whether 'accessibleNode' interface is a proper place for it. It feels that Window is more appropriate for this or AccessibleDocument if we want that one. Note, the taxonomy mechanism I suggested at https://wicg.github.io/historical-a11yapi/taxonomy.html can be used as feature detection mechanism too, and can be extended by simple supports(taxonomy, value) method.

On the validation part, I believe the most natural way is 2a (throw on setting), which has a pref hit by design, especially if role checking or any other thing like states is more sophisticated than traversing a list of values, for example, when checking a role's context.

So if avoiding a perf hit, the 2nd approach looks reasonable with me, and I like it as a first step towards turning the 'get' into a computed value, which of course implies validation feature by definition. I would avoid though having strong defined Roles enum since it doesn't make validation any easier but reduces flexibility in defining new roles.

Having said that I'm not sure that we have to sort out the validation and feature detection mechanisms for the phase 1 (see issue #71 )

[alice]

We possibly should keep feature detection and validation separated, at least I don't see a good way to wrap them into one method. Compare: check whether 'gridcell' role is supported by the browser (feature detection) vs setting 'gridcell' role out of role 'grid' context (validation).

Right, I think we're all in agreement that the "incorrect context" type validation is out of scope here. If anything, what would be in scope would be:

• "type safety" for enum types (accessibleNode.role = "banana")
• feature detection

I like supports() method as a feature detection, it looks similar to DOMImplementation.hasFeature. Not sure however whether 'accessibleNode' interface is a proper place for it. It feels that Window is more appropriate for this or AccessibleDocument if we want that one.

Agreed - at the very least it should be a static method rather than an instance method, but I agree that Window or some other hypothetical global place would be better.

Note, the taxonomy mechanism I suggested at https://wicg.github.io/historical-a11yapi/taxonomy.html can be used as feature detection mechanism too, and can be extended by simple supports(taxonomy, value) method.

This is great! So we'd have the ARIA taxonomy as the default, but also potentially allow other taxonomies in future?

On the validation part, I believe the most natural way is 2a (throw on setting), which has a pref hit by design, especially if role checking or any other thing like states is more sophisticated than traversing a list of values, for example, when checking a role's context.

So one of these?

el.accessibleNode.role = "banana";    // (2a) - log a warning
el.accessibleNode.role = "banana";    // (2b) - throws exception
el.accessibleNode.setRole("banana");  // (4) - explicit method, throws exception

I think most people thought that having a simple setter throw an exception (2b) would be pretty weird, hence (4).

So if avoiding a perf hit, the 2nd approach looks reasonable with me, and I like it as a first step towards turning the 'get' into a computed value, which of course implies validation feature by definition. I would avoid though having strong defined Roles enum since it doesn't make validation any easier but reduces flexibility in defining new roles.

So that would be the "simple type safety" option? i.e.

el.accessibleNode.role = "banana";
el.accessibleNode.role; // null

el.accessibleNode.role = "button";
el.accessibleNode.role; // "button"

Having said that I'm not sure that we have to sort out the validation and feature detection mechanisms for the phase 1 (see issue #71 )

I think if we can come to an agreement it would be good to avoid having this part of the API change later on (i.e. training developers not to use something that they'll later be able to use), and also to avoid this unprecedented behaviour.

[domenic]

(2) seems much more web-idiomatic to me. (5) is very un-web-idiomatic; please don't do (5).

I don't think having multiple ways to set something is great, so I am not a fan of (4).

(3) seems OK but is additive.

(0) is I think impossible in Web IDL.

(2a) is OK but outside the realm of specs, and a little unprecedented. If we were going to start warning for setting invalid enum values I'm not sure I'd start with this API.

[alice]

4 shouldn't imply multiple ways to set things; we would choose one of those options.

[WilcoFiers]

What is the status of this topic? It looks to me like solution 2 was chosen, but with some stuff being outdated it is a little hard to tell.