Skip to content

Fix custom select box hover styles #129970

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

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Fix custom select box hover styles #129970

merged 2 commits into from
Aug 4, 2021

Conversation

lezgomatt
Copy link
Contributor

@lezgomatt lezgomatt commented Aug 2, 2021

This PR fixes #130038.

Summary of changes on custom select boxes:

  • Consistent application of hover styles
    • Before: The hover FG would apply to focused options, but the hover BG would not
    • After: Neither FG or BG will be set, focused styles always win
  • Proper clearing of styles when hovering over disabled options
    • Before: Hover styles might not be cleared if some theme styles were unset, and the styles did not always match regular items
      • The background-color (not color) was set to the listActiveSelectionForeground (an active FG is probably a mistake too)
      • The background-color (again) would be set to selectBackground (which might not match the selectListBackground)
    • After: Hover styles are always cleared for disabled options, and should always match regular unselected items

Some other notes:

  • This is the only style with an outline of 1.6px, everything else seems to use 1px, not sure if this is intentional
  • I'm not actually sure if the regular hover styles are ever used since hovering seems to imply focus

@hediet
Copy link
Member

hediet commented Aug 3, 2021

Thanks for the PR!
Can you please open an issue to describe what problem this PR fixes (Including Gifs)?
Thank you!

@lezgomatt
Copy link
Contributor Author

Hi @hediet, created #130038 (with a video). Please let me know if there are any other issues. Thanks!

@roblourens roblourens self-assigned this Aug 3, 2021
@roblourens
Copy link
Member

Thanks! I am still a little confused about the styling of this component. I never see list.hoverBackground/Foreground take effect, only the quick input style? Do you happen to know why? And I actually don't see any different between hover/focus, shouldn't there still be hover-specific styles?

@lezgomatt
Copy link
Contributor Author

lezgomatt commented Aug 4, 2021

Thanks! I am still a little confused about the styling of this component. I never see list.hoverBackground/Foreground take effect, only the quick input style? Do you happen to know why? And I actually don't see any different between hover/focus, shouldn't there still be hover-specific styles?

Yes, if I'm not mistaken, after this fix, the hover styles never show up (except the one for the disabled option which clears the styles). (see EDIT below) The reason is because hovering over an option automatically causes it to be focused as well, which is not how the other UI elements work.

To fix this, an option should not automatically focus on hover, but only on selection via keyboard (to be consistent with the other UI elements). However, that is a bigger change, and may have implications which I do not fully understand.

Alternatively, I could let the hover style win over the focused style (by removing the :not(.focused) part), but this would not be consistent with how the other the other UI elements behave.

EDIT: After playing around with the select box, I did fine one scenario where the hover styles do apply: Try hovering over an option, and then switching the focus to another option using the keyboard arrows. The hovered element should now show the hover styles.

@roblourens
Copy link
Member

The reason is because hovering over an option automatically causes it to be focused as well, which is not how the other UI elements work.

Yeah, maybe we did that to make it look a little more like a typical dropdown which I guess wouldn't have two highlight colors.

EDIT: After playing around with the select box, I did fine one scenario where the hover styles do apply: Try hovering over an option, and then switching the focus to another option using the keyboard arrows. The hovered element should now show the hover styles.

I see that, thanks

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Thanks!

@roblourens roblourens added this to the August 2021 milestone Aug 4, 2021
@roblourens roblourens merged commit 44358e8 into microsoft:main Aug 4, 2021
@lezgomatt lezgomatt deleted the fix-custom-select-box-hover-styles branch August 5, 2021 14:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom select box hover style issues
3 participants