Save post button: avoid extra re-renders when enablng/disabling tooltip #56502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Type] Bug
|
Size Change: +87 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ciampo
commented
Nov 24, 2023
There was a problem hiding this comment.
Reviewing the diff with the "Hide whitespace" option enabled can be quite helpful here
ciampo
commented
Nov 24, 2023
| variant="tertiary" | ||
| icon={ isLargeViewport ? undefined : cloudUpload } | ||
| // Make sure the aria-label has always a value, as the default `text` is undefined on small screens. | ||
| label={ text || label } |
There was a problem hiding this comment.
the label prop has two functions in button:
- it serves as an aria-label, if the aria-label prop isn't explicitly set
- it's used as tooltip text
Swapping label for aria-label allows us to keep the button correctly labelled, without triggering the internal Button's tooltip.
The same string of text is also being passed explicitly to the Tooltip component via the text prop.
ciampo
commented
Nov 24, 2023
| * We want the tooltip to show the keyboard shortcut only when the | ||
| * button does something, i.e. when it's not disabled. | ||
| */ | ||
| shortcut={ isDisabled ? undefined : displayShortcut.primary( 's' ) } |
There was a problem hiding this comment.
This prop is now passed to the Tooltip component instead
|
I think that we could merge this PR anyway, while we discuss whether the approach in #56490 is viable. |
afercia
reviewed
Nov 24, 2023
afercia
reviewed
Nov 24, 2023
That's correct. A more general fix is being discussed in #56490, although I'm not sure how easy it's going to be to find a solid solution that doesn't introduce regressions. This PR offers a fix specifically to the Save button, and shows an alternative workaround to setting the
Since the button is currently working as expected thanks to having the Regardless, I think that it would be better for #44735 to become an actual GH issue, rather than a discussion, |
tyxla
approved these changes
Nov 24, 2023
| } ) | ||
| : undefined | ||
| } | ||
| onClick={ isDisabled ? undefined : () => savePost() } |
There was a problem hiding this comment.
Just noticed we can shorten this to:
Suggested change
| onClick={ isDisabled ? undefined : () => savePost() } | |
| onClick={ isDisabled ? undefined : savePost } |
Yes, there should be a "Create issue from discussion" link in the sidebar of the discussion page:
|
5 tasks
ramonjd
reviewed
Nov 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #44735
Alternative approach to #56490
This PR makes changes to how the tooltip is rendered for the post save button, thus fixing an issue that was causing loss of keyboard focus on that button.
Why?
This change is necessary to fix the issue described in #44735, where basically the element rendered by
Buttonwould be removed and re-added to the DOM any time theButtoncomponent switches from having to show a tooltip to not having to show it, and vice-versa.The fact that a new HTML element is re-created every time triggers a focus loss that can be very annoying, especially to assistive technology users.
How?
The approach is similar to what proposed in #56490 — although the fix is applied only for this specific instance of
Button, instead of applying it directly to the button component (since that could have broader consequences).The issue is related to the fact that React can't reconciliate the "with tooltip" and "without tooltip" version of the
Buttoncomponent without re-creating the whole button every time.To solve this, I decided to tweak the code so that:
Buttoncomponent never renders a tooltip (thanks to removing theshowTooltipandshortcutprops, and changing thelabelprop toaria-label)Buttoncomponent is always wrapped in aTooltipcomponent. The component receives a different set of props depending on whether a tooltip should be effectively shown.Note: this doesn't mean that the tooltip will always be shown, though — the original user-facing behaviour should have been retained by only passing props to the
Tooltipcomponent when needed.Testing Instructions
From #44735 (comment):
Screenshots or screencast
Kapture.2023-11-24.at.12.53.04.mp4