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

Update from PR #1018 for nameFrom: heading #1860

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook cookiecrook commented Jan 26, 2023

Part of #w3c/accname/issues/138

Adds nameFrom: heading (this overcomes PR #1018)

Will update these along the way after further discussion and review.

Implementation tracking


Preview | Diff

index.html Outdated Show resolved Hide resolved
@cookiecrook cookiecrook requested a review from jnurthen January 26, 2023 03:04
@cookiecrook
Copy link
Contributor Author

Adding three reviewers that were prior contributors to #1018.

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

just a quick list of the roles that will get name from heading

  • alertdialog
  • article
  • complementary
  • dialog
  • form
  • region (with HTML caveat)

My initial review:

complementary may need a caveat added per the conditional complementary role for the aside element.

similarly, I think form would need a host language caveat mentioned as well, as while I think this update works since ARIA requires role=form have an accName, I don't think every HTML form element should automatically get an accName. That'd have the potential to add extra and unnecessary landmark verbosity to many web pages

should we consider adding group to this as well? Esp. since it's more likely for AT to even expose an element with role=group if it's named. If there's a heading there, it could be done for authors without the need to use aria-labelling. But per my comment above, maybe this would have the inverse impact of creating too much verbosity where things were already "fine" from the user's perspective?

additionally, navigation should be considered for naming by heading. many instances of people using headings (visible or not) to name navigation landmarks via aria-labelledby.

i'm quite a fan of all other aspects of this PR.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Feb 13, 2023

@scottaohara wrote:

Thanks.

complementary may need a caveat added per the conditional complementary role for the aside element.

To clarify, are you suggesting nameFrom: heading be removed from both <aside> and role="complementary" or only from the contexts where aside is not complementary?

similarly, I think form would need a host language caveat mentioned as well, as while I think this update works since ARIA requires role=form have an accName, I don't think every HTML form element should automatically get an accName. That'd have the potential to add extra and unnecessary landmark verbosity to many web pages

I don't have a strong opinion either way, so I'll remove it in the PR unless there are objections. To clarify, are you suggesting nameFrom: heading be removed from both <form> and role="form" or something else?

should we consider adding group to this as well? Esp. since it's more likely for AT to even expose an element with role=group if it's named. If there's a heading there, it could be done for authors without the need to use aria-labelling. But per my comment above, maybe this would have the inverse impact of creating too much verbosity where things were already "fine" from the user's perspective?

I think group should remain explicitly named, for the verbosity risk you've outlined.... but I could be convinced if you and others have strong opinions for it.

additionally, navigation should be considered for naming by heading. many instances of people using headings (visible or not) to name navigation landmarks via aria-labelledby.

I support this.

Copy link
Member

@jnurthen jnurthen left a comment

Choose a reason for hiding this comment

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

common/script/aria.js needs moving to aria-common but otherwise looks good.

I like the scope of the other changes being limited to

  • alertdialog
  • article
  • complementary
  • dialog
  • region (with caveats)
    as it is now. We can always add more later if folks think that is desired.

@scottaohara
Copy link
Member

@cookiecrook
Re: complementary and form, yes would need to be clear that while these ARIA roles can get name from heading, the equivalent HTML elements may not (aside) or would not (form) get name from headings.

re: group, agreed. i felt icky suggesting it, and your response didn't wash the ick away. so let's forget it..

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Feb 15, 2023

Re: complementary and form, yes would need to be clear that while these ARIA roles can get name from heading, the equivalent HTML elements may not (aside) or would not (form) get name from headings.

Maybe we should consider taking complementary and region out of the list because of these caveats? We can always add them back in with separate issues.

Especially since we're now working on automated AccName tests...

@scottaohara
Copy link
Member

sounds like a good idea to me.

cookiecrook added a commit to w3c/aria-common that referenced this pull request Feb 16, 2023
…cussion, these will be taken up as separate issues
@cookiecrook
Copy link
Contributor Author

Recycling reviews because I think this addresses all the feedback now.

Affected roles now limited to the following for this PR.

  • alertdialog
  • article
  • dialog

Others that are no longer in this diff, but should be taken as separate PRs due to editorial and host language caveats are:

  • complementary
  • region

Also @jnurthen I have filed the script changes as w3c/aria-common#89 I wasn't sure how to mark that PR as blocking this one. FYI @scottaohara

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Good catch by @scottaohara in review

Co-authored-by: Scott O'Hara <[email protected]>
@benbeaudry
Copy link
Contributor

@cookiecrook This change is a step in the right direction in my opinion. I don't have any issues with it. Merge it, and we'll build it in Chromium eventually!

@pkra
Copy link
Member

pkra commented Jun 2, 2023

@spectranaut @jnurthen perhaps we should have labels for the new process requirements, e.g., something for each step in https://github.com/w3c/aria/blob/main/documentation/process.md#normative-changes).

Then PRs / features like this could also easily be looked up.

@spectranaut spectranaut added the waiting for implementations Cannot be merged until there are two browser impls or one impl + impl commit label Jun 20, 2023
@pkra pkra added this to the ARIA 1.3 milestone Jun 28, 2023
index.html Outdated Show resolved Hide resolved
var fromContent = '';
var fromEncapsulation = '';
var fromLegend = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, the script diff looks jumbled below, but it's just that I removed the stale encapsulation and legend blocks, and added nameFrom heading before contents and prohibited. Although I did not touch contents and prohibited, the GitHub diff viewer is intertwining those sections with the removed encapsulation and legend sections.

</div>
<section id="namefromauthor">
<h4>Roles Supporting Name from Author</h4>
<div id="index_fromauthor"></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is whitespace correction, which I would normally be opposed to in a substantive diff, but the spaces vs tab inconsistency was in a section that I had to modify anyway. I claim editorial discretion.

@@ -3018,7 +3036,6 @@ <h2>Definition of Roles</h2>
<div class="role-description">
<p>A dialog is a descendant window of the primary window of a web application. For <abbr title="Hypertext Markup Language">HTML</abbr> pages, the primary application window is the entire web document, i.e., the <code>body</code> element.</p>
<p>Dialogs are most often used to prompt the user to enter or respond to information. A dialog that is designed to interrupt workflow is usually modal. See related <rref>alertdialog</rref>.</p>
<p>Authors MUST provide an accessible name for a dialog, which can be done with the <pref>aria-label</pref> or <pref>aria-labelledby</pref> attribute.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This is an editorial removal. Not substantive.

Accessible Name Required is defined as True below, so this normative requirement remains. I'm removing excessive duplication that will be resolved in the AccName spec with w3c/accname/issues/182.

index.html Outdated
@@ -6802,7 +6824,7 @@ <h5>Presentational Role Inheritance</h5>
<div class="role-description">
<p>A <rref>landmark</rref> containing content that is relevant to a specific, author-specified purpose and sufficiently important that users will likely want to be able to navigate to the section easily and to have it listed in a summary of the page. Such a page summary could be generated dynamically by a user agent or assistive technology.</p>
<p>Authors SHOULD limit use of the region role to sections containing content with a purpose that is not accurately described by one of the other <a href="#landmark_roles">landmark roles</a>, such as <rref>main</rref>, <rref>complementary</rref>, or <rref>navigation</rref>.</p>
<p>Authors MUST give each element with role region a brief label that describes the purpose of the content in the region. Authors SHOULD reference a visible label with <pref>aria-labelledby</pref> if a visible label is present. Authors SHOULD include the label inside of a heading whenever possible. The heading MAY be an instance of the standard host language heading element or an instance of an element with role <rref>heading</rref>.</p>
<p>Authors MUST give each element with a region role a brief label that describes the purpose of the region. For host languages that support elements with the default role of region (e.g., HTML <code>section</code>), authors would need to specify an explicit <code>role="region"</code> on the element to enable the nameFrom: heading technique.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This diff was originally authored by Jon Gunderson in #1018 and further edited by @scottaohara.

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@@ -3554,7 +3571,6 @@ <h2>Definition of Roles</h2>
<div class="role-description">
<p>A <rref>landmark</rref> region that contains a collection of items and objects that, as a whole, combine to create a form. See related <rref>search</rref>.</p>
<p>A form can contain a mix of host language form controls, scripted controls, and hyperlinks. Authors are reminded to use native host language semantics to create form controls whenever possible. If the purpose of a form is to submit search criteria, authors SHOULD use the <rref>search</rref> role instead of the generic <code>form</code> role.</p>
<p>Authors MUST give each element with role <code>form</code> a brief label that describes the purpose of the form. Authors SHOULD reference a visible label with <pref>aria-labelledby</pref> if a visible label is present. Authors SHOULD include the label inside of a heading whenever possible. The heading MAY be an instance of the standard host language heading element or an instance of an element with role <rref>heading</rref>.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This is an editorial removal. Not substantive.

Accessible Name Required is defined as True below, so this normative requirement remains. I'm removing excessive duplication that will be resolved in the AccName spec with w3c/accname#182.

@@ -6879,7 +6895,6 @@ <h5>Presentational Role Inheritance</h5>
<div class="role-description">
<p>A <rref>landmark</rref> containing content that is relevant to a specific, author-specified purpose and sufficiently important that users will likely want to be able to navigate to the section easily and to have it listed in a summary of the page. Such a page summary could be generated dynamically by a user agent or assistive technology.</p>
<p>Authors SHOULD limit use of the region role to sections containing content with a purpose that is not accurately described by one of the other <a href="#landmark_roles">landmark roles</a>, such as <rref>main</rref>, <rref>complementary</rref>, or <rref>navigation</rref>.</p>
<p>Authors MUST give each element with role region a brief label that describes the purpose of the content in the region. Authors SHOULD reference a visible label with <pref>aria-labelledby</pref> if a visible label is present. Authors SHOULD include the label inside of a heading whenever possible. The heading MAY be an instance of the standard host language heading element or an instance of an element with role <rref>heading</rref>.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This is an editorial removal. Not substantive.

Accessible Name Required is defined as True below, so this normative requirement remains. I'm removing excessive duplication that will be resolved in the AccName spec with w3c/accname#182.

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

I added the "waiting for implementations" tag but I was wrong, this isn't ready for implementation -- I didn't look close enough. This change still needs the following before it can be implemented:

  • three approving reviews
  • accname changes
  • tests

This new process is a work in progress, so let me know if there are questions/concerns!

@jcsteh
Copy link

jcsteh commented Dec 12, 2023

My only concern here is performance. The content of container elements like these can be potentially massive, and because of potential mutations, it's not even as simple as just caching the first heading when we build the tree. Is it reasonable for UAs to impose some limit on how many nodes we check? If so, I wonder if that needs to be specified somehow?

@cookiecrook
Copy link
Contributor Author

@jcsteh wrote:

Is it reasonable for UAs to impose some limit on how many nodes we check?

Certainly possible, but where to draw the line? Some web dev will hit an arbitrary depth limit. Do we tie it to DOM or rendering heuristics? My original issue was intended to allow some spec permissiveness so that implementations could account for this as best they could, which may eventually inform a best path forward... AS of now, some of those heuristics are explicitly disallowed by the spec.

What modification do you propose?

index.html Outdated Show resolved Hide resolved
scottaohara added a commit to w3c/html-aam that referenced this pull request Feb 6, 2024
closes #457

Related to the following:
- w3c/aria#1860
- w3c/accname#229 (this needs to be merged so the new links to 'accName: name from heading' will work
@jnurthen jnurthen added the waiting for implementations Cannot be merged until there are two browser impls or one impl + impl commit label Apr 16, 2024
pkra pushed a commit that referenced this pull request May 23, 2024
closes #457

Related to the following:
- #1860
- w3c/accname#229 (this needs to be merged so the new links to 'accName: name from heading' will work
@pkra pkra added the spec:aria label Jun 14, 2024
@aleventhal
Copy link
Contributor

I'm no longer blocking this, so @jnurthen you're up next and then this can finally land.

@aleventhal
Copy link
Contributor

aleventhal commented Jul 9, 2024

Ah, this is "waiting for implementations", which means we should land the changes in Blink and not wait for it to be merged into the spec, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:aria waiting for implementations Cannot be merged until there are two browser impls or one impl + impl commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARIA Spec could be more flexible when elements with "nameFrom:author" are left unlabeled by the author
9 participants