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

Add live regions logic #687

Merged
merged 8 commits into from
Aug 6, 2024
Merged

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jul 19, 2024

Description

  • Adds logic that inserts ARIA live assertive and polite regions to an application's document body during KDS initialization and documents this on the new "Installation" page
  • Relatedly adds useKLiveRegion composable with public methods for updating the live regions with assertive and polite messages.

See "Comments" section below on why this strategy, after all.

Issue addressed

  • Closes Implement KLiveRegion and useKLiveRegion #668
    • Note that the interface is slightly different than I originally proposed. KDS takes care of inserting the live regions to DOM instead of having to have KLiveRegion. This makes things more robust and easier in consuming apps, and prepares ground for root teleport element.

Changelog

  • [Add live regions logic #687]
    • Description: Adds logic that inserts ARIA live assertive and polite regions to an application's document body during KDS initialization and documents this on the new "Installation" page. Relatedly adds useKLiveRegion composable with public methods for updating the live regions with assertive and polite messages.
    • Products impact: new API
    • Addresses: Implement KLiveRegion and useKLiveRegion #668
    • Components: useKLiveRegion
    • Breaking: No
    • Impacts a11y: Yes. It will fix several places utilizing live regions that don't work in our applications at all. Furthemore, it follows the recommended practices that will fix major a11y issues with live regions we're having.
    • Guidance: Find all polite and live regions (or roles) in an application. Remove them and instead use useKLiveRegion.sendPoliteMessage and useKLiveRegion.sendAssertiveMessage to update the live regions that KDS inserted to document body during installation.

Steps to test

Implementation notes

At a high level, how did you implement this?

  • In KThemePlugin.js insert live region elements to document.body via plain JavaScript when DOM is loaded
  • Publicly expose useKLiveRegion.sendPoliteMessage and useKLiveRegion.sendAssertiveMessage to update the region elements

Does this introduce any tech-debt items?

Opposite - after we use it more widely, it will fix several places utilizing live regions that don't work in our applications at all. Furthermore, it follows the recommended practices that will fix major a11y issues with live regions we're having.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

I explored several approaches regarding inserting live region elements and their many combinations, some of the more important ones:

  • Mounting KLiveRegion.vue as a Vue component

    • Makes setup complex by requiring attaching to the root Vue app element rather than simply to document.body
    • Its {{ message }} changes not being announced properly when compared to using plain JS to update elements' textContent
  • Adding mounted hook to KThemePlugin's mixin, where we check if this.$root === this and if so, we insert the live regions to DOM at this point

  • Adjusting Vue prototype's $mount and some other places in a way similar to above

    • same issues as above
  • Exposing mountLiveRegions publicly and instructing apps to call it in the root app mounted

    • even though this mitigated the issue with many mounted conditions being executed, it really didn't help with the live regions not being ready in time when announcing very shortly after Kolibri loaded

Note that @rtibbles also mentioned some other Vue prototype places that we may want to experiment with. However, at the end of the day, since we need to update DOM as soon as possible, after seeing above, I can hardly imagine something faster than utilizing document.readyState and DOMContentLoaded event.

The original reason for not using this strategy was my wish to attach KLiveRegion as Vue component as well as a Nuxt problem. However, those can be overcome easily and I think reliability and robustness in consumer apps is the most important criteria here.

@MisRob MisRob force-pushed the live-region branch 4 times, most recently from 7d6c8cf to 55328aa Compare July 20, 2024 05:05
lib/KThemePlugin.js Outdated Show resolved Hide resolved
@MisRob MisRob force-pushed the live-region branch 17 times, most recently from d1344de to 69c25f5 Compare July 23, 2024 18:07
@MisRob MisRob changed the title [WIP] Live regions Add live regions logic Jul 23, 2024
@MisRob MisRob requested review from AlexVelezLl and rtibbles July 23, 2024 19:43
@MisRob MisRob added the TODO: needs review Waiting for review label Jul 23, 2024
@MisRob MisRob marked this pull request as ready for review July 23, 2024 20:14
@MisRob
Copy link
Member Author

MisRob commented Jul 23, 2024

cc @BabyElias I hope you can use this from your table work after we get it merged and released. You're welcome to review too, all of it, or parts of it! Whatever work time-wise.

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks you @MisRob! This looks good to me! Just had a concern about what would happen if we directly mounted the live region in the KThemePlugin, and a couple of optional things I noticed.

docs/pages/installation.vue Outdated Show resolved Hide resolved
docs/pages/usekliveregion.vue Outdated Show resolved Hide resolved
docs/pages/usekliveregion.vue Outdated Show resolved Hide resolved
lib/KThemePlugin.js Show resolved Hide resolved
lib/KThemePlugin.js Show resolved Hide resolved
lib/composables/useKLiveRegion/__tests__/index.spec.js Outdated Show resolved Hide resolved
@MisRob MisRob requested a review from AlexVelezLl August 1, 2024 15:35
@MisRob
Copy link
Member Author

MisRob commented Aug 1, 2024

All feedback is resolved @AlexVelezLl, thank you

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @MisRob! :)

@MisRob MisRob merged commit 873e248 into learningequality:develop Aug 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement KLiveRegion and useKLiveRegion
3 participants