Tooltip2: hide the tooltip from AT when it is not open

[broccolinisoup]

Closes https://github.com/github/primer/issues/2822

Changelog

New

Changed

Initially we were rendering the tooltip element visually hidden even when it is not opened but we received a feedback from the sign-off review that the tooltip should be hidden from screen readers when it is not visible for visual users too.
In the meantime, the popover-polyfill released a major change that injects the styles with JS, therefore no need to tweak anything in the CSS itself. This PR;

1. Removes the visually hidden styles and leverages the default popover stylesheet styles that has display: none already.
2. Removes the unnecessary CSS polyfill as they are injected via JS.

As always, shout out to @keithamus for his guidance here 🎉
Before

Before	After

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• Integration tests pass at github/github (Learn more about how to run integration tests)

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 1fcb31a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.02 KB (-0.04% 🔽)
dist/browser.umd.js	104.56 KB (-0.06% 🔽)

[keithamus]

I'm going to mark this as request changes because I want to keep a close eye on these changes before they ship.

I think now we're using @oddbird/[email protected] - which injects stylesheets in JS - we can remove a lot of the CSS and the polyfill will do the correct accessible thing.

I think the correct diff for this PR would look like removing the popoverStyles const, and related polyfill code, and I don't think we'll need to add any additional CSS.

[broccolinisoup]

This was needed for firefox. Otherwise the styles wasn't applied due to [popover] having more specificity than the styled component's class name.

[broccolinisoup]

This trick caused a bit of a flashing though.

Rewatch.Screen.Recording.-.2023-11-07.at.12.15.mp4

Video description: There is a "Delete" button and the page is reloaded. The tooltip text without any styles appears right under the button very briefly like flashing then disappears

And if I load the style component (the tooltip element itself) with display: 'none' and then add display: 'block' when it is open, the flash is gone (commit for changes) but I am not sure if this is a good way to solve this. Let me know if you have any a way of solving this already or any other thoughts!

[keithamus]

This looks reasonable for now. The polyfill injecting CSS means that we suffer from a FOUC, which is also true for dotcom today. I'm looking into ways to minimise or remove the FOUC but given it's only Firefox, and Firefox will soon be shipping native popovers, it's not a blocking concern.

[siddharthkp]

And if I load the style component (the tooltip element itself) with display: 'none' and then add display: 'block' when it is open

I kinda like this as a temporary measure but also if the flash only happens in firefox, we can choose to ignore it. No strong feelings, up to you

[broccolinisoup]

Just to clarify, the fix that needed for firefox was adding the [popover] attribute selector to the class styling but this addition caused flashing in all browsers. So I guess it is better to keep it until Firefox catches up.

[broccolinisoup]

I'm going to mark this as request changes because I want to keep a close eye on these changes before they ship.

I think now we're using @oddbird/[email protected] - which injects stylesheets in JS - we can remove a lot of the CSS and the polyfill will do the correct accessible thing.

I think the correct diff for this PR would look like removing the popoverStyles const, and related polyfill code, and I don't think we'll need to add any additional CSS.

Thanks for the review @keithamus! I really appreciate it. I pushed another commit based on your suggestions, and everything worked (with just a tiny adjustment) as you described 🎉

[keithamus]

This now looks great!

[broccolinisoup]

@siddharthkp Could I get your approval please if it looks good? Need primer-react approval to merge 👀

[siddharthkp]

Ship it!

[siddharthkp]

@siddharthkp Could I get your approval please if it looks good? Need primer-react approval to merge 👀

Oh, my bad! Didn't realise that.

[patrickhlauke]

👍