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

Define concept of minimum role #454

Closed
wants to merge 30 commits into from
Closed

Define concept of minimum role #454

wants to merge 30 commits into from

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented Jan 10, 2023

Worked on this with @aleventhal as this concept is necessary for some current HTML features (as identified in the attribute mapping table in this PR) as well as future proposed features, such as popover.

Implementation


Preview | Diff

Worked on this with @aleventhal as this concept is necessary for some current HTML features (as identified in the attribute mapping table in this PR) as well as future proposed features, such as `popover`.
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@adampage adampage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no feedback of substance 😅, just a couple proofing suggestions.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
scottaohara and others added 2 commits January 19, 2023 06:44
Co-authored-by: Adam Page <[email protected]>
Co-authored-by: Adam Page <[email protected]>
Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an approval caveat remove the sentence fragment, at least!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@spectranaut
Copy link
Contributor

Maybe it's time to open issues on the browsers with this change?

needed to call out that an explicit role of none, presentation or generic would still need to result in that role being ignored if other conditions were true.
index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a few problems with this and suggest we schedule a deep dive.

The first is that this doesn't seem to account for native-reserved roles, or roles that we can't yet or haven't yet mimicked with ARIA. For example, <input type="date" autofocus> or <video tabindex="0">… I think this change suggests UAs MUST map both to group, even though it wouldn't be logical to do so. Even if ARIA eventually adds a role for native date pickers, video is likely to remain separate for a while, because there isn't a good method to support the video API that goes along with it.

The second issue is that ARIA's concept of group requires a much more explicit and purposeful author decision, IMHO, than the related concept in the AX API... As a reminder, AXGroup predates ARIA's group by almost a decade, and is used as a generic container. ARIA's group, as I understand it, represents some explicit logical grouping, so I am not certain it's a good change to force promotion of something like <div contenteditable> when the author may have added the relevant role and label to another nearby container. E.g.,

<main aria-label="Edit post">
  <div contenteditable>
    Now I have an extraneous, unlabeled group inside my main, where I hadn't before.
  </div>
</main>

Update b/c I thought of another one. Does the sub-span in this one really need a minimum role?

<main contenteditable="true">
    Obi-Wan never told you what happened to your father.
    He told me enough! He told me you killed him!
    <span contenteditable="false">No. I am your father.</span>
</div>

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@scottaohara
Copy link
Member Author

@cookiecrook thank you for the review.

per your two comments:

...that this doesn't seem to account for native-reserved roles...

it was definitely the intent to account for that, so happy to incorporate changes to make sure that's included / I'll have a think on other wording to make sure that's clear.

re:

so I am not certain it's a good change to force promotion of something like <div contenteditable> when the author may have added the relevant role and label to another nearby container.

that's an interesting idea. Though it makes me think back to past projects I worked on - one being a website builder where there were blocks of editable areas within predefined templates. So a main could have 2 or 3 contenteditable divs within it. Something to think about...

@cookiecrook
Copy link
Collaborator

cookiecrook commented Jan 30, 2023

per your two comments:

FYI Our comments crossed when I added a third to the list: contenteditable="false" or contenteditable="plaintext-only" override cascades on sub-sections may not need explicit roles... example above.

@scottaohara
Copy link
Member Author

scottaohara commented Jan 30, 2023

@cookiecrook we had talked about false not triggering a need for a minimum role. In the comments for contenteditable it indicates the minimum role only changes if the value is true. I have a comment in the source code to mention what happens with plaintext-only when that is added to the standard.

scottaohara and others added 5 commits January 31, 2023 07:59
Co-authored-by: James Craig <[email protected]>
Co-authored-by: Valerie Young <[email protected]>
adds "not mapped" to the ARIA role cell.  there is no perfect role for this right now, and until one is made (if ever) we shouldnt' change this.
index.html Outdated Show resolved Hide resolved
@spectranaut
Copy link
Contributor

