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): make autocomplete accessible #4904

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Dec 1, 2023

☑️ Resolves

The solution may look a bit complex with dirty places, but I see no better solution except forking Tribute.js.

Complex parts with tribute:

  • No event for "tribute is open"
    • tribute-active-true does not respect async loading
  • No event for "selection changed" or method to get currently selected item
    • Needed for activedescendant
  • When tribute is closed, it is still in the DOM with the old list. On open you first have an old list and then it loads a new actual list (which might be different)
  • If you have many tributes on an element, you cannot get one that was active on tribute-active-true event. Tribute is supposed to be used only once on element
  • If you are using 1 Tribute for many collections, containerClass is not updating on opening another collection (known Tribute.js issue)

refactor(NcRichContenteditable): create 1 tribute for all autocompletes

  • Fixes Tribute was already bound to DIV warning
  • Tribute is supposed to be only once on the element
  • Simplifies further work with Tribute

image

refactor(NcRichContenteditable): move constants out of reactive data

Not required, but simplifies work with actual component's state.

  • smilesList is always a constant and reused across all the instances
  • tribute options are used only once to initialize tribute

refactor(NcRichContenteditable): cancel autoComplete on close

  • Correctly implement debounced method, so that debouncedFunction properties are available and each component instance has its own debounce.
    • Defining method as debounced is not valid in Vue. In this case, we have 1 shared debounce for all component instances.
  • Clear debounce on autoComplete close
    • Otherwise, if you closed autocomplete for fast and then reopen another one, you may receive data from the previous autocomplete after debounce timeout.

fix(NcRichContenteditable): make autocomplete accessible

  • Set aria-* attributes to the contenteditable
  • Set aria-activedescendant to connect with the current selected element from autocomplete
  • Make the list of options listbox + option instead of native list + listitem
    • list doesn't work as a list of options for screen reader

fix(NcRichContenteditable): provide focus-visible class

  • For further work - allows to set styles for keyboard navigation, emulating :focus-visible

🖼️ Screenshots

🏚️ Before 🏡 After
Contenteditable: .
image image
Options list .
Ignored image
image image

🏁 Checklist

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

@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress 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 Dec 1, 2023
@ShGKme ShGKme added this to the 8.2.1 milestone Dec 1, 2023
@ShGKme ShGKme self-assigned this Dec 1, 2023
@susnux susnux modified the milestones: 8.2.1, 8.3.1 Dec 1, 2023
@ShGKme ShGKme modified the milestones: 8.3.1, 8.4.1 Dec 27, 2023
@susnux susnux modified the milestones: 8.4.1, 8.5.0 Jan 15, 2024
@Pytal Pytal modified the milestones: 8.5.0, 8.6.0 Jan 23, 2024
@Antreesy Antreesy modified the milestones: 8.6.0, 8.6.1 Jan 30, 2024
@ShGKme ShGKme modified the milestones: 8.6.1, 8.6.2 Feb 1, 2024
@ShGKme ShGKme force-pushed the fix/nc-rich-contenteditable--a11y-tribute branch from 23fdbae to 16d225f Compare February 2, 2024 20:40
- Fixes `Tribute was already bound to DIV` warning
- Tribute is supposed to be only once on the element
- Simplifies further work with Tribute

Signed-off-by: Grigorii K. Shartsev <[email protected]>
- `smilesList` is always a constant and reused across all the instances
- tribute options are used only once to initialize tribute

Signed-off-by: Grigorii K. Shartsev <[email protected]>
- Correctly implement `debounced` method, so that debouncedFunction properties are available and each component instance has its own debounce
- Clear debounce on autoComplete close

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme force-pushed the fix/nc-rich-contenteditable--a11y-tribute branch from 98b6a12 to 5a90fc3 Compare February 5, 2024 12:16
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

tested and works with screen reader!

@ShGKme ShGKme force-pushed the fix/nc-rich-contenteditable--a11y-tribute branch 2 times, most recently from 8ac2eba to f91c871 Compare February 5, 2024 13:19
@ShGKme ShGKme requested review from Pytal and szaimen February 5, 2024 13:26
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 5, 2024
@ShGKme ShGKme marked this pull request as ready for review February 5, 2024 13:26
@szaimen szaimen removed their request for review February 5, 2024 13:27
@emoral435 emoral435 self-requested a review February 5, 2024 14:56
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Awesome work! 🚀

@ShGKme ShGKme force-pushed the fix/nc-rich-contenteditable--a11y-tribute branch from f91c871 to 66034a3 Compare February 5, 2024 19:36
Comment on lines +937 to +941
// Setup integration only once on the first open
if (this.isTributeIntegrationDone) {
return
}
this.isTributeIntegrationDone = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing added during the force push.

To set up integration only once after the first open, not each time.

@ShGKme ShGKme merged commit f2473fb into master Feb 5, 2024
18 checks passed
@ShGKme ShGKme deleted the fix/nc-rich-contenteditable--a11y-tribute branch February 5, 2024 19:51
@ShGKme
Copy link
Contributor Author

ShGKme commented Feb 5, 2024

Sorry for a fast merge, rushing to get this in the tomorrow's release. Feel free to review merged, I'll have a look.

ShGKme added a commit that referenced this pull request Feb 5, 2024
…editable--a11y-tribute

fix(NcRichContenteditable): make autocomplete accessible
ShGKme added a commit that referenced this pull request Feb 5, 2024
…editable--a11y-tribute

fix(NcRichContenteditable): make autocomplete accessible
@ShGKme ShGKme mentioned this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: rich-contenteditable Related to the rich-contenteditable components
Projects
None yet
5 participants