Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not set tabIndex when no popover will show #6851

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

benrucker
Copy link
Contributor

@benrucker benrucker commented Jun 18, 2024

Fixes no ticket

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

This PR makes Popovers not add a tabIndex to the target when the Popover will not render due to disabled={true} or content={undefined}. This has been done by making sure that we do not override tabIndex on the popover target in those scenarios even if openOnTargetFocus is also true.

This is useful when rendering shared components that have conditional Tooltips implemented via the content or disabled props. Before this change, those components would always be focusable.

This PR does not address Popovers that are rendered in control mode and not open, though they exhibit similar behavior. This could be a follow-up change if desired.

Popover -> Tooltip nesting

This PR is consistent with previous behavior regarding nesting a Tooltip in a Popover.

For context, default Popovers do not apply a tabIndex=0 to their targets. This is because Popovers are by default PopoverInteractionKind.CLICK, which does not meet the pre-requisite for getting tabIndex=0 applied to the target of PopoverInteractionKind.HOVER.

Previously, a Popover with a nested Tooltip would always have a focusable target. Now, that will only be true if the Tooltip is both not disabled and has content.

This behavior has been codified in the test suite.

Reviewers should focus on:

  • If the tests make sense
    • Is sharing the button reference brittle? Should I extract the props instead, or make it a getButton function of some sort?
  • Does this need a docs update? A further comment on the openOnTargetFocus TSDoc?

After

⚠️ Note that I've added new lines here for demonstration purposes to exhibit the new behavior. No changes to the docs are being proposed in this PR.

Screen.Recording.2024-06-18.at.11.30.09.AM.mov

Lines that are no longer focusable:

  • Tooltip with no content
  • Disabled Tooltip
  • Popover with a nested disabled Tooltip
  • Disabled Popover with a nested disabled Tooltip

Lines that were never focusable:

  • Popover (not disabled)

Lines that are notably still focusable:

  • Popover with nested Tooltip
  • Disabled Popover with a nested Tooltip

@svc-palantir-github
Copy link

Make popover targets not tabble when disabled

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@benrucker benrucker force-pushed the br/no-tab-index-when-popover-disabled branch 3 times, most recently from c26173f to ced45fd Compare June 18, 2024 18:58
@benrucker benrucker marked this pull request as ready for review June 18, 2024 18:59
@svc-palantir-github
Copy link

Make popover targets not tabble when disabled

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Copy link
Contributor

@evansjohnson evansjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

This is looking good, I've encountered this issue in the wild and believe I've done annoying things like optionally wrap Tooltip when not disabled to avoid this. Let's just get the empty check cleaned up to support the trim case then I think this should be ready to merge.


for good measure I also tested

disabling tooltip (popover) shouldn't override already focusable target element

<Tooltip
    disabled={true}
    content="tooltip!"
    intent="danger"
    placement="right"
>
    <Button onClick={() => console.log("hello world")}>I should still be focusable</Button>
</Tooltip>

keeps existing tabIndex

<Tooltip
    disabled={true}
    content="tooltip!"
    intent="danger"
    placement="right"
>
    <div tabIndex={0}>target text, not sure why anyone would define tabIndex here but...</div>
</Tooltip>

Screenshot 2024-06-26 at 3 48 34 PM

and in the renderTarget case the consumer is fully responsible for rendering anyways - if they accept our "recommended" tabIndex, they'll get this new behavior, otherwise they can always do whatever they want.

@@ -375,7 +375,8 @@ export class Popover<
onKeyDown: this.handleKeyDown,
};
// Ensure target is focusable if relevant prop enabled
const targetTabIndex = openOnTargetFocus && isHoverInteractionKind ? 0 : undefined;
const targetTabIndex =
content != null && !disabled && openOnTargetFocus && isHoverInteractionKind ? 0 : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you split out and reuse this check?
const isContentEmpty = content == null || (typeof content === "string" && content.trim() === "");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay! I pulled that into a private class method. Let me know if you envisioned a different way to share it between renderTarget and render. Thanks for the review!

@evansjohnson evansjohnson self-assigned this Jun 26, 2024
@svc-palantir-github
Copy link

Move content emptiness check to private class function

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson
evansjohnson previously approved these changes Jun 28, 2024
Copy link
Contributor

@evansjohnson evansjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the fix!

@benrucker benrucker force-pushed the br/no-tab-index-when-popover-disabled branch from 10e6464 to 52db8bc Compare June 28, 2024 18:36
@policy-bot policy-bot bot dismissed evansjohnson’s stale review June 28, 2024 18:36

Invalidated by push of 52db8bc

@benrucker benrucker force-pushed the br/no-tab-index-when-popover-disabled branch from 52db8bc to bbdc761 Compare June 28, 2024 18:37
@benrucker benrucker force-pushed the br/no-tab-index-when-popover-disabled branch from bbdc761 to 0b53277 Compare June 28, 2024 18:39
@svc-palantir-github
Copy link

Move content emptiness check to private class function

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@evansjohnson evansjohnson merged commit 9ee655b into develop Jun 28, 2024
14 checks passed
@evansjohnson evansjohnson deleted the br/no-tab-index-when-popover-disabled branch June 28, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants