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: disable browser autocomplete and edit dropdown items elements #2513

Merged

Conversation

httpsmenahassan
Copy link
Contributor

@httpsmenahassan httpsmenahassan commented Aug 9, 2023

Add attributes to input element to disable browser autocomplete on autosuggest component and change the div that houses the dropdown items to be a ul, change the button elements that render the list to be li. add new role to each element

Description

Resolves #2435 and resolves #2557.

We had to make more changes than anticipated here given that we previously relied on the button value attribute. After changing button elements to li, we realized that li's value attribute is an integer value, which posed some problems.

Note that this doesn't fix the issue where if you try to tab out while in the dropdown, the menu is still displayed. (#2437)

Deploy Preview

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

Merge Checklist

  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • 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
Copy link

netlify bot commented Aug 9, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 79f0cb4
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/64df908974452f000869e515
😎 Deploy Preview https://deploy-preview-2513--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM, with a nit, once CI is passing.

src/Menu/index.scss Outdated Show resolved Hide resolved
@@ -283,8 +286,6 @@ FormAutosuggest.propTypes = {
ignoredArrowKeysNames: PropTypes.arrayOf(PropTypes.string),
/** Specifies loading state. */
isLoading: PropTypes.bool,
/** An ARIA role describing the form autosuggest. */
role: PropTypes.string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this during the Paragon Working Group. We made the decision to remove consumers' ability to change the role to something that would break WCAG.

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

:shipit:

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4d4577e) 91.70% compared to head (79f0cb4) 91.70%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2513   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files         236      236           
  Lines        4219     4219           
  Branches     1021     1021           
=======================================
  Hits         3869     3869           
  Misses        346      346           
  Partials        4        4           
Files Changed Coverage Δ
src/Form/FormAutosuggestOption.jsx 100.00% <ø> (ø)
src/Form/FormAutosuggest.jsx 94.44% <100.00%> (ø)

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

@httpsmenahassan httpsmenahassan marked this pull request as draft August 15, 2023 19:44
@httpsmenahassan httpsmenahassan force-pushed the Form.AutosuggestInput branch 2 times, most recently from e2df596 to ffb32de Compare August 17, 2023 19:30
</div>
);
}

FormAutosuggest.defaultProps = {
arrowKeyNavigationSelector: 'a:not(:disabled),button:not(:disabled, .btn-icon),input:not(:disabled)',
arrowKeyNavigationSelector: 'a:not(:disabled),li:not(:disabled, .btn-icon),input:not(:disabled)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We realized that after disabling the browser autocomplete that changing button to li broke the keyboard navigation through the dropdown items but this was easily solved by changing this line to include li.

Add attributes to `input` element to disable browser autocomplete on autosuggest component and change the div that houses the dropdown items to be a `ul`, change the button elements that render the list to be `li`. add new `role` to each element
@brian-smith-tcril
Copy link
Contributor

@httpsmenahassan I know you've made changes (that, from what I can tell, look great!) since this PR was approved. This is good to go now right?

@httpsmenahassan
Copy link
Contributor Author

@brian-smith-tcril yes! this PR is all set on my end.

@brian-smith-tcril
Copy link
Contributor

@adamstankiewicz it sounds like this one should be 100% good to go!

@brian-smith-tcril brian-smith-tcril merged commit 93bf239 into openedx:master Aug 31, 2023
9 checks passed
@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 21.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@edx-semantic-release
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants