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

Issue899 accname from heading #1018

Closed
wants to merge 25 commits into from
Closed

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Jul 22, 2019

Update 2023-Jan-25: This PR abandoned in favor of #1860 and can be closed.

No longer attached to w3c/accname#138 (Note: PR modified by @cookiecrook)

Proposed definition of accessible name from heading

Proposed list of roles that support name from heading

Non-Landmark Roles Authoring Techniques

Landmark Roles Authoring Techniques


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 20, 2021, 10:59 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

waiting for function failed: timeout 30000ms exceeded

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.


Preview | Diff

@LJWatson
Copy link

I don't understand the use case for including main in this list. There should only be one element with a role of main available at any one time, so what is the rationale for giving it an accessible name?

The same is true if the container with a role of article represents the main content of the page (as a blog post might for example).

The proposal here says that the first descendant element with a role of heading will serve as the accessible name for any of the containers included on the list. What happens when two of those containers are nested, as in the following (extremely common) pattern:

<main>
<article>
<h1>My post</h1>
...
</article>
</main>

Would both the <main> and <article> elements inherit the heading as accessible name?

@jongund
Copy link
Contributor Author

jongund commented Jul 31, 2019

@LJWatson

I think the issue of nesting of elements that can receive accessible name by heading needs discussion. In general it seems that a heading should only provide an accessible name for one element. So in your case only the article role would get the accessible name. But there maybe cases where it could be useful, but in general I think we should be more conservative in this technique providing accessible names.

@scottaohara
Copy link
Member

to @LJWatson's point about main though, when did that get added to the list?

It's not in the listing of elements in the issue?

and seems form was dropped?

@LJWatson
Copy link

@jongund said:

I think the issue of nesting of elements that can receive accessible name by heading needs discussion. In general it seems that a heading should only provide an accessible name for one element. So in your case only the article role would get the accessible name. But there maybe cases where it could be useful, but in general I think we should be more conservative in this technique providing accessible names.>

I agree about being conservative with this. Whatever route is taken, I think the spec text needs to be unambiguous on the matter though.

@jongund
Copy link
Contributor Author

jongund commented Jul 31, 2019

@LJWatson @scottaohara @jnurthen @mcking65

I added a restrictions for naming only the closest ancestor of elements that can be named with headings to address the nesting issue. I also added region role as a potential role to be named with header. This would require in HTML to use <section role="region"> for the heading to name a section element.

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

Apologies @jongund and @mcking65, i was completely mistaken on thinking an element with role=region would be exposed without a name. No screen reader / browser pairing I currently have access to exposed a nameless element with role=region (JAWS, NVDA, VO on macOS and iOS).

So now, as Matt did, I'm questioning my ask to include role=region in this list. I think I could go either way on it now.

@jongund
Copy link
Contributor Author

jongund commented Aug 1, 2019

@cookiecrook
I made the changes based on today's teleconference to require the heading to be the first child element and removed the main role from the list of roles that can be named using a heading role. But I think the restriction of first child would probably not satisfy the original purpose of the issue 899 of labeling elements with the article role. At the FTF this spring James Craig described the use of article as messy and restricting the heading to be first child would probably not help very much in giving unlabeled 'articles' accessible names.

Seems like we could use two potentially different definitions for name from heading:

  1. Stricter requiring first child element to be a heading role for the navigation, complementary and region roles
  2. Looser requirement allowing any descendant element with a heading role for article, dialog and alertdialog roles, with a restriction of the heading apply only to only the closest ancestor.

@cookiecrook
Copy link
Contributor

cookiecrook commented Aug 1, 2019

@jongund wrote
I made the changes based on today's teleconference to require the heading to be the first child element

Option 1 is way too prescriptive and will not solve the issue for most of the suggested use cases. As one anecdotal example, Facebook articles, when they use headings (many FB articles do not), never place them as the first child of the article. Often, even the first leaf node with content is also not a heading... It's an image, or a link, or another control.

Option 2 is a little better, but "~first heading" would be better than "~nearest in the DOM hierarchy." For example:

article

  • div > div > heading (expected heading to choose, but excluded due to algorithm)
  • content
  • div > heading (included b/c nearer in tree, even though later in the DOM content)

@cookiecrook
Copy link
Contributor

Also, first heading may be more performant than nearest by depth, since the latter requires loading more content to calculate the comparison.

@jongund
Copy link
Contributor Author

jongund commented Aug 2, 2019

@LJWatson @scottaohara @cookiecrook

Updated name from heading to account for first child requirement for landmark roles and more general requirement for other roles.

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
index.html Outdated Show resolved Hide resolved
Trying to get a clean diff with all the previously discussed changes, before recycling all the review requests.
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
@cookiecrook cookiecrook self-requested a review October 8, 2022 00:12
@cookiecrook
Copy link
Contributor

@scottaohara @accdc This may be ready for re-review now.

@cookiecrook cookiecrook requested a review from accdc October 8, 2022 00:13
@cookiecrook cookiecrook self-assigned this Oct 8, 2022
@cookiecrook cookiecrook requested review from cookiecrook and removed request for cookiecrook October 8, 2022 00:18
@jnurthen
Copy link
Member

jnurthen commented Oct 8, 2022

@scottaohara @accdc This may be ready for re-review now.

I think it needs all the encapsulation and legend stuff pulling out of it.

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.

some suggested edits and notes.

I don't GitHub is properly showing all the changes for the index file though. So I'm going to need to do another pass on this.

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
@scottaohara
Copy link
Member

scottaohara commented Oct 11, 2022

@jnurthen @cookiecrook - I'm afraid GitHub is all wonky on this PR. There are a bunch of resolved comments with changes that @cookiecrook made that aren't showing up in the file diff for me, nor do they appear in the preview diff of the spec. Is this just an issue for me (user error?), or is this happening for others as well?

As mentioned, I did my first pass based on what was available in the file diff this morning, but I think I need to come back to this later. I kinda wonder if a fresh PR is in order here detangle this and help mitigate the merge conflict of the aria.js file?

@accdc
Copy link

accdc commented Oct 12, 2022

I'm thoroughly confused by the code spaghetti and can't pick it apart either. If someone could please condense and resubmit this I would appreciate it.

@cookiecrook
Copy link
Contributor

I have taken assignment and am working through that as time allows. I will consider the fresh PR suggestion.

@cookiecrook
Copy link
Contributor

@scottaohara wrote:

There are a bunch of resolved comments with changes that @cookiecrook made that aren't showing up in the file diff for me,

That might be accurate. A lot of what I did was back out changes that were unnecessary, including all the whitespace diffs and redundant guidance/requirements that was already covered elsewhere. If you have a specific change that is still in the branch but not showing up in the diff, please let me know. That'd push me over to start a new PR entirely. Note: I've turned this back into a Draft PR until I resolve the merge conflicts.

@jnurthen
Copy link
Member

I tried rebasing this onto main but with the myriad of commits (especially stuff that was later reverted) I’m not sure that is a good idea. IMO closing this and creating a new PR would be the best course of action.

cookiecrook and others added 4 commits January 25, 2023 14:21
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
cookiecrook added a commit that referenced this pull request Jan 26, 2023
@cookiecrook
Copy link
Contributor

This PR is abandoned in favor of #1860 and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants