Media plan storage indicator: add accessible tooltip

[frontdevde]

Changes proposed in this Pull Request

This PR adds an accessible tooltip to the plan storage indicator in /media.

See paYJgx-WR-p2 for use of reakit and a11y considerations.

Fixes #40986

I also noticed that the plan storage block doesn't have a focus style that matches the rest of the buttons in the media library. I changed it to now match the other blocks.

Testing instructions

• Using the calypso.live link below, navigate to /media
• Hover over the storage indicator to show the new tooltip
• Verify it works as expected (see GIF above)

[matticbot]

Test live: https://calypso.live/?branch=update/plan-storage-add-tooltip

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~310 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest      +1313 B  (+0.6%)     +310 B  (+0.7%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~33 bytes added 📈 [gzipped])

name        parsed_size           gzip_size
entry-main       +108 B  (+0.0%)      +33 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~36496 bytes added 📈 [gzipped])

name              parsed_size           gzip_size
media                +41427 B  (+9.8%)   +13055 B  (+11.3%)
post-editor          +41210 B  (+2.0%)   +11561 B   (+2.1%)
gutenberg-editor     +41210 B  (+4.6%)   +11540 B   (+4.7%)
checkout              +1412 B  (+0.1%)     -519 B   (-0.1%)
settings-writing       +118 B  (+0.0%)       +4 B   (+0.0%)
settings               +118 B  (+0.0%)       +5 B   (+0.0%)
themes                 +108 B  (+0.0%)     +881 B   (+0.8%)
devdocs                 -95 B  (-0.0%)       +4 B   (+0.0%)
domains                 +49 B  (+0.0%)      -35 B   (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~25016 bytes added 📈 [gzipped])

name                                parsed_size            gzip_size
async-load-post-editor-media-modal     +41268 B  (+14.0%)   +12121 B  (+15.1%)
async-load-design-blocks               +41210 B   (+1.4%)   +12045 B   (+1.7%)
async-load-design-playground              -44 B   (-0.0%)     +263 B   (+0.1%)
async-load-design                         -44 B   (-0.0%)     +587 B   (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[frontdevde]

As the reakit tooltip sits outside the flow at the end of the page, we need a z-index higher than the media library modal at 100200.

[obenland]

I'm not over the moon about introducing a new Tooltip component and not fixing/replacing the existing one, to be honest.

@Automattic/team-calypso Do you have thoughts/preferences on this? Is moving Tooltip away from Popover even an option at this point?

[mreishus]

I noticed that the ./client/ directory of calypso includes version 1.0.0-beta.14 of reakit, when 1.2.3 is the latest. It looks like commit 79cfa4d added the beta.14 version, which never got updated. Then a9fb3b8 removed the last use of reakit in ./client/, but didn't remove the dependency. This means the changes in this PR are the only use of reakit in ./client, so an upgrade to the reakit only has to consider the changes done in the PR to make sure they still work. I think this is a great opportunity to move off 1.0.0-beta.14 if we're still going with reakit.

(I also want to note that @wordpress/components, @wordpress/block-editor also depend on reakit, so it's not 100% new to calypso)

[griffbrad]

I have no issue with the Reakit use here overall. As @mreishus notes, it's already used heavily by @wordpress/components and will likely be used elsewhere in Calypso as well. In the context of Popover and Tooltip, it relies on popper.js (https://popper.js.org/), which provides much better, more sophisticated positioning logic than the current Calypso component. Discussion of using popper.js directly or via Reakit has recently resurfaced in Gutenberg:

WordPress/gutenberg#21275 (comment)

I don't want to over-complicate this PR by requiring that we sort all of this out fully before merging. However, the many !important declarations in the styles here seem like a code smell and would ultimately just have to be sorted out by anyone attempting to improve Popover or Tooltip as a whole when they encounter this overly-specific special-case styling and arrow positioning.

@diegohaz Is there a straightforward way to reuse elements of Reakit's positioning and ARIA support in the context of Calypso's existing components? Perhaps via state and/or props hooks?

[frontdevde]

I'm not over the moon about introducing a new Tooltip component and not fixing/replacing the existing one, to be honest.

Agreed. Reading @jeryj's a11y considerations in paYJgx-WR-p2, a more holistic update to all Tooltips in Calypso definitely seems desirable. That said, it would probably be a lot more involved (if only for the amount of testing required to make sure we cover all use cases and don't break anything).

So in the interest of making progress on and solving the specific use case in #40986 the idea was to try reakit's Tooltip component as a proof of concept here first. As @mreishus and @griffbrad noted reakit is already used in core and "will likely be used elsewhere in Calypso as well".

I think this is a great opportunity to move off 1.0.0-beta.14 if we're still going with reakit.

Definitely! Just updated it to the latest stable version.

However, the many !important declarations in the styles here seem like a code smell and would ultimately just have to be sorted out by anyone attempting to improve Popover or Tooltip as a whole when they encounter this overly-specific special-case styling and arrow positioning.

Agreed, I removed them. FWIW I had used these styles to force the arrow positioning to the bottom right end of the reference element to reflect the design here #40986 (comment).

@diegohaz When trying the placement options on the tooltip arrow they weren't working as expected. What's the right way to get the arrow to stick to the bottom of the tooltip (plus when it flips on the top of the tooltip) and point to the end of the reference element as show in this design #40986 (comment)?

[diegohaz]

@frontdevde

The placement options on the tooltip arrow we not working as I expected them to. What's the right way to get the arrow to stick to the bottom of the tooltip (plus when it flips on the top of the tooltip) and point to the end of the reference element as show in this design #40986 (comment)?

The arrow component expects to receive the placement property returned by the useTooltipState hook so it can automatically calculate its position and rotate accordingly to the placement of the popover element. So, placement always refers to the popover position, not the arrow's.

The arrow element is always positioned at the center relatively to the reference element. And as far as I know, Popper.js doesn't give us an option to change this. But I think you can just set a margin-left on the arrow element to shift it.

Considering that the arrow width is 1em, the code below would position the arrow at the right edge of the tooltip. But you can play with this value to get the right position.

.tooltip__arrow {
  margin-left: calc(50% - 0.5em);
}

[frontdevde]

So, placement always refers to the popover position, not the arrow's. The arrow element is always positioned at the center relatively to the reference element.

I see! In that case the unexpected placements of the arrow when changing the placement to a different value than the reference element's placement was indeed the expected result. :) I would've thought a separate arrow placement would be a feature included in Popper.js.

No worries, moving it with CSS works well in that case then. Thank you, @diegohaz!

[obenland]

Nice work @frontdevde!

[a8ci18n]

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4277552

Thank you @frontdevde for including a screenshot in the description! This is really helpful for our translators.

[a8ci18n]

Translation for this Pull Request has now been finished.