🐛 [BUG] - Button focus animation

[primer]

🐛 [BUG] - Button focus animation

[TylerJDev]

Hey @rezrah, thank you for the extra context and added video!
I dug into this issue a bit, and I was able to reproduce this in both 0.12.1 and 0.12.0. I'd like to note that the effect of this bug is much more noticeable in 0.12.1 and I was able to find out why.

From what I was able to find, this is caused by our transition set alongside our box-shadow, in addition to our added [disabled] selector for the :focus class:

.Button:focus:not(.Button--disabled[disabled]) {
  outline: none;
  box-shadow: 0 0 0 2px var(--brand-color-canvas-default), 0 0 0 5px var(--brand-color-focus);
}

As shown in the video, our box-shadow moves from beneath the component before it fully borders the button itself.
The recent release made this effect instantaneous, meaning that you only need to press and hold the button for the transition to play.

This appears to be because in previous releases, the applied :hover effect took priority over our :focus effect, as both pseudo classes contained their own box-shadow rule. This meant that the border around the button wouldn't show unless you hovered outside of the button due to the :hover box-shadow being applied when hovered.

In the current update, the applied :focus effect now takes priority over the :hover effect. This means that as long as you're focused on the button, the box-shadow for :focus will overrule the :hover box-shadow. This stems from the added [disabled] in the following selector:

.Button:focus:not(.Button--disabled[disabled])

The [disabled] selector is only applied for :focus currently, and I believe that this is why it's taking priority over :hover now. Potentially because it's a more specific selector vs .Button--primary:not(.Button--disabled):hover 🤔 ?

If we want parity with the previous release 0.12.0, we can add [disabled] to our :hover rules, which will make them priority again over :focus (e.g. .Button--primary:not(.Button--disabled[disabled]):hover). This reverts us back to how it was in our previous release, but the underlying issue will still exist.

An alternative fix would be either of the following:

• Swap from box-shadow to outline
  We could mirror the styling of box-shadow with outline and this would also allow our :hover box-shadow to always have priority in future updates. This brings the added benefit of no longer needing to remake our border in WHCM.
• Add [disabled] to :hover rules
  This does bring us back to how it was in previous releases, but the transition animation issue will still be present, just not as noticeable.
• Add transition: none to :focus
  The following code example would also fix this issue:

.Button:focus:not(.Button--disabled[disabled]) {
  outline: none;
  transition: none; /* Added property */
  box-shadow: ... ;
}

This would mean that the more general transition rule in .Button wouldn't apply for :focus and should prevent the transition animation issue that's shown in the video.

[rezrah]

Hey @TylerJDev, thanks for doing the triage on this. Really appreciate the detailed report also 🙏.

Looking at the available options, here are my thoughts:

Swap from box-shadow to outline

I don't think this is feasible as Safari has a bug with border-radius and outline, which forced us to switch to box-shadow. See the comment here where it was identified. It was fixed in the same PR.

Add [disabled] to :hover rules

From your comment, this puts us back to pre-a11y remediation? Perhaps a last resort.

Add transition: none to :focus

I haven't seen it in action, but am I correct in assuming this has no negative consequences and is your preferred option? If so, can we put this up in Storybook and do some feature branch testing?

[TylerJDev]

No problem! Thanks as well 😁!

I did not know about the outline issue, that is good to know.
In that case I would agree with the transition: none being the best approach, with Add [disabled] to :hover rules being the last choice.

I'm creating up a draft PR that we can hopefully test this out on and see out it will look. If it's good we can probably push this change out as the fix.