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

Fix/form toggle accessibility #909

Merged
merged 5 commits into from
May 29, 2017
Merged

Conversation

youknowriad
Copy link
Contributor

This PR closes #906 by:

  • Adding an optional "On/Off" Toggle to the FormToggle component
  • Transforming the FormToggle component to an inline component
  • Dropping the embedded label from the FormToggle component
  • Explicitly use id/for for the input/label of the "Pending review" toggle.

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 26, 2017
@youknowriad youknowriad self-assigned this May 26, 2017
@youknowriad youknowriad requested review from afercia and aduth May 26, 2017 09:28
</label>
</div>
{ showHint &&
<span className="components-form-toggle__hint">
Copy link
Member

Choose a reason for hiding this comment

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

Per #906, it seemed the recommendation would be to disable this from being read by screen readers, I presume because the input's own value state would suffice.

For this reasons, our decision was to keep using visible "on" and "off" text, hidden to assistive technologies with aria-hidden="true" because the state change is already announced by screen readers.

Also discussing the hint more generally at #906 (comment)

checked,
onChange = noop,
showHint = true,
id = 'toggle-' + this.id,
Copy link
Member

Choose a reason for hiding this comment

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

Should we need to suffix this or manipulate it in any way, or can we treat it as verbatim presumed unique from the parent passing the prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should treat it as unique from the parent to allow its usage for the label htmlFor without the need to "know" the prefix.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh, I missed that this was assigning the default. But why do we need a default? If the parent component isn't passing an ID, what purpose does the ID have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It's probably safe to delete.

@youknowriad youknowriad force-pushed the fix/form-toggle-accessibility branch from 3228a9b to 916a98b Compare May 26, 2017 14:48
opacity: 0;
margin: 0;
padding: 0;
z-index: z-index( '.components-form-toggle__input' );
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the need for z-index? Does it need to be higher than a dialog?

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 a trick to allows the onChange handler to trigger when we click on the invisible input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh Higher than the dialog probably not. I'm updating

Copy link
Member

@aduth aduth May 26, 2017

Choose a reason for hiding this comment

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

I'm wondering if some logical groupings in the _z-index.scss, even just a few comments, could help avoid this temptation of assigning it a value a little more than the next highest. Like "Relative Siblings", "Layout", "Dialogs", etc. We have some grouping already, but mostly by related components. Dunno if that's a more sustainable pattern than lower-to-highest. It's a difficult problem, and one which I don't think needs to be solved here, but I expect will continue to surface over time.

Related discussion at #637

Copy link
Contributor Author

@youknowriad youknowriad May 26, 2017

Choose a reason for hiding this comment

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

Yes, I like the idea of grouping by "type" and inside the lowest type "relative siblings" group by related components.

@youknowriad youknowriad force-pushed the fix/form-toggle-accessibility branch from 4d1e195 to 619f91c Compare May 26, 2017 15:14
}

PostStatus.instances = 1;
Copy link
Member

@aduth aduth May 26, 2017

Choose a reason for hiding this comment

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

It's not a terribly complex pattern, but unfortunate in how it forces a class component. What do you think about a higher-order component for convenience?

function MyComponent( { id } ) {
	return <div id={ id } />;
}

export default withUniqueId( 'prefix-' )( MyComponent );

Not to be solved here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny, because I was thinking the exact same thing 👍 (maybe uid is better as prop or uniqueId)

Copy link
Member

Choose a reason for hiding this comment

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

(maybe uid is better as prop or uniqueId)

I don't feel strongly that it's necessary, but between the two I might prefer uniqueId just for clarity by verbosity.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I like the idea of treating these components as inputs and leaving it to the developer to create the associated label. Or at least I think this helps avoid the sorts of issues observed in #906. We should start to look at how to document components as such. Code-wise I think this looks good. Still curious to see where #906's discussion goes, but with current usage not making use of the hint, I think it can be reevaluated later (or dropped now and introduced later? Not really sure its value if we choose not to use it).

@youknowriad youknowriad force-pushed the fix/form-toggle-accessibility branch from 619f91c to 1a2c89b Compare May 29, 2017 08:43
@youknowriad youknowriad merged commit 48ab106 into master May 29, 2017
@youknowriad youknowriad deleted the fix/form-toggle-accessibility branch May 29, 2017 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch toggle accessibility
2 participants