Add accessibility in video background settings component [WPB-24448]#21351
Add accessibility in video background settings component [WPB-24448]#21351zskhan wants to merge 1 commit into
Conversation
zskhan
requested review from
arjita-mitra,
e-maad,
otto-the-bot,
screendriver and
thisisamir98
as code owners
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
| return true; | ||
| }; | ||
|
|
||
| const getBackgroundEffectLabel = (effect: BackgroundEffectSelection, backgrounds: BuiltinBackground[]): string => { |
There was a problem hiding this comment.
BackgroundEffectSelection looks like a discriminated union. Could we make this mapping exhaustive? (pattern matching for example)
That would fail at compile time when a new effect type is added, rather than silently falling back.
| const background = backgrounds.find(({id}) => id === effect.backgroundId); | ||
| return background ? t(background.labelKey) : t('videoCallBackgroundVirtual'); | ||
| } | ||
| default: |
There was a problem hiding this comment.
Can we avoid this default branch and use an exhaustive check instead? The fallback may hide missing cases if the union grows.
| css={tileButtonStyles} | ||
| data-selected={selected} | ||
| aria-pressed={selected} | ||
| role="radio" |
There was a problem hiding this comment.
Since this element is role="radio" can we align the parent container to role="radiogroup" semantics?
| <h3 id={blurSectionId} css={sectionLabelStyles}> | ||
| {t('videoCallBackgroundBlurSectionLabel')} | ||
| </h3> | ||
| <div css={tileGridStyles} role="group" aria-labelledby={blurSectionId}> |
There was a problem hiding this comment.
This group contains radio options. Should this be role="radiogroup"?
| <h3 id={virtualSectionId} css={sectionLabelStyles}> | ||
| {t('videoCallBackgroundVirtualSectionLabel')} | ||
| </h3> | ||
| <div css={tileGridStyles} role="group" aria-labelledby={virtualSectionId}> |
There was a problem hiding this comment.
Same question as above: because children are role="radio" I think role="radiogroup" would be the semantically correct container role here.
| const virtualSectionId = useId(); | ||
| const closeButtonRef = useRef<HTMLButtonElement>(null); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Can we add a test for this autofocus behavior?
| return; | ||
| } | ||
|
|
||
| if (!isTabKey(event)) { |
There was a problem hiding this comment.
We now intercept only Tab but there is a test missing for this behavior change.
…ts and focus management
zskhan
force-pushed
the
feat/background-effect-settings-accessibility
branch
from
d2d4c31 to
8b051ea
Compare
|
3 participants
Add accessibility in video background settings component also improve focus management and few styling glitches