Repository navigation
Fix contrast of keyboard focus indicator on card action buttons - #5879
Eugene (OEvgeny) wants to merge 4 commits into
Conversation
Card action buttons (`.ac-pushButton`, e.g. hero card buttons) had no focus style and relied on the browser default focus ring. Its dark part sits directly on the button, and the action set clips anything drawn outside of the button. On a pressed button (`#0063B1`), the ring is about 3:1 (2.76:1 as measured in Edge, reported in #5742). - On `:focus-visible`, draw a two-tone ring inside the button: an outer ring using the transcript keyboard indicator style options (2px solid black by default) on the edge of the button, and an inner 2px band in the pressed button text color (`cardPushButtonTextColor`, white by default) - Update JSDoc of `cardPushButtonTextColor` - Add html2 test for the default and Fluent themes - Update snapshots of existing tests with a focused card action button
There was a problem hiding this comment.
🟢 Approval recommended
Static review identified no blocking issues in the focused styling change or its regression coverage; tests were not executed during this review.
No open issues 🎉
What changed in this PR
Improves card action button focus visibility with an inset two-tone ring, addressing #5742 without clipping inside Adaptive Cards.
Changes:
- Adds focus styling using existing style options.
- Adds keyboard-focus regression tests for default and Fluent themes.
- Documents the focus-band color and accessibility fix.
| File | Description |
|---|---|
| packages/bundle/src/adaptiveCards/Attachment/AdaptiveCardRenderer.module.css | Adds the inset two-tone focus ring. |
| packages/bundle/src/adaptiveCards/AdaptiveCardsStyleOptions.ts | Documents the focus-band color option. |
| CHANGELOG.md | Records the contrast fix. |
| tests/html2/richCard/heroCard.pushButton.focusVisible.html | Checks focus styling before and after pressing. |
| tests/html2/richCard/heroCard.pushButton.focusVisible.fluent.html | Runs the regression test with Fluent styling. |
Review effort: Balanced
There was a problem hiding this comment.
Seems the new styling didn't help here much.
There was a problem hiding this comment.
These buttons are not pressed. The browser ring was already 2px near-black on white here (19:1), and the button border is also black, so the new 2px ring looks about the same. The fix is for colored buttons: on pressed blue, the old ring was 2.76:1. I tried a thicker ring inside the border, but it looked blurry at fractional display scaling, so the ring stays 2px on the edge, like our other buttons.
There was a problem hiding this comment.
This is thicker, black border in white label. It's inconsistent with the above blue border on active button. We should reconcile favoring this one.
Seems:
disableHeroCard.obsolete.html.snap-2 <— thinner, didn't modify HTML it seems
heroCard.pushButton.focusVisible.html <— new test, thicker, but no CSS override
There was a problem hiding this comment.
Agreed, this is the real style. The test HTML was not changed. The thinner look came from the shared test stylesheet
focusManagement.css, which overridesoutlinewith!important. It now skips keyboard-focused card buttons, so the 5 affected snapshots show this same ring.
…ylesheet The test stylesheet overrode the outline of every focused element with an !important dotted ring, which mixed with the card button focus ring and left only its inner band. Keyboard-focused card action buttons now show the real focus ring; mouse-focused ones keep the test marker.
| expect(document.activeElement).toBe(placesToBuyButton); | ||
| expect(placesToBuyButton.textContent).toBe('Places To Buy'); | ||
| expect(placesToBuyButton.matches(':focus-visible')).toBe(true); | ||
| expect(getFocusIndicatorStyle()).toEqual({ ...focusIndicatorStyle, backgroundColor: 'rgb(255, 255, 255)' }); |
There was a problem hiding this comment.
Maybe don't compare the CSS and let VRT do the work?
There was a problem hiding this comment.
Agreed: I removed the computed-style checks (outline, box-shadow, background color), so the test now only puts the button into each state (focused with Tab, then pressed with aria-pressed="true"), checks that it keeps the focus, and relies on the snapshots, which did not change.
Changelog Entry
Fixed
Description
Card action buttons (
.ac-pushButton, used for hero card buttons) had no focus style, so they showed the browser default focus ring. The dark part of that ring sits right on the button. On a pressed button (#0063B1), the contrast is about 3:1: the tester measured 2.76:1 in Edge. The ring cannot simply move outside the button, because the Adaptive Cards action set hasoverflow: hiddenand clips it. This PR draws a two-tone focus ring inside the button. The default theme and the Fluent theme are both fixed.Design
The ring is drawn inside the button: an outer ring on the edge of the button and an inner 2px band between the ring and the button fill.
cardPushButtonTextColor(white by default). This color is chosen to contrast withcardPushButtonBackgroundColor, so the band stays visible on a pressed button when these options are customized. Against#0063B1, the contrast is 6.14:1.The rule applies to all card action buttons, not only pressed ones. The focus ring keeps the same shape when a button changes between pressed and not pressed. Also, some unpressed buttons fail on main: a destructive button on hover (
#BF0000) with the browser ring is 2.91:1.The Copilot variant of the Fluent theme has its own, more specific focus style for card buttons, and it looks the same in normal mode (byte-identical screenshots). It still gets
outline-offset: -2pxfrom the new rule, which improves forced colors: its focus ring is now drawn inside the button, where before the action set clipped it.The legacy
styleSet.adaptiveCardRenderer(StyleSet/AdaptiveCardRenderer.ts) is unchanged because the renderer no longer uses it (it uses the CSS module).Question for maintainers: should the inner band use
currentColorinstead ofcardPushButtonTextColor? With a customcardPushButtonTextColor, unpressed buttons show the band in that color.Specific Changes
AdaptiveCardRenderer.module.css: added a:focus-visiblestyle to.ac-pushButton: an outline with negative offset and an insetbox-shadowbandAdaptiveCardsStyleOptions.ts: updated the JSDoc ofcardPushButtonTextColor, which now also colors the inner band of the focus indicatorAdded
__tests__/html2/richCard/heroCard.pushButton.focusVisible.html(and the.fluent.htmlvariant), which uses the same carousel as the mock bot: it focuses "Places To Buy" with the keyboard, presses it, checks that it keeps the focus, and relies on snapshots for the focus indicator__tests__/assets/focusManagement.css: the test stylesheet no longer overrides the focus style of keyboard-focused card action buttons with its!importantdotted outline and yellow fill, so the tests that use it show the real focus ringUpdated the snapshots of 8 existing tests that show a focused card action button (only the focus ring changed)
I have added tests and executed them locally
I have updated
CHANGELOG.mdI have updated documentation
Review Checklist
z-index)package.jsonandpackage-lock.jsonreviewed