Buttons that have visible text should not use tooltips - New occurrences

[afercia]

Description

Previously: #42676 / #42815

This was already fixed for the buttons in the top bar in #42815 but it appears it's something that many contributors miss. I spotted a few new occurrences of this issue in newsly / refactored buttons.
It would be great to consider to automate this behavior in the components so that we don't have to worry about it again. Cc @ciampo

• Buttons that have visible text should not use tooltips to repeat the visible text.
• The only case where a tooltip should be used for buttons with visible text is when the button action is associated with a keyboard shortcut. So far, tooltips are the only place where the keyboard shortcut is visually exposed, besides the dedicated modal dialog. Although that's not ideal, it's the design convention used in the editor so far.

Step-by-step reproduction instructions

• Edit a post that is a draft.
• Use the keyboard to navigate the top bar buttons.
• Move focus to the 'Save' button.
• Observe the button does have visible text.
• Observe the button does show a tooltip that just repeats the visible text. Screenshot:

• Additionally: Observe the button doesn't have a focus style. Worth reminding we keep 'disabled' (actually aria-disabled) buttons focusable for discoverability and normally they show a ligher focus style. E.g. the 'disabled' Undo button. Screenshot:

• Move focus to the Preview button. Since in the default state it's an icon button, it's okay to show a tooltip. Screenshot:

• Go to Options > Preferences, and enable 'Show button text labels'.
• The Preview button now shows visible text instead of an icon.
• Move focus to the Preview button again.
• Observe the button shows a tooltip that just repeates the visible text. Screenshot:

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[afercia]

The 'Save/Saved' button has its own specific issues previously reported on #43952 and #44735

I will keep it out for now.

So far, we are forcinc the tooltip of this button to always render, to avoid a focus loss. This is very specific to this button, because rendering the tooltip conditionally based on the button state would trigger a full re-rendering of the DOM element and thus trigger a focus loss. It is still possible to reproduce the potential focus loss by removing this showTooltip prop and then saving a post edit by pressing the Save button with the keyboard.
As such, for now we have to keep forcing the tooltip on the Save button, even though it's not ideal.

Looks like the refactoring of the Tooltip with using ariakit didn't adress this issue. @ciampo @talldan could you please confirm when you have a chance?

[ciampo]

It would be great to consider to automate this behavior in the components so that we don't have to worry about it again

I'm not sure we can find a way for the Button component to reliably understand if its children contain visible text. I am also afraid that this change may introduce unexpected changes across the many usages of the component.
My preference would be to continue to rely on consumers of the component to correctly show tooltips when needed. I wonder if we can instead avoid such regressions with automated testing.

It is still possible to reproduce the potential focus loss by removing this showTooltip prop and then saving a post edit by pressing the Save button with the keyboard.
As such, for now we have to keep forcing the tooltip on the Save button, even though it's not ideal.
Looks like the refactoring of the Tooltip with using ariakit didn't adress this issue.

I gave this a quick look and the issue doesn't seem related to the Tooltip component anymore, but more to the fact that the Button component conditionally renders a component around {element} based on the shouldShowTooltip variable. The same reconciliation problem would happen if we rendered a div instead of the Tooltip component.

[afercia]

It would be great to consider to automate this behavior in the components so that we don't have to worry about it again

I'm not sure we can find a way for the Button component to reliably understand if its children contain visible text. I am also afraid that this change may introduce unexpected changes across the many usages of the component. My preference would be to continue to rely on consumers of the component to correctly show tooltips when needed. I wonder if we can instead avoid

@ciampo thanks for your feedback. I'm not sure I understand what the action plan is here, your comment seems truncated at the end :)

[ciampo]

@ciampo thanks for your feedback. I'm not sure I understand what the action plan is here, your comment seems truncated at the end :)

Woops, I must have completed the sentences only in my head 🙈 I updated my original comment, thank you for spotting it!