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(picker): move tooltip outside of action menu's button #5077

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rubencarvalho
Copy link
Collaborator

@rubencarvalho rubencarvalho commented Feb 7, 2025

Description

When a self-managed tooltip is added to an sp-picker (or sp-action-menu), clicking on the tooltip triggers the menu/dropdown to open but then immediately close. This happens because the tooltip was placed inside the button content, causing event propagation issues and CSS hover/active states on the button when interacting with the tooltip itself.

I moved the tooltip slot outside of the trigger button content which prevents event propagation issues and fixes CSS hover state problems by properly separating the tooltip from the button's content (it no longer is a direct child in the DOM).
Because of this change, I needed to override the selfManaged functionality by reassigning the triggerElement to be the button. This was necessary because the selfManaged logic traverses the DOM tree upward, meaning it would not find the correct focusable element to attach the trigger to with the new structure.

Related issue(s)

Motivation and context

Even though the reported bug aligns with our documentation, it is confirmed to create a poor user experience and unexpected results. Users don’t expect interacting with the tooltip to have the same effect as interacting with the button.
Additionally, if customers used the overlay-trigger alternative (without using the slot), the behavior would be different (but correct). With these changes, both approaches will now produce the same expected results.

How has this been tested?

1 - Visit the “before” (current behavior):
Before - Picker
Before - Action menu

2 - Visit the “after” (fixed behavior):
After - Picker
After - Action menu

  • Hover over the tooltip in both the versions.

  • In the fixed version, hovering over the tooltip should not trigger the button’s hover or active state.

  • Click on the tooltip in both versions:

  • Clicking the tooltip should not open the dropdown or trigger the button’s click behavior.

  • Sanity check:

  • Clicking directly on the action button (outside the tooltip area) still opens the dropdown as expected.

  • Did it pass in Desktop?

  • Did it pass in Mobile?

  • Did it pass in iPad?

Screenshot test regression seems expected to me - the tooltip content is no longer centered-aligned (before on the left, after on the right):

Screenshot 2025-03-11 at 13 00 39

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Copy link

changeset-bot bot commented Feb 7, 2025

⚠️ No Changeset found

Latest commit: c224990

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

github-actions bot commented Feb 7, 2025

Branch preview

Review the following VRT differences

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

Copy link

github-actions bot commented Feb 7, 2025

Tachometer results

Currently, no packages are changed by this PR...

Copy link

github-actions bot commented Feb 7, 2025

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.98 0.99
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 242.134 kB 229.405 kB 216.229 kB 🏆
Scripts 60.024 kB 54.205 kB 54.203 kB 🏆
Stylesheet 46.07 kB 40.718 kB 27.41 kB 🏆
Document 6.251 kB 5.526 kB 🏆 5.557 kB
Font 126.801 kB 126.605 kB 🏆 126.615 kB

Request Count

Category Latest Main Branch
Total 52 52 52
Scripts 41 41 41
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

@coveralls
Copy link
Collaborator

coveralls commented Feb 7, 2025

Pull Request Test Coverage Report for Build 13901636140

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.002%) to 97.979%

Totals Coverage Status
Change from base Build 13900661645: -0.002%
Covered Lines: 33672
Relevant Lines: 34170

💛 - Coveralls

@rubencarvalho rubencarvalho changed the title chore: move tooltip outside of button content chore: move tooltip outside of action menu's button Mar 6, 2025
@rubencarvalho rubencarvalho changed the title chore: move tooltip outside of action menu's button fix(action-menu): move tooltip outside of action menu's button Mar 6, 2025
@rubencarvalho rubencarvalho changed the title fix(action-menu): move tooltip outside of action menu's button fix(picker): move tooltip outside of action menu's button Mar 11, 2025
@rubencarvalho rubencarvalho marked this pull request as ready for review March 11, 2025 12:02
@rubencarvalho rubencarvalho requested a review from a team as a code owner March 11, 2025 12:02
@Rajdeepc
Copy link
Contributor

Can we add a storybook and a test to validate this case?

@rubencarvalho
Copy link
Collaborator Author

1 - Visit the “before” (current behavior): Before - Picker Before - Action menu

2 - Visit the “after” (fixed behavior): After - Picker After - Action menu

  • Hover over the tooltip in both the versions.

There isn’t a “new test case" per se. The tooltip’s behavior and look and feel are unchanged - except that it no longer affects the button trigger. The existing tooltip stories already cover this:


1 - Visit the “before” (current behavior):
Before - Picker
Before - Action menu

2 - Visit the “after” (fixed behavior):
After - Picker
After - Action menu

Hover over the tooltip in both the versions.


Let me know if you have something else in mind!

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.

[Bug]: the tooltip within sp-action-menu, which is clickable but it won't be keep the menu on
4 participants