clay
clay copied to clipboard
liferay
feat(@clayui/css): Convert to use focus-visible pattern
#4967
@matuzalemsteles :focus-visible is working for me in Chrome, FF, MS Edge. The only problem we have is with IE11 and older Safari browsers (I don't think we have any official mandate to support this). I only tested on Safari 14.1.2. The regular focus works the same as focus-visible. It doesn't show the focus ring on click; only when you interact with the keyboard.
The way this works is we keep :focus the same and use the @supports selector(:focus-visible) {} media query to undo the focus ring and re-apply it on :focus-visible. I think this is the simplest way. I still need to update this pr, but I didn't want to take too long putting this up for review.
oh this is very interesting @pat270 so the focus ring would be controlled by the browser if I'm getting this right? It sure makes things a lot simpler and a lot easier that we may not need to have to update in DXP.
I think we don't need to worry about IE support. For Safari it seems a bit strange the behavior, it shows the focus ring when you press and hold, unlike other browsers, I tested it in 15.2 but it seems that it was fully supported in Safari version 15.4 according to MDN,
Thanks @matuzalemsteles, yes it will be controlled by the browser. We only need to figure out how to on input elements. We can also deprecate and disable c-inner by default now.
@matuzalemsteles I disabled c-inner by default. I think I covered all the button and anchor tag cases, hopefully I didn't break anything.
@pat270 I think the problem with disabling c-inner by default is that we need to remove all cases from c-inner, this breaks the elements now that it's disabled. I think we can control this within Clay but I'm not sure we can do that in DXP as well. This can also represent a breaking change, I wanted to avoid that initially.
Also, cc @drakonux is one of the solutions we found to implement this easily using the browser's native selector, the difference is that this is not configurable by the developers and is controlled by the browser itself.
@matuzalemsteles What component broke when we removed c-inner? I thought I checked most places and it seemed fine.
@pat270 I only saw it in TreeView, the documentation site also broke because we use c-inner but this is controlled.
cc @ethib137 and @marcoscv-work, maybe you can help us with that to make a decision about it here.
Basically this is related to issue #3342, ideally the idea was for this to be configurable but making it configurable brings implementations on the JS side and that we would need to implement focus features and other mechanisms as was initially done in #4972 but this implementation uses the feature native to the browser and that makes things simpler but is not configurable and will apply that default immediately.
This is a tough one. We already have inconsistencies on how this is applied in portal (toggle panel buttons in control panel header), which I personally find confusing. As a technical user I often switch between clicking somewhere and then navigating with keyboard. When the focus state is not easily displayed, I'm unsure of what pressing enter or tab will do. This slows me down, since I'll usually press tab then shift+tab in order to figure out where the focus is then press enter or space to interact with the item.
It was mentioned before that this should likely be a product decision and I think I agree. I'm curious also if we have done any UX testing for this to understand what our users would prefer?
@ethib137 actually the ring will appear when pressing Enter or Tab, that's the change here, the ring doesn't appear when just the action is done with the click. The problem of leaving this to a product decision that can generate inconsistency, where a team decides to keep this behavior for clicks and another does not, this can generate more problems for the user. In fact, from what I remember, the use of the ring was added only for keyboard navigation, the difference is that we always add it for clicks as well.
About UX testing I'm not sure if it's already been done, @drakonux and @emiliano-cicero can talk more about it.
Speaking from another perspective, looking at the component community and design systems, they are also moving in this direction the ring only appears when you have keyboard navigation.
Just to clarify my point, the fact that the ring only appears with keyboard and not when clicking is what I find disorienting. For instance in the attached gif, I can't tell that focus is on the first item of the dropdown when I open the menu by clicking on the trigger. It's only after using arrow keys to move that I'm then aware that pressing enter would cause me to "Add an Event".
I definitely agree, we should make sure we apply this consistently one way or another. By making it a product decision, I was thinking that we should have the PEDS PM weigh in on this change. I'll post in Slack to have Mateo consider this.
It is very helpful to be aware of the fact that other component libraries are applying this behavior. I noticed that Gmail also applies this in a number of situations.
Yeah, makes sense to me @ethib137! Yes, there are some ways that I noticed that other libraries/systems do to show the user that the button has "focus" when the menu is open. In addition to showing hover on items or when focus has moved to the first item, it adds another different state to the button, usually active being a darker color.