fix: move focus ring to button host to allow padding #3792
Conversation
CuddleBunny
requested review from
chrisdholt,
EisenbergEffect,
janechu and
nicholasrice
as code owners
| @@ -71,11 +72,10 @@ export const BaseButtonStyles = css` | |||
| background-color: ${neutralFillActiveBehavior.var}; | |||
| } | |||
|
|
|||
| .control:${focusVisible} { | |||
There was a problem hiding this comment.
Doh - I just realized this is going to break :focus-visible when that rolls in. :focus-visible is not out from behind a flag yet so I think this is okay for now but we should get other folk's opinion on that. @EisenbergEffect @chrisdholt
There was a problem hiding this comment.
Hmm. Assuming we had focus visible, what would be the alternative fix? If we can plan for forward compat, that's definitely preferred.
There was a problem hiding this comment.
Propose :focus-visible-within be added to the spec? 🙃 I can't think of a pure CSS way to keep :focus-visible. Does fast have a mechanism for forwarding styles to child elements?
There was a problem hiding this comment.
I can't either... You can forward styles to shadowed element has a part attribute, which button does.
There was a problem hiding this comment.
@nicholasrice @CuddleBunny :focus-visible is shipping in Chrome 86 which is currently in Beta: https://twitter.com/ChromiumDev/status/1301262894617640960
This will cover both Chrome and I'm checking on if it will also be in "Edgium" (h/t @CuddleBunny) for 86...
There was a problem hiding this comment.
Perhaps this should just be closed as a non-issue and documented that button padding can be controlled by targeting the control part? At this point it is down to developer experience vs accessibility. The latter is probably more important.
There was a problem hiding this comment.
@CuddleBunny Unfortunately I think that is the best course for now. Thanks for looking at this though!
EisenbergEffect
added
the
closed:not-actionable
Moved the focus ring to the button/anchor host to allow for padding. Resolves #3790
Issue type checklist
Is this a breaking change?
Adding or modifying component(s) in
@microsoft/fast-componentschecklistProcess & policy checklist