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(NcRichContenteditable): add aria-placeholder #4407

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Aug 9, 2023

☑️ Resolves

The solution in this PR is different from the suggestion in the linked issue.

Using aria-describedby requires a layout update, adding a wrapper element for NcRichContenteditable with the div[contenteditable] itself and a new visually hidden describing element. Though the layout of the element is not a part of its public interface, I think developers expect the component to be a directly contenteditable element and I prefer to keep current simple layout as long as it's possible.

A good and very simple alternative solution IMO is aria-placeholder.

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Add aria-placeholder

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: rich-contenteditable Related to the rich-contenteditable components labels Aug 9, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Aug 9, 2023
@ShGKme ShGKme requested review from AndyScherzinger and Pytal August 9, 2023 18:54
@ShGKme ShGKme self-assigned this Aug 9, 2023
@AndyScherzinger
Copy link
Contributor

Not an expert in terms of BITV compliance of the solution, else looks good to me.

@Pytal
Copy link
Contributor

Pytal commented Aug 9, 2023

Makes sense though I suspect @michaelnissenbaum may not find this acceptable for similar reasons as on MDN image

@Pytal
Copy link
Contributor

Pytal commented Aug 9, 2023

Will likely have to go with one of the alternative solutions depending on his feedback

@AndyScherzinger
Copy link
Contributor

Can't we just put the text below the input field as suggested my Michael?

@jancborchardt

@michaelnissenbaum
Copy link

I don't recommend using placeholders, whether in HTML (placeholder) or through aria-placeholder. Instead, place the hint below the input field.

@jancborchardt
Copy link
Contributor

Yep, we have enough room in the comments interface, there can be a subline in color-text-maxcontrast.

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 11, 2023

I'll make another PR with a new feature to add an always visible description for NcRichContenteditable.

But making the placeholder visible by default as a description will break existing behavior and may not be suitable for any case. I think, in some cases, component users will want to keep using the placeholder as it currently is.

What do you think about merging this first to make the current placeholder accessible and then adding an additional description field?

@susnux
Copy link
Contributor

susnux commented Aug 11, 2023

Using aria-describedby requires a layout update

Isn't there a aria-description now? I remember seeing this for similar purpose.

Edit: Never mind its still a draft 😞

@susnux susnux merged commit 4c7c12a into master Aug 16, 2023
@susnux susnux deleted the fix/a11y-for-richcontenteditable-placeholder branch August 16, 2023 16:56
@rakekniven
Copy link
Contributor

Can someone add an image showing how the solution will look like? @susnux

@marcoambrosini
Copy link
Contributor

@jancborchardt this is resulting with a hint implemented on all NcRichContenteditable instances. This is not acceptable, we just cannot have this everywhere. (think of Talk for example)

I don't think the comments instructions on how to use the input field are even needed, but regardless of that, let's treat the comments interface as a the only instance of this problem and solve it there? These NcRichContenteditable are usually in context with headings and other hints around them that are pretty clear and requiring an hint in all of them will mess up the UI in most places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: rich-contenteditable Related to the rich-contenteditable components
Projects
None yet
8 participants