- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.4k
fix(select): remove item focus style when there are no selected items only at ionic mode #30750
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
Conversation
| The latest updates on your projects. Learn more about Vercel for GitHub. 
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things as well:
- Checks are failing. Please have those passing before asking for a review.
- This is causing a regression. Notice how there's a border radius when opening a select popover:
| next | ROU-12235 | 
|---|---|
| https://github.com/user-attachments/assets/a156e4da-2fbc-472b-9b33-b9aaa85db78c | https://github.com/user-attachments/assets/ff9d844d-ddbf-42b4-baf0-632ac8cf6053 | 
I haven't checked the other interfaces but I assume that they have the same issue. Please check.
- What's the ticket to add in the work for "This change will require an additional interaction to observe the focus behavior when navigating through keyboard, since tap-based navigation does not rely on focus styling."? Because the options are no longer showing the focus rings when navigating with keyboard.
        
          
                ...cordion.e2e.ts-snapshots/accordion-states-focused-ionic-md-ltr-light-Mobile-Chrome-linux.png
          
            Show resolved
            Hide resolved
        
              
          
                ...n.e2e.ts-snapshots/accordion-states-inset-focused-ionic-md-ltr-light-Mobile-Chrome-linux.png
          
            Show resolved
            Hide resolved
        
              
          
                ...est/states/item.e2e.ts-snapshots/item-states-diff-ionic-md-ltr-light-Mobile-Chrome-linux.png
          
            Show resolved
            Hide resolved
        
              
          
                ...ts-snapshots/toolbar-basic-nested-slotted-images-ionic-md-ltr-light-Mobile-Firefox-linux.png
          
            Show resolved
            Hide resolved
        
      | 
 Basically we must add this behaviour just when keyboard navigation is used to navigate through the elements, before this implementation it was defined on top of tap navigation which causes this visual issue, now focus style is missing when there are no pre-selected option when keyboard navigation is in use. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that we should add a test under select-modal to capture the focus states when an option is selected and when it's not.
        
          
                ...lays/datetime-button.e2e.ts-snapshots/datetime-overlay-modal-ios-ltr-Mobile-Safari-linux.png
          
            Show resolved
            Hide resolved
        
              
          
                ...bar.e2e.ts-snapshots/toolbar-basic-slotted-images-ionic-md-ltr-light-Mobile-Chrome-linux.png
          
            Show resolved
            Hide resolved
        
      | 
 @joselrio I understand that the behavior needs to be added for keyboard. What's the Jira ticket that implements it? | 
Co-authored-by: Maria Hutt <[email protected]>
| 
 I agree, but let's do it under the context of the task I mentioned before since focus should be specially tackled under that context ;) | 
| 
 Task do not exist yet, but it will be created since we already aligned with @jessicamendesOS and @gnbm about it. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue number: resolves #
This PR introduces improvements to the visual focus styling of Ionic items when inside a select modal.
What is the current behavior?
--background-focusedand--background-focused-opacitywere missing fromitem.ionic.scss, which resulted in the native default outline focus style being applied to focused items.What is the new behavior?
NOTE
Does this introduce a breaking change?
Other information