fitzee commented on code in PR #45471:
URL: https://github.com/apache/superset/pull/45471#discussion_r4321912631
##########
superset-frontend/src/features/versionHistory/SaveGroupItem.tsx:
##########
@@ -324,6 +312,7 @@ export default function SaveGroupItem({
return (
<Container
isHighlighted={isHighlighted}
+ aria-current={marksCurrentState ? 'true' : undefined}
Review Comment:
**non-blocking (a11y):** `aria-current` is set on the outer generic `<div>`,
but the element that gets focus is the inner `Header` (`role="button"`,
`aria-label={headline}`). When you Tab to an element, screen readers announce
that element's own name, role and states, not an ancestor's `aria-current`. So
keyboard users won't hear "current" on this row, which is what the PR
description promises. Groups with no records have no focusable element at all.
Put `aria-current` on `Header` when `hasRecords`, or add it to the accessible
name, e.g. `t('%s (current version)', headline)`, and update the Playwright
selector in `activity-log.spec.ts`.
##########
superset-frontend/src/features/versionHistory/SaveGroupItem.tsx:
##########
@@ -338,20 +327,11 @@ export default function SaveGroupItem({
aria-label={hasRecords ? headline : undefined}
hasRecords={hasRecords}
>
- <IconWrapper isCurrent={isCurrent}>
- {/* Semantic group icons per the design: the current (live)
- version is a check-circle, saved snapshots a save icon. */}
- {isCurrent ? (
- <Icons.CheckCircleOutlined iconSize="l" />
- ) : (
- <Icons.SaveOutlined iconSize="l" />
- )}
+ <IconWrapper>
+ <Icons.SaveOutlined iconSize="l" />
</IconWrapper>
<HeaderText>
- <HeadlineRow>
- <Headline title={headline}>{headline}</Headline>
- {currentTag}
- </HeadlineRow>
+ <Headline title={headline}>{headline}</Headline>
Review Comment:
**non-blocking:** With the tag gone, the live save looks like every other
dated row whenever the Current version section is shown (unsaved
Explore/dashboard edits, or a restore notice). It still behaves differently,
though. Its change rows (`ActionRow`) are still `role="button"` /
`tabIndex={0}`, and `previewIntent` sends them to `onExitPreview`. That
dispatches `clearVersionPreview`, which does nothing when no preview is open.
Its menu also leaves out Restore. So the user gets a focusable button that does
nothing and a different menu, with nothing on screen to explain either. The tag
used to explain it. In Explore with unsaved changes this matters even more: the
newest save is now the *last saved* state, not the current one, and a user is
likely to click it to see that state.
Options: a subdued text label such as `t('Latest save')` (not a Tag), a
tooltip on the rows, or making the live group's rows non-interactive (no
role/tabIndex) when no preview is open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
