palantir / blueprint

A React-based UI toolkit for the web
https://blueprintjs.com/
Apache License 2.0
20.65k stars 2.17k forks source link

Do not set `tabIndex` when no popover will show #6851

Closed benrucker closed 3 months ago

benrucker commented 3 months ago

Fixes no ticket

Checklist

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:

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.

https://github.com/palantir/blueprint/assets/12519846/43efdbb4-da3d-45cd-bcd9-7f7aa4237e0d

Lines that are no longer focusable:

Lines that were never focusable:

Lines that are notably still focusable:

svc-palantir-github commented 3 months ago

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.

svc-palantir-github commented 3 months ago

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.

svc-palantir-github commented 3 months ago

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.

svc-palantir-github commented 3 months ago

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.