We had a deep dive this morning, it seems the main conclusions were:

  • to remove the minimum role of group from contenteditable, as there is already a history of browsers/ATs attemping to make contenteditable work and we can't be sure if making those elements "group" would make things better or worse.
  • there was agreement that the minimum role of contenteditable=plaintextonly could be "textbox"
  • there was quite a bit of discussion about whether or not "minimum role" was the right language (instead of "implicit role for this attribute", opponents say the "implicit role" has a specific meaning related to the tagname so sound cause confusion). @mcking65, who thought it was unclear, committed to reviewing this PR in order to understand.
  • It was decided not to use the language "fallback" because that implies that authors shouldn't rely on the minimum role, when in fact, the aim is to allow them to rely on the minimum role. If adding an attribute makes the semantic meaning of the element clear, then you shouldn't need to also add a role. Therefore validators will have to be updated to not report errors when these attributes are used on elements with no implicit or explicit roles.

@cookiecrook
Copy link
Collaborator

It was decided not to use the language "fallback"

Also that term had been used to describe the fallback roles such as role="switch checkbox" where checkbox was the ARIA 1.0 fallback role if the engine did not yet support the ARIA 1.1 switch role.

@aleventhal
Copy link
Collaborator

aleventhal commented Sep 5, 2023 via email

@jcsteh
Copy link

jcsteh commented Sep 5, 2023

I somehow missed this altogether, so no, i haven't looked at it. I'll endeavour to do so soonish.

@jcsteh
Copy link

jcsteh commented Sep 14, 2023

I'm trying to grasp exactly what this gives us that we didn't have before:

  1. I thought the spec currently required that focusable elements were exposed in the tree, but it seems it doesn't. So, I guess this makes it clear that <div tabindex="0"> should be exposed, for example. Is that a primary intention here?
  2. As far as I know, accesskey is only useful on something that is focusable. So the accesskey is probably covered by the tabindex case.
  3. Is there an advantage of using "group" over "generic"? From what I've seen, group is not normally a role that is focusable. Group is no worse than generic, I guess, but generic is what some browsers already expose in cases like these, so I'm wondering why the change here rather than reflecting what already happens in some cases.
  4. I do understand that this might be necessary for future proposed features, so if we're just setting up groundwork here, that's fine. I'm just trying to understand the impact on current features first.

@scottaohara
Copy link
Member Author

hi @jcsteh, thanks for giving this a review, and yes, your final point is definitely the crux of this proposal - create consistency in how these and other future / semi-present (popover) global attributes are handled, so that the different browsers and AT can coalesce around how generic elements, which should generally be ignored, can be exposed to users / handled by authors.

Some more comments related to your other questions:

  1. Yes, this is meant to not only explicitly identify that focusable elements need to be exposed, but help to standardize how. E.g., moving focus to a generic div when using firefox/chrome produce no announcement of any role with JAWS/NVDA, right now. But presently VoiceOver and Narrator will announce this same element as a 'group' (putting aside whether because the API is already exposing this as a group, or this is just how they presently decided to announce a generic which has been marked as important in the a11y tree).

  2. regarding accesskey (but also draggable), agreed that these attributes will commonly be used on focusable elements - but since they're global attributes, they don't have to be.

  3. The primary advantage again comes down to consistency / match what is already happening in some cases. Allowing AT to expose a 'group' role arguably being more meaningful than exposing a 'generic' role, or having the AT or browser come up with their own unique role description for a generic. Standardizing around 'group' could then allow consistency in how such elements were named, or not, or how if they had a title attribute, or other globally allowed aria-* attributes which are allowed on generics but often don't' get exposed to users in an overt way (if at all), they could instead be exposed in the manner in which they are with the group role.

@jcsteh
Copy link

jcsteh commented Sep 22, 2023

1. Yes, this is meant to not only explicitly identify that focusable elements need to be exposed, but help to standardize how.  E.g., moving focus to a generic div when using firefox/chrome produce no announcement of any role with JAWS/NVDA, right now.

From what I can see, focusing a div produces exactly the same result as focusing a group with NVDA, except that a div reports "section" and a group reports "grouping".

2. regarding `accesskey` (but also `draggable`), agreed that these attributes will commonly be used on focusable elements - but since they're global attributes, they don't _have to be_.

accesskey is interesting in that it is global, but as far as I can tell, it isn't useful if the element to which it is applied is not focusable. Firefox and Chrome both currently expose accesskey on a non-focusable element via accessibility APIs, but pressing the access key does nothing. This raises the question of whether we should be exposing this at all in this case, since it's misleading to the user. Making that decision is probably out of scope here, but it does impact whether we bother having a minimum role in this case. I can't see a benefit given the above.

3. The primary advantage again comes down to consistency / match what is already happening in some cases.  Allowing AT to expose a 'group' role arguably being more meaningful than exposing a 'generic' role, or having the AT or browser come up with their own unique role description for a generic.  Standardizing around 'group' could then allow consistency in how such elements were named, or not, or how if they had a `title` attribute, or other globally allowed `aria-*` attributes which are allowed on generics but often don't' get exposed to users in an overt way (if at all), they could instead be exposed in the manner in which they are with the group role.

The problem here is that this assumes either that group is entirely intuitive as a fallback or that it is already being used like this, and I'm not convinced either of these is true. As I noted, unless I'm missing something, group was never really intended to get focus itself, but rather, to be a container for other focusable elements. In that sense, group is no better than generic because in either case, it's not being used for its intended semantic purpose.

@jcsteh
Copy link

jcsteh commented Sep 22, 2023

To put this another, more practical way, if a screen reader user tabs to something and just hears "group", they're not going to know what it is. That's no better to the current situation where the user might tab to something and hear just "section", which is also meaningless. Arguably, it's worse because "group" has existing semantic meaning. At least with generic, screen readers could potentially say "unknown element" or something like that, which would signal to the user that the screen reader doesn't have sufficient information about what this is.

@BrettLewisVispero
Copy link

BrettLewisVispero commented Sep 22, 2023 via email

@scottaohara
Copy link
Member Author

had a call with aaron and james today
agreed that accesskey can be dropped from this
no disagreement about introducing the concept
still not 100% on tabindex, but not against it - it's just a change that may create other issues to review (re: naming divs with title which are now groups because of tabindex=0)
can this be merged with the other work i'm doing for contextual role (proably?)

i need to work though if this should be merged, or can be revised into contextual role. i'll work on that soon

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 7, 2023
As a new concept called minimum role was added to the HTML-AAM spec[1],
and the contents corresponding to global attributes in that spec have
been implemented.

[1]: w3c/html-aam#454

AX-Relnotes: n/a.
Fixed: 1474047
Change-Id: I586506e8ddd1e81ad11e92ef9e167586cb41545e
@aleventhal
Copy link
Collaborator

@scottaohara what's the update on this? Which attributes should we start with?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@scottaohara
Copy link
Member Author

OK, finally got back to this and resolved all the merge conflicts. The PR is much simpler to review now in the diff mode.

This now adds the minimum role concept, and to start we're indicating this will occur for the draggable and autofocus attributes.

tabindex is presently not part of this PR (well, it's commented out for further investigation/discussion).

I plan on some editorial revising of this section with the contextual role PR (#484) - but need this to merge first before I work on that anymore.

I think this finalizes the outstanding conversations for this PR (with any potential addition of tabindex being it's own topic for the future)

cc @aleventhal @cookiecrook @jcsteh @benbeaudry

@aleventhal
Copy link
Collaborator

Still LGTM.

@benbeaudry
Copy link

+1!

scottaohara added a commit to web-platform-tests/wpt that referenced this pull request Mar 5, 2024
add tests for minimum role in html aam
w3c/html-aam#454
@scottaohara
Copy link
Member Author

merged via w3c/aria#2220

aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 27, 2024
Bug: w3c/html-aam#454
Change-Id: Ia3930b068311f26f6a7e0be8bbcce53acf820bdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5663675
Auto-Submit: Aaron Leventhal <[email protected]>
Commit-Queue: Mark Schillaci <[email protected]>
Commit-Queue: Aaron Leventhal <[email protected]>
Reviewed-by: Mark Schillaci <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1320576}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants