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

Bug: Aria attributes for ContentEditable are ignored #6789

Closed
BennyAlex opened this issue Nov 4, 2024 · 7 comments · Fixed by #6814
Closed

Bug: Aria attributes for ContentEditable are ignored #6789

BennyAlex opened this issue Nov 4, 2024 · 7 comments · Fixed by #6814
Labels
good first issue Good for newcomers

Comments

@BennyAlex
Copy link

When adding aria-labelledby to the ContentEditable it is not visible in the dom.

<ContentEditable
className={'ContentEditable__root'}
aria-labelledby='TEST'
/>

@BennyAlex BennyAlex changed the title Bug: Aria attributes for contentEditable are ignored Bug: Aria attributes for ContentEditable are ignored Nov 4, 2024
@etrepum etrepum added the good first issue Good for newcomers label Nov 5, 2024
@etrepum
Copy link
Collaborator

etrepum commented Nov 5, 2024

I think this is a regression in #6171 - the aria-* props should be set at a lower priority than {...rest}. I think that moving them before the {...rest} will probably fix this issue. As a workaround in the meantime you can use the camel cased aria props that are defined by this component.

export type Props = {
  editor: LexicalEditor;
  ariaActiveDescendant?: React.AriaAttributes['aria-activedescendant'];
  ariaAutoComplete?: React.AriaAttributes['aria-autocomplete'];
  ariaControls?: React.AriaAttributes['aria-controls'];
  ariaDescribedBy?: React.AriaAttributes['aria-describedby'];
  ariaErrorMessage?: React.AriaAttributes['aria-errormessage'];
  ariaExpanded?: React.AriaAttributes['aria-expanded'];
  ariaInvalid?: React.AriaAttributes['aria-invalid'];
  ariaLabel?: React.AriaAttributes['aria-label'];
  ariaLabelledBy?: React.AriaAttributes['aria-labelledby'];
  ariaMultiline?: React.AriaAttributes['aria-multiline'];
  ariaOwns?: React.AriaAttributes['aria-owns'];
  ariaRequired?: React.AriaAttributes['aria-required'];
  autoCapitalize?: HTMLDivElement['autocapitalize'];
  'data-testid'?: string | null | undefined;
} & Omit<React.AllHTMLAttributes<HTMLDivElement>, 'placeholder'>;

@etrepum
Copy link
Collaborator

etrepum commented Nov 5, 2024

Alternatively if these do need to be higher priority than explicit aria- attributes for some reason, they should not be set when undefined. I don't see why that should be the case and it would complicate the code to do it that way.

@vantage-ola
Copy link
Contributor

hmm, so

/*LexicalContentEditableElement.tsx*/

function ContentEditableElementImpl(
...
  return (
    <div
      ...
      aria-readonly={isEditable ? undefined : true}
      aria-required={ariaRequired}
      autoCapitalize={autoCapitalize}
      className={className}
      contentEditable={isEditable}
      data-testid={testid}
      id={id}
      ref={mergedRefs}
      role={isEditable ? role : undefined}
      spellCheck={spellCheck}
      style={style}
      tabIndex={tabIndex}
      {...rest}
...

will fix this?

@etrepum
Copy link
Collaborator

etrepum commented Nov 10, 2024

It should, the easiest way to find out is to try it

@vantage-ola
Copy link
Contributor

ok. i am sorry but how? by using it in the a new react project? or testing it out in the lexical playground?

@etrepum
Copy link
Collaborator

etrepum commented Nov 10, 2024

Yes, any of those ways would work. You could also write a unit test for it.

@vantage-ola
Copy link
Contributor

Alright, I will do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants