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

fix: Require less cursor precision when moving from tooltip target to tooltip content #3359

Merged
merged 3 commits into from
Apr 9, 2025

Conversation

avinashbot
Copy link
Member

@avinashbot avinashbot commented Mar 19, 2025

Description

For accessibility reasons, you should be allowed to move your cursor from the tooltip trigger to the tooltip itself. And you can't do that in the slider. And you can also do that in other components, but you have to gently maneuver your cursor from the trigger through the arrow and into the tooltip, but that feels needlessly precise. So we create an extension to the tooltip's bounding box that extends the full height/width of the tooltip but covers the empty space next to the arrow.

Slider needed a little more attention because it the mouseenter/mouseleave handlers were placed on an element too far away from where the tooltip was actually being rendered. So I just moved those handlers out to a parent div (new div because I guess the intention is to not make the logic cover the step labels).

Related links, issue #, if available: AWSUI-60211, AWSUI-60222

How has this been tested?

Manually. There should be no visual or behavioral changes, only the sizes of the mouseover/mouseout triggers.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.50%. Comparing base (3d8e33f) to head (d16c085).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/breadcrumb-group/item/item.tsx 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3359      +/-   ##
==========================================
+ Coverage   96.46%   96.50%   +0.03%     
==========================================
  Files         805      804       -1     
  Lines       23029    23004      -25     
  Branches     7969     7543     -426     
==========================================
- Hits        22215    22199      -16     
- Misses        760      798      +38     
+ Partials       54        7      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avinashbot avinashbot marked this pull request as ready for review March 26, 2025 07:07
@avinashbot avinashbot requested a review from a team as a code owner March 26, 2025 07:07
@avinashbot avinashbot requested review from jperals and removed request for a team March 26, 2025 07:07
&::before {
inset-block: 0;
inset-inline-start: -$arrow-height;
block-size: $arrow-height;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like block-size should be inline-size in this and the next rule set.

I've added some styles with the dev tools to debug this pseudoelement, and it only has the expected position and dimensions when the popover/tooltip is anchored to the top or bottom, not left or right.

Screenshot 2025-03-26 at 08 24 16
Screenshot 2025-03-26 at 08 24 57

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, whoops, good find - I messed up the logical properties. Fixed and manually tested more cases, so new approval request coming soon.

@avinashbot avinashbot force-pushed the tooltip-hover-target branch 3 times, most recently from 4c28834 to 4d878ab Compare April 8, 2025 13:58
})}
style={{
[customCssProps.sliderTooltipPosition]: `calc(${percent}% - ${thumbSize}px)`,
onMouseEnter={() => {
Copy link
Member

Choose a reason for hiding this comment

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

There is now a sort of intermediate state in the slider where the tooltip is already open but the handle is not shown in hover state. This did not occur before —tooltip appearing and handle in hover state would always happen at the same time.

slider-tooltip-hover.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

That intermediate state is what this PR explicitly enables - the whole idea is to allow the cursor to move off the slider handle and then into the tooltip much easier. I don't think it should show hover styling when the mouse is no longer on the handle, so this means that intermediate state has to exist.

@avinashbot avinashbot added this pull request to the merge queue Apr 9, 2025
Merged via the queue into main with commit 8fe8792 Apr 9, 2025
38 checks passed
@avinashbot avinashbot deleted the tooltip-hover-target branch April 9, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants