feat(WCAG): Associate field descriptions to inputs #4896
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
Component Testing Report Updated Sep 13, 2023 2:31 PM (UTC)
|
pedrobonamin
force-pushed
the
feature/edx-419-field-descriptions-not-associated-with-fields-wcag-131
branch
from
6080007 to
d9bbd02
Compare
pedrobonamin
force-pushed
the
feature/edx-419-field-descriptions-not-associated-with-fields-wcag-131
branch
from
d9bbd02 to
39b2979
Compare
|
No changes to documentation |
pedrobonamin
force-pushed
the
feature/edx-419-field-descriptions-not-associated-with-fields-wcag-131
branch
from
f99b502 to
b41a522
Compare
pedrobonamin
force-pushed
the
feature/edx-419-field-descriptions-not-associated-with-fields-wcag-131
branch
from
b41a522 to
5c7efbf
Compare
| @@ -67,6 +67,7 @@ export type PortableTextEditableProps = { | |||
| scrollSelectionIntoView?: ScrollSelectionIntoViewFunction | |||
| selection?: EditorSelection | |||
| spellCheck?: boolean | |||
| describedBy?: string | |||
There was a problem hiding this comment.
Could we rather support arbitrary props forwarding here, as there are already quite a few props to this component?
That way we could send in our own HTML props whatever they might be, and don't have to be overly specific about props we don't really care about in this context.
So something like this:
<SlateEditable
{...props}
autoFocus={false}
className={props.className || 'pt-editable'}
decorate={decorate}
onBlur={handleOnBlur}
onCopy={handleCopy}
onDOMBeforeInput={handleOnBeforeInput}
onFocus={handleOnFocus}
onKeyDown={handleKeyDown}
onPaste={handlePaste}
readOnly={readOnly}
renderElement={renderElement}
renderLeaf={renderLeaf}
scrollSelectionIntoView={scrollSelectionIntoViewToSlate}
/>
There was a problem hiding this comment.
It could be simpler, but the problem that I see doing that is that we are memoizing the component and we will need to listen to those changes in the useMemo.
If we add the props object to the useMemo, every re render will recalculate the useMemo again.
There was a problem hiding this comment.
Could we rather support arbitrary props forwarding here
For that to work we'll also have to make PortableTextEditableProps extend ComponentProps<typeof SlateEditable>, right? I don't have the full picture, but seems nice to keep the props interface minimal here? Guess it's also something we can always consider doing later too.
On a related note: Any reason to not name it aria-describedby here? That would help us avoid ambiguity if we decided to support arbitrary props forwarding some time in the future.
| * and added to the descriptive element id. | ||
| * @internal | ||
| */ | ||
| export function constructDescriptionId( |
There was a problem hiding this comment.
Minor detail, but I think it's more common for us to use create (or get) as prefix for this type of utility functions, so maybe rename to createDescriptionId if it's not too much trouble?
| @@ -67,6 +67,7 @@ export type PortableTextEditableProps = { | |||
| scrollSelectionIntoView?: ScrollSelectionIntoViewFunction | |||
| selection?: EditorSelection | |||
| spellCheck?: boolean | |||
| describedBy?: string | |||
There was a problem hiding this comment.
Could we rather support arbitrary props forwarding here
For that to work we'll also have to make PortableTextEditableProps extend ComponentProps<typeof SlateEditable>, right? I don't have the full picture, but seems nice to keep the props interface minimal here? Guess it's also something we can always consider doing later too.
On a related note: Any reason to not name it aria-describedby here? That would help us avoid ambiguity if we decided to support arbitrary props forwarding some time in the future.
|
@bjoerge, @pedrobonamin - I have made a suggestion on how we can forward those props in a PR targeting this branch: |
There was a problem hiding this comment.
Please consider my suggestion on how we can support whatever html props.
I'm OK with this approach 👍🏼 |
This will forward html props for the editable element to the slate editable. It will also get rid of the need of having an own wrapper div element to put the ref on. Point it directly on the editable element.
This doesn't need to be memoed, at least not after supporting restProps here. Debugging shows that the nodes inside are not re-rendered by removing this memo, which is what we care about.
pedrobonamin
deleted the
feature/edx-419-field-descriptions-not-associated-with-fields-wcag-131
branch
Description
In the editor, some fields have a description in addition to just a label. This description is not associated with the field in code. This makes it harder to discover for some users, including screenreader users.
With this changes, when fields have a description, this description will be associated to the input by using
aria-describedby. This is discoverable by screen readers.Note: Couldn't make it work in portable text editor correctly, as the focused element in PTE is not the same one that receives the aria-describedby value when passing the prop , a follow up ticket will be created for this.
What to review
Is there any missing field type to consider?
To trigger the descriptions using voice over functionality you have to:
cmd+shift+option+/to show the information.Notes for release
WCAG : When fields have a description, associate them to the input by using
aria-describedby.aria-describedby.accesibility.mov