Skip to content

fix(tooltip): ensure esc closes tooltip, avoid bubbling beyond the body#10220

Merged
tay1orjones merged 2 commits into
carbon-design-system:mainfrom
tay1orjones:10204-tooltip-dismissal
Dec 3, 2021
Merged

fix(tooltip): ensure esc closes tooltip, avoid bubbling beyond the body#10220
tay1orjones merged 2 commits into
carbon-design-system:mainfrom
tay1orjones:10204-tooltip-dismissal

Conversation

@tay1orjones

@tay1orjones tay1orjones commented Dec 3, 2021

Copy link
Copy Markdown
Member

Closes #10204

Changelog

New

  • test(tooltip): add test to cover "esc to close" functionality

Changed

  • fix(tooltip): remove global/document esc keyDown handler, replace it with scoped onKeyDown handler on tooltip body
  • docs(tooltip): remove interactive elements from the tooltip body in the default story
  • docs(modal): add a tooltip to the WithStateManager story to more easily test tooltips inside an element with it's own "esc to close" handler.

Testing / Reviewing

I'm sorry this has quite a few steps to fully test the changes here:

  • In the carbon-components-react storybook deploy preview:
    • Ensure tooltip without interactive body contents can be closed via esc
    • Ensure tooltip with interactive body contents can be close via esc
    • Ensure Modal WithStateManager tooltip can be closed via esc and does not close the modal. A second press of esc should close the modal.
  • Pull this down to test locally - we need to ensure the same steps from above work w/ React v16
    • Modify the react deps in the root package.json to be 16.8.6 (here is a reference from the PR updating it to v17)
    • Modify the react deps in the packages/react/package.json to be 16.8.6
    • yarn install at the root
    • yarn start the packages/react storybook
    • You can validate the storybook is using React v16 by adding a console.log(React.version); (anywhere, in a component, etc) - it should output to the console v16.8.6
    • Redo the tests that you did on the deploy preview:
      • Ensure tooltip without interactive body contents can be closed via esc
      • Ensure tooltip with interactive body contents can be close via esc
      • Ensure Modal WithStateManager tooltip can be closed via esc and does not close the modal. A second press of esc should close the modal.

@tay1orjones tay1orjones requested a review from joshblack December 3, 2021 20:00
@tay1orjones tay1orjones requested a review from a team as a code owner December 3, 2021 20:00
@tay1orjones tay1orjones requested a review from abbeyhrt December 3, 2021 20:00
@netlify

netlify Bot commented Dec 3, 2021

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 7c1e5b8

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61aa7f71f0ae1500078c664a

😎 Browse the preview: https://deploy-preview-10220--carbon-react-next.netlify.app

@tay1orjones

Copy link
Copy Markdown
Member Author

@amortiz @lji00 If you have time, I would appreciate your eyes on this to double check we've hit the mark on fixing this one.

@netlify

netlify Bot commented Dec 3, 2021

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 7c1e5b8

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61aa7f714816860008826d59

😎 Browse the preview: https://deploy-preview-10220--carbon-elements.netlify.app

@netlify

netlify Bot commented Dec 3, 2021

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 7c1e5b8

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61aa7f718953b50008c572c8

😎 Browse the preview: https://deploy-preview-10220--carbon-components-react.netlify.app/

@amortiz

amortiz commented Dec 3, 2021

Copy link
Copy Markdown

@tay1orjones not sure if i'm testing correctly. I went here:
https://deploy-preview-10220--carbon-components-react.netlify.app/?path=/story/components-tooltip--render-custom-icon

So i can click on the tooltip, press ESC when focus is on link/button, and it closes correctly. Once it merges and a new version is available i can test in my code sandbox links i created to confirm on both react 16 and 17

thanks

@tay1orjones

Copy link
Copy Markdown
Member Author

@amortiz thanks! Yeah that's perfect. The storybook is running with React v17 right now.

@tay1orjones tay1orjones merged commit 5b691a1 into carbon-design-system:main Dec 3, 2021
tay1orjones added a commit to tay1orjones/carbon that referenced this pull request Dec 3, 2021
@tay1orjones

Copy link
Copy Markdown
Member Author

This has been published in v10.49.1.

Here's two identical sandboxes demonstrating the fix with tooltip esc working as intended with both React v16 and React v17:

React 16: https://codesandbox.io/s/carbon-tooltip-escape-delegation-react16-l1ih9
React 17: https://codesandbox.io/s/carbon-tooltip-escape-delegation-react17-pydv4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tooltip with focusable element is not accessible on React 17

4 participants