Skip to content

Conversation

@bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jul 25, 2025

Description 📝

This PR makes some fixes related to disabled options in Autocompletes:

  • Fixes issue where disabled Autocomplete options do not look like they are disabled
    • In this PR, disabled options now look disabled
  • Fixes issue where disabled Autocomplete option tooltips do not open when you focus the option with the keyboard
    • In this PR, you will now see the tooltip when you focus the disabled option with your keyboard
  • Fixes issue where aria-disabled is set to false on disabled Autocomplete options
    • In this PR, aria-disabled should now be set to true for disabled options

Preview 📷

Region Select

Before After
Screenshot 2025-07-25 at 2 43 28 PM Screenshot 2025-07-25 at 2 43 54 PM
Screenshot 2025-07-25 at 2 48 59 PM Screenshot 2025-07-25 at 2 48 31 PM

Placement Group Select

Before After
Screenshot 2025-07-25 at 2 52 45 PM Screenshot 2025-07-25 at 2 53 19 PM

How to test 🧪

  • Test the Region Select component on the Linode Create flow and in other places
    • Verify disabled regions show a tooltip explaining why
      • Verify you can trigger the tooltip using your keyboard
    • Verify disabled options appear visibly disabled
  • Test the Placement Group Affinity Type Select in the Placement Group Create drawer
    • Verify disabled options show a tooltip explaining why
      • Verify you can trigger the tooltip using your keyboard
    • Verify disabled options appear visibly disabled
  • Test other autocompletes that feature disabled options
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All tests and CI checks are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@bnussman-akamai bnussman-akamai self-assigned this Jul 25, 2025
@bnussman-akamai bnussman-akamai added Bug Fixes for regressions or bugs UX/UI Changes for UI/UX to review labels Jul 25, 2025
@bnussman-akamai bnussman-akamai marked this pull request as ready for review July 25, 2025 19:28
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner July 25, 2025 19:28
@bnussman-akamai bnussman-akamai requested review from mjac0bs and pmakode-akamai and removed request for a team July 25, 2025 19:28
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Great find on the keyboard accessibility.

Saw this unit test failure locally, as well, and looks like a new issue, though I'm not exactly sure what in this PR caused it:

Screenshot 2025-07-25 at 3 35 26 PM

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 690 passing tests on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing690 Passing4 Skipped113m 47s

@mjac0bs mjac0bs self-requested a review July 29, 2025 18:22
@bnussman-akamai
Copy link
Member Author

@mjac0bs I think I got the scroll / tooltip overflow behavior somewhat better. It should now be similar to how production works. Not perfect, but as good as I can get it...

@bnussman-akamai
Copy link
Member Author

bnussman-akamai commented Jul 30, 2025

@mjac0bs Nevermind, that broke tooltip interactivity.

I don't really have a sound solution for preventing tooltip overflow...

I think I'll need some help here, or we can merge this because I think this PR makes a net improvement, and we can look to iterate on the tooltip's behavior

@mjac0bs
Copy link
Contributor

mjac0bs commented Jul 30, 2025

Sorry that tooltip's been a handful! I'll take a look again with the latest, but given that it's somewhat of an existing issue made slightly more prevalent, I'm okay with the changes you made and doing further follow up on tooltip behavior too.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

I got deep in the weeds of the Popover's modifier prop before creating a ticket M3-10400.

Popper.js is based on a "plugin-like" architecture, most of its features are fully encapsulated "modifiers". A modifier is a function that is called each time Popper.js needs to compute the position of the popper. For this reason, modifiers should be very performant to avoid bottlenecks. To learn how to create a modifier, read the modifiers documentation.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Jul 30, 2025
Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

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

ux question (non-blocking): Not sure if this is a UX issue or something for a follow-up ticket, but I noticed that when the cursor is already over the dropdown and you start using the keyboard, the focus/highlight jumps around based on the cursor position, which feels a bit confusing. It works fine when the cursor is outside the list

Screen.Recording.2025-07-31.at.6.47.47.PM.mov

@jaalah-akamai
Copy link
Contributor

ux question (non-blocking): Not sure if this is a UX issue or something for a follow-up ticket, but I noticed that when the cursor is already over the dropdown and you start using the keyboard, the focus/highlight jumps around based on the cursor position, which feels a bit confusing. It works fine when the cursor is outside the list

That may be something we're doing with CSS, the MUI Autocomplete does not do this. I'll create a ticket - thanks @pmakode-akamai

@github-project-automation github-project-automation bot moved this from Review to Approved in Cloud Manager Jul 31, 2025
@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jul 31, 2025
@bnussman-akamai
Copy link
Member Author

Going to merge so that disabled states work again. Hopefully we can dial in focus and tooltip behavior in the future. (M3-10400)

@bnussman-akamai bnussman-akamai merged commit 0eea9b0 into linode:develop Jul 31, 2025
34 of 35 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Merged in Cloud Manager Jul 31, 2025
bnussman-akamai added a commit that referenced this pull request Jul 31, 2025
bnussman-akamai added a commit that referenced this pull request Jul 31, 2025
I removed autoHighlight #12583 and I believe it broke a ton of cypress tests (thanks for catching @jdamore-linode )

Co-authored-by: Banks Nussman <banks@nussman.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs UX/UI Changes for UI/UX to review

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants