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 accname 'flash screen' example to use native clickable label element #206

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook cookiecrook commented Oct 6, 2023

Closes #65


Preview | Diff

@scottaohara
Copy link
Member

@cookiecrook @MelSumner should we call out the fact that this example is invalid HTML? i mean, it's far easier to use this pattern than the 'correctly nested' markup pattern, which could look like:

<label for="flash" id=label>
  <input type="checkbox" id="flash" aria-labelledby="label number times">
  Flash the screen 
</label>
<label for=number>
  <input type="text" value="5" aria-label="number of times" id=number> <span id=times>times</span>.
</label>

but a label is only allowed to contain a single labellable element, and if it weren't for the for attribute pointing to the checkbox, this would start breaking in other ways which wouldn't produce the described accName - but the use of for isn't referenced in the 'consider this' text as being necessary to make this work as the example says it will.

sorry, really don't mean to be so pedantic about this, but it does just strike me as odd for a spec to use an invalid example without at least mentioning it is invalid. ☹️

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 making this an official 'review' per my above comment.

again not in opposition to the example, i just think it needs to state it is invalid markup so as to not have this spec seemingly imply something is valid which html says is not.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Oct 12, 2023

I'm only slightly embarrassed to say I didn't think about the fact that two inputs in a label would make it invalid, especially since the for/id relationship is explicitly defined. I don't think any accessibility implementations would be tripped up by the inclusion of the second input, which also has an explicitly defined aria-label.

But that said, I agree we shouldn't use an invalid example.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Oct 12, 2023

@scottaohara how one of these options

  1. Avoid the second <input> element in the <label> element; use an ARIA textbox instead.
<label for="flash">
  <input type="checkbox" id="flash">
  Flash the screen <span tabindex="0" role="textbox" aria-label="number of times" contenteditable>5</span> times.
</label>
  1. Like yours, but avoid the <label> elements entirely, since the controls use aria-labelledby and aria-label.
  <input type="checkbox" id="flash" aria-labelledby="flash number times">
  <span id="flash">Flash the screen</span>
  <input id="number" type="text" value="5" aria-label="number of times">
  <span id="times">times.</span>

@scottaohara
Copy link
Member

fwiw, @cookiecrook, i agree - it 'should/could' to be "OK" - though i believe I've run into some windows based voice dictation software falling down on this sort of thing, where screen readers generally handle it ok these days. anyway... appreciate your alternatives.

and yeah, i think either of those examples works. I really appreciate the first one, specifically as a "html doesn't allow this? oh yeah?" it made me grin.

@cookiecrook cookiecrook marked this pull request as draft October 13, 2023 09:42
index.html Outdated Show resolved Hide resolved
@cookiecrook
Copy link
Contributor Author

Recycling @scottaohara's review with the new diff.

@cookiecrook cookiecrook marked this pull request as ready for review October 13, 2023 23:59
@cookiecrook cookiecrook mentioned this pull request Oct 14, 2023
@cookiecrook cookiecrook changed the title update accname 'flash screen' example to use native HTML inputs update accname 'flash screen' example to use native clickable label element Oct 14, 2023
@cookiecrook
Copy link
Contributor Author

I really appreciate the first one, specifically as a "html doesn't allow this? oh yeah?" it made me grin.

It also keeps the "clickable" functionality of the HTML <label> 👍

index.html Outdated Show resolved Hide resolved
@cookiecrook
Copy link
Contributor Author

@accdc @pkra @spectranaut @jnurthen Please merge?

@accdc
Copy link
Contributor

accdc commented Nov 3, 2023

I'm happy merging since we all appear to be in agreement. I'm not sure how to resolve the branch merge conflicts though so will need someone's assistance who has a better handle on Git than I.

@accdc accdc merged commit 681094c into main Nov 3, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Nov 3, 2023
…lement (#206)

SHA: 681094c
Reason: push, by accdc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@cookiecrook cookiecrook deleted the fix-embedded-control-example branch November 3, 2023 23:21
@cookiecrook
Copy link
Contributor Author

I'm not sure how to resolve the branch merge conflicts though

I didn't see any merge conflicts. Merge was successful. Deleting branch.

@cookiecrook
Copy link
Contributor Author

Oh... I see. @jnurthen forward merged main into this one instead of a rebase squash. Thanks.

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.

Example 3 is wrong
5 participants