feat: add attributes to input and wrapper component #2570

httpsmenahassan · 2023-08-22T19:35:22Z

Description

Resolves #2438.

Create IDs for items in dropdown
Add aria-activedescendant attribute to input to identify to assistive technologies which item in the dropdown is being focused
Add aria-live=”assertive” to wrapper component to allow screen readers to read out the number of options in the dropdown

Deploy Preview

https://deploy-preview-2570--paragon-openedx.netlify.app/components/form/form-autosuggest/

Merge Checklist

Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2023-08-22T19:35:27Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`2d57bc9`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/650b481ac4c2b800088e8fc9
😎 Deploy Preview	https://deploy-preview-2570--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-08-23T01:02:55Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.43% 🎉

Comparison is base (0291845) 91.81% compared to head (2d57bc9) 92.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
+ Coverage   91.81%   92.25%   +0.43%     
==========================================
  Files         235      235              
  Lines        4217     4222       +5     
  Branches     1020     1020              
==========================================
+ Hits         3872     3895      +23     
+ Misses        341      323      -18     
  Partials        4        4

Files Changed	Coverage Δ
src/Form/FormAutosuggest.jsx	`93.87% <100.00%> (+0.32%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adamstankiewicz · 2023-08-23T15:39:54Z

src/Form/FormAutosuggest.jsx

      // eslint-disable-next-line no-shadow
      const { children, onClick, ...rest } = child.props;
+      // Generate a unique ID for each menu item
+      const menuItemId = `pgn__form-autosuggest__menuItem-${index}`;


Two questions here:

Duplicate IDs are possible here (e.g., if there are multiple FormAutosuggest components rendered at the same time in the UI with the same number of children. There might be an opportunity to be use name to differentiate between multiple FormAutosuggest components? That said, it doesn't appear to be a required field as is.

Is there a concern if/when the children change order between re-renders? In theory, the index would be different between re-renders on each item.

it seems on http://react-autosuggest.js.org/ the ids will update as the items in the dropdown change

As long as the id changes in all the appropriate places at the same time (which I'm assuming is the same; that is, it doesn't update the ID in one place and leave it unchanged somewhere else), I imagine the re-rendering/re-ordering of React.Children wouldn't be an issue. Just worth a sanity check :)

brian-smith-tcril · 2023-08-23T15:44:35Z

@adamstankiewicz just to verify/document decisions from Paragon WG today:

make name a required prop for autosuggest
ensure we generate unique ids (something like autoSuggestName-uuid) to set aria-activedescendant

adamstankiewicz · 2023-08-23T16:14:22Z

@brian-smith-tcril @httpsmenahassan

make name a required prop for autosuggest

I'd like to better understand how and why name is used today. It looks like it is only passed to a feedback-for attribute, but on a quick search, I'm struggling to find any documentation on if this is a standard attribute related to a11y. I haven't seen feedback-for used on the FormControlFeedback component before...

If we truly need it, then we'll need to consider making it a required prop. Ideally, though, the solution for generating unique IDs wouldn't rely on the prop as if it did, we'd need to make a breaking change to make the name (or otherwise) prop required.

ensure we generate unique ids (something like autoSuggestName-uuid) to set aria-activedescendant

If the ID is guaranteed to be unique based on a uuid, I don't think the name bit above is relevant? FWIW, we generate a UUID for use in React key attributes in some places to ensure uniqueness:

import { v4 as uuidv4 } from 'uuid';

We also use this newId function in Paragon (source) to generate unique IDs in some places.

brian-smith-tcril · 2023-08-23T16:16:49Z

@adamstankiewicz if we can avoid making name a required prop and instead just do the uuid thing that works perfectly imo

Create IDs for items in dropdown. Add aria-activedescendant attribute to input to identify to assistive technologies which item in the dropdown is being focused. Add aria-live=”assertive” to wrapper component to allow screen readers to read out the number of options in the dropdown.

brian-smith-tcril

Awesome!

edx-semantic-release · 2023-10-11T14:43:46Z

🎉 This PR is included in version 21.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

edx-semantic-release · 2023-10-11T15:05:15Z

🎉 This PR is included in version 22.0.0-alpha.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

httpsmenahassan force-pushed the attributesForm.Autosuggest branch from 3d11100 to 236afbc Compare August 23, 2023 00:40

adamstankiewicz reviewed Aug 23, 2023

View reviewed changes

httpsmenahassan force-pushed the attributesForm.Autosuggest branch 2 times, most recently from 9f7ac7e to f557bc6 Compare August 25, 2023 18:06

httpsmenahassan force-pushed the attributesForm.Autosuggest branch from f557bc6 to 4fd71b4 Compare September 20, 2023 01:56

httpsmenahassan changed the title ~~Attributes form.autosuggest~~ fix: disable browser autocomplete and edit dropdown items elements Sep 20, 2023

httpsmenahassan force-pushed the attributesForm.Autosuggest branch 2 times, most recently from 3114298 to e18a794 Compare September 20, 2023 14:40

httpsmenahassan changed the title ~~fix: disable browser autocomplete and edit dropdown items elements~~ feat: add attributes to input and wrapper component Sep 20, 2023

httpsmenahassan force-pushed the attributesForm.Autosuggest branch from e18a794 to 4471d66 Compare September 20, 2023 18:43

httpsmenahassan force-pushed the attributesForm.Autosuggest branch from 4471d66 to 2d57bc9 Compare September 20, 2023 19:29

httpsmenahassan marked this pull request as ready for review September 20, 2023 19:51

brian-smith-tcril approved these changes Sep 20, 2023

View reviewed changes

brian-smith-tcril merged commit acf452b into openedx:master Oct 11, 2023
8 checks passed

edx-semantic-release added the released label Oct 11, 2023

edx-semantic-release added the released on @alpha label Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add attributes to input and wrapper component #2570

feat: add attributes to input and wrapper component #2570

httpsmenahassan commented Aug 22, 2023 •

edited

Loading

netlify bot commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

adamstankiewicz Aug 23, 2023 •

edited

Loading

brian-smith-tcril Aug 23, 2023

adamstankiewicz Aug 23, 2023

brian-smith-tcril commented Aug 23, 2023

adamstankiewicz commented Aug 23, 2023

brian-smith-tcril commented Aug 23, 2023

brian-smith-tcril left a comment

edx-semantic-release commented Oct 11, 2023

edx-semantic-release commented Oct 11, 2023

feat: add attributes to input and wrapper component #2570

feat: add attributes to input and wrapper component #2570

Conversation

httpsmenahassan commented Aug 22, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented Aug 22, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

adamstankiewicz Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

brian-smith-tcril Aug 23, 2023

Choose a reason for hiding this comment

adamstankiewicz Aug 23, 2023

Choose a reason for hiding this comment

brian-smith-tcril commented Aug 23, 2023

adamstankiewicz commented Aug 23, 2023

brian-smith-tcril commented Aug 23, 2023

brian-smith-tcril left a comment

Choose a reason for hiding this comment

edx-semantic-release commented Oct 11, 2023

edx-semantic-release commented Oct 11, 2023

httpsmenahassan commented Aug 22, 2023 •

edited

Loading

netlify bot commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

adamstankiewicz Aug 23, 2023 •

edited

Loading