-
Notifications
You must be signed in to change notification settings - Fork 100
SPARK-8: Fix Modal Focus Issue when using Keyboard #1963
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
Conversation
🦋 Changeset detectedLatest commit: bc2c361 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dancormier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is straightforward and refined, the tests are thorough, and after testing it in Firefox I can report that it works as expected.
Great work here @mukunku! Happy to see this merged in when you're feeling ready.
| test: /\.less$/, | ||
| use: [ | ||
| MiniCssExtractPlugin.loader, | ||
| isProd ? MiniCssExtractPlugin.loader : "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the commit message for this was to "fix webpack.config.js for windows". I'm curious what was happening initially (for the record, this looks good. I'm just personally curious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When loading the design site locally none of the css would load with the following error:
Refused to apply style from 'http://localhost:8080/xxxxxx' because its MIME type ('text/html') is not a supported stylesheet MIME type, and strict MIME checking is enabled.
Giamir gave me the solution to disable the css plugin locally as he encountered the same issue when assisting Tavian.
I believe it may be related to this as the root cause: webpack/webpack-dev-server#3168 (comment)
* SPARK-8: Fix Modal Focus Issue when using Keyboard (#1963) * fix reported focus issue * Also handle case where modal has no tabbable elements * fix webpack.config.js for windows * move if check up * Create loud-suits-stare.md * fix whitespace * revert html changes * revert harder * add tests * lint * chore(new-release) (#1964) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * [SPARK-6] - update the notice border color for dark mode and high contrast dark mode (#1965) * Correct readme * Don't run minicssextractplugin when running locally * Revert changes * update border colour for dark mode and high contrast dark mode * Create tasty-masks-pump.md * Minor notice border style refactor * Update visual regression test images --------- Co-authored-by: Dan Cormier <[email protected]> * chore(new-release) (#1966) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * build(deps-dev): bump webpack from 5.99.9 to 5.101.2 (#1967) Bumps [webpack](https://github.com/webpack/webpack) from 5.99.9 to 5.101.2. - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.99.9...v5.101.2) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.101.2 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * SPARK-12: Move to monorepo setup by splitting Stacks Docs and Classic into NPM workspaces (#1969) * mention why we are doing this as part of project SHINE * move folders * update paths * fix docs site and visual tests script * fix a11y and unit test scripts and tweak visual regression threshold * fix workflow tests * revert ci import paths to fix tests * try older image * fix visual ci tests? * mark stacks-docs not publishable * adjust adr based on pr feedback * adjust names * address PR feedback * add missing workspace param * switch back to old script * fix netlify and a11y * fix path * try fix workflows * fix harder * PR feedback * pr feedback round 2 * move some more packages around * Create eighty-pillows-relate.md * chore(screenshots): adjust gitignore patterns excluding failed images (#1972) * chore(monorepo): follow-up tweaks post monorepo changes (#1974) * chore(git-lfs): ensure visual test baseline images are tracked by git lfs (#1978) * Fix aria-current attribute placement in sidebar widget navigation examples (#1970) * chore(new-release) (#1979) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * build(deps-dev): bump vite from 7.1.3 to 7.1.5 (#1980) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 7.1.3 to 7.1.5. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v7.1.5/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 7.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: svc_tooling <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tavian Taylor <[email protected]> Co-authored-by: Dan Cormier <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giamir Buoncristiani <[email protected]> Co-authored-by: qheaden-stack <[email protected]>
* SPARK-8: Fix Modal Focus Issue when using Keyboard (#1963) * fix reported focus issue * Also handle case where modal has no tabbable elements * fix webpack.config.js for windows * move if check up * Create loud-suits-stare.md * fix whitespace * revert html changes * revert harder * add tests * lint * chore(new-release) (#1964) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * [SPARK-6] - update the notice border color for dark mode and high contrast dark mode (#1965) * Correct readme * Don't run minicssextractplugin when running locally * Revert changes * update border colour for dark mode and high contrast dark mode * Create tasty-masks-pump.md * Minor notice border style refactor * Update visual regression test images --------- Co-authored-by: Dan Cormier <[email protected]> * chore(new-release) (#1966) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * build(deps-dev): bump webpack from 5.99.9 to 5.101.2 (#1967) Bumps [webpack](https://github.com/webpack/webpack) from 5.99.9 to 5.101.2. - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.99.9...v5.101.2) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.101.2 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * SPARK-12: Move to monorepo setup by splitting Stacks Docs and Classic into NPM workspaces (#1969) * mention why we are doing this as part of project SHINE * move folders * update paths * fix docs site and visual tests script * fix a11y and unit test scripts and tweak visual regression threshold * fix workflow tests * revert ci import paths to fix tests * try older image * fix visual ci tests? * mark stacks-docs not publishable * adjust adr based on pr feedback * adjust names * address PR feedback * add missing workspace param * switch back to old script * fix netlify and a11y * fix path * try fix workflows * fix harder * PR feedback * pr feedback round 2 * move some more packages around * Create eighty-pillows-relate.md * chore(screenshots): adjust gitignore patterns excluding failed images (#1972) * chore(monorepo): follow-up tweaks post monorepo changes (#1974) * chore(git-lfs): ensure visual test baseline images are tracked by git lfs (#1978) * Fix aria-current attribute placement in sidebar widget navigation examples (#1970) * chore(new-release) (#1979) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * build(deps-dev): bump vite from 7.1.3 to 7.1.5 (#1980) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 7.1.3 to 7.1.5. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v7.1.5/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 7.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): add readme for each workspace (#1982) * add readme's for each package * Create six-buckets-grab.md * adjust stacks classic readme a bit * don't publish changeset for private package stacks-docs (#1983) * chore(new-release) (#1985) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: svc_tooling <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tavian Taylor <[email protected]> Co-authored-by: Dan Cormier <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giamir Buoncristiani <[email protected]> Co-authored-by: qheaden-stack <[email protected]>
Summary
This is for SPARK-8.
This is an issue with Firefox. If the modal doesn't have
tabindexset, when anything not tabbable within the modal is selected/focused, like raw text, and a key is pressed the event target will always be thebodyof the page. This causes a bug with thekeepFocusWithinModalevent handler where no matter what key you press the first tabbable element gets focused within the modal if you're not already focused on a tabbable element, deselecting any text you may have highlighted.Solution
The easiest solution is to only handle
Tabkey presses in this handler; not sure if there's a reason to handle any other kind of key presses. This addresses the original reported issue.Additionally, while unlikely, I also added code to handle the case where the modal has no tabbable items; including the close button in the top-right. Without this additional code, if you have a modal with no
buttons and pressTabfocus will go through elements in the main page, behind the modal. With the code, theTabkeypress becomes a no-op if there's nothing to switch focus to.How to Test
To re-create the issue:
https://deploy-preview-1963--stacks.netlify.app/product/components/modals/#examples
To test the fix:
Taband notice nothing changesTabkey presses will mess with focus changes now.