-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wb-1797] Address cell color contrast a11y issues #2435
base: main
Are you sure you want to change the base?
Changes from all commits
1538fcf
f2e611a
20fc419
5149ed0
7b6efb0
142f546
20d256d
0d21574
a66356b
e9b73bc
ce980f3
bebbdf4
27c2941
962fe64
6d0b1d5
dc57eec
8cd75b8
072e006
c3e0408
e3bc9b8
f5291ee
01a6add
31a2073
018a3b9
862e3c3
44f5ae0
2d6d5ef
da9b159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/wonder-blocks-tokens": minor | ||
--- | ||
|
||
Adds `fadedOffBlack72` color primitive token and sets the `semanticColor.text.secondary` token to this primitive. The slightly darker gray has better color contrast on a variety of backgrounds, including the fadedBlue8 background |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
--- | ||
"@khanacademy/wonder-blocks-cell": patch | ||
--- | ||
|
||
DetailCell and CompactCell: update styling to address accessibility issues (color contrast and using color as the only visual indicator). Updated styles include: | ||
|
||
- General: | ||
- Changing the grey used for subtitles | ||
- Using `icon.primary` for the right accessory | ||
- Press state: | ||
- Changing the background to `fadedBlue8` | ||
- Adding a thin left border when clickable cells are pressed | ||
- Hover state: | ||
- Changing the background to `fadedBlue8` | ||
- Disabled state: | ||
- Changing the focus outline to `action.disabled.default` | ||
- Selected state (cells with `active=true`): | ||
- Adding a thick left border | ||
- Changing the text color to `activeBlue` | ||
- The styling no longer changes when a selected cell is hovered or pressed on |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
import * as React from "react"; | ||
import {StyleSheet} from "aphrodite"; | ||
import type {Meta, StoryObj} from "@storybook/react"; | ||
|
||
import {PropsFor, View} from "@khanacademy/wonder-blocks-core"; | ||
import {spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
import {CompactCell} from "@khanacademy/wonder-blocks-cell"; | ||
import {PhosphorIcon} from "@khanacademy/wonder-blocks-icon"; | ||
import {IconMappings} from "../wonder-blocks-icon/phosphor-icon.argtypes"; | ||
import {LabelSmall} from "@khanacademy/wonder-blocks-typography"; | ||
|
||
/** | ||
* The following stories are used to generate the pseudo states for the | ||
* CompactCell component. This is only used for visual testing in Chromatic. | ||
*/ | ||
export default { | ||
title: "Packages / Cell / CompactCell / All Variants", | ||
parameters: { | ||
docs: { | ||
autodocs: false, | ||
}, | ||
backgrounds: { | ||
default: "offWhite", | ||
}, | ||
}, | ||
} as Meta; | ||
|
||
type StoryComponentType = StoryObj<typeof CompactCell>; | ||
|
||
const states = [ | ||
{ | ||
label: "Default", | ||
props: {}, | ||
}, | ||
{ | ||
label: "Disabled", | ||
props: {disabled: true}, | ||
}, | ||
{ | ||
label: "Selected using active: true", | ||
props: {active: true}, | ||
}, | ||
]; | ||
|
||
const defaultProps = { | ||
title: "Title for article item", | ||
leftAccessory: ( | ||
<PhosphorIcon icon={IconMappings.playCircle} size="medium" /> | ||
), | ||
rightAccessory: <PhosphorIcon icon={IconMappings.caretRight} />, | ||
}; | ||
|
||
const States = (props: {label: string} & PropsFor<typeof CompactCell>) => { | ||
return ( | ||
<View> | ||
<View style={[styles.scenarios]}> | ||
{states.map((scenario) => { | ||
return ( | ||
<View style={styles.scenario} key={scenario.label}> | ||
<LabelSmall> | ||
{props.label} ({scenario.label}) | ||
</LabelSmall> | ||
<CompactCell {...props} {...scenario.props} /> | ||
</View> | ||
); | ||
})} | ||
</View> | ||
</View> | ||
); | ||
}; | ||
|
||
const AllVariants = () => ( | ||
<View style={{gap: spacing.large_24}}> | ||
<States label="Default" {...defaultProps} /> | ||
<States label="Clickable" {...defaultProps} onClick={() => {}} /> | ||
<States label="Link" {...defaultProps} href="/" /> | ||
</View> | ||
); | ||
|
||
export const Default: StoryComponentType = { | ||
render: AllVariants, | ||
}; | ||
|
||
export const Hover: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {hover: true}}, | ||
}; | ||
|
||
export const Focus: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {focusVisible: true}}, | ||
}; | ||
|
||
export const HoverFocus: StoryComponentType = { | ||
name: "Hover + Focus", | ||
render: AllVariants, | ||
parameters: {pseudo: {hover: true, focusVisible: true}}, | ||
}; | ||
|
||
export const Active: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {active: true}}, | ||
}; | ||
|
||
const styles = StyleSheet.create({ | ||
statesContainer: { | ||
padding: spacing.medium_16, | ||
}, | ||
scenarios: { | ||
display: "flex", | ||
flexDirection: "row", | ||
alignItems: "center", | ||
gap: spacing.xxxLarge_64, | ||
flexWrap: "wrap", | ||
}, | ||
scenario: { | ||
gap: spacing.small_12, | ||
overflow: "hidden", | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,12 @@ import type {StyleType} from "@khanacademy/wonder-blocks-core"; | |
import Clickable from "@khanacademy/wonder-blocks-clickable"; | ||
import {View} from "@khanacademy/wonder-blocks-core"; | ||
import {Strut} from "@khanacademy/wonder-blocks-layout"; | ||
import {color, spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
import { | ||
border, | ||
color, | ||
semanticColor, | ||
spacing, | ||
} from "@khanacademy/wonder-blocks-tokens"; | ||
|
||
import {CellMeasurements, getHorizontalRuleStyles} from "./common"; | ||
|
||
|
@@ -118,7 +123,11 @@ function CellInner(props: CellCoreProps): React.ReactElement { | |
// custom styles | ||
style, | ||
horizontalRuleStyles, | ||
active && styles.activeInnerWrapper, | ||
]} | ||
// Set className so we can set styles on the inner wrapper directly | ||
// when the clickable element is pressed | ||
className="inner-wrapper" | ||
Comment on lines
+128
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: Thanks for adding this comment explaining the why we do this for this very specific case. |
||
> | ||
{/* Left accessory */} | ||
<LeftAccessory | ||
|
@@ -235,6 +244,12 @@ const styles = StyleSheet.create({ | |
padding: `${CellMeasurements.cellPadding.paddingVertical}px ${CellMeasurements.cellPadding.paddingHorizontal}px`, | ||
flexDirection: "row", | ||
flex: 1, | ||
borderRadius: "inherit", | ||
// Hide overflow so that if custom styling applies a border radius, the | ||
// left visual indicator for press/active states does not overflow | ||
overflow: "hidden", | ||
// Make sure inner wrapper is always the same height as parent | ||
height: "100%", | ||
|
||
// Reduce the padding of the innerWrapper when the focus ring is | ||
// visible. | ||
|
@@ -244,6 +259,18 @@ const styles = StyleSheet.create({ | |
}px`, | ||
}, | ||
}, | ||
activeInnerWrapper: { | ||
position: "relative", | ||
":before": { | ||
content: "''", | ||
position: "absolute", | ||
top: 0, | ||
left: 0, | ||
bottom: 0, | ||
width: border.width.thick, | ||
backgroundColor: semanticColor.surface.emphasis, | ||
}, | ||
}, | ||
|
||
content: { | ||
alignSelf: "center", | ||
|
@@ -264,7 +291,7 @@ const styles = StyleSheet.create({ | |
accessoryRight: { | ||
// The right accessory will have this color by default. Unless the | ||
// accessory element overrides that color internally. | ||
color: color.offBlack64, | ||
color: semanticColor.icon.primary, | ||
}, | ||
|
||
/** | ||
|
@@ -309,28 +336,52 @@ const styles = StyleSheet.create({ | |
border: `${spacing.xxxxSmall_2}px solid ${color.blue}`, | ||
borderRadius: spacing.xxxSmall_4, | ||
}, | ||
[":focus-visible[aria-disabled=true]:after" as any]: { | ||
borderColor: semanticColor.action.disabled.default, | ||
}, | ||
|
||
// hover + enabled | ||
[":hover[aria-disabled=false]" as any]: { | ||
background: color.offBlack8, | ||
background: color.fadedBlue8, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: You could probably use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll switch to using semantic tokens in a follow up PR and include this! Thanks for the suggestion! |
||
}, | ||
|
||
// pressed + enabled | ||
[":active[aria-disabled=false]" as any]: { | ||
background: color.offBlack16, | ||
background: color.fadedBlue8, | ||
}, | ||
// press + enabled + not currently selected (active prop: false) | ||
// We apply the left bar indicator styles on the inner-wrapper element | ||
// instead of the clickable element directly because we need to hide the | ||
// left bar overflow when custom cell styles apply a border-radius. We | ||
// have overflow: hidden on the inner wrapper instead of the clickable element | ||
// because setting it on the clickable element causes issues with existing | ||
// cases. | ||
[":active[aria-disabled=false]:not([aria-current=true]) .inner-wrapper" as any]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing #2435 (comment) : Discussed with @jandrade , we can use a class selector on the inner wrapper element directly (instead of using |
||
{ | ||
position: "relative", | ||
":before": { | ||
content: "''", | ||
position: "absolute", | ||
top: 0, | ||
left: 0, | ||
bottom: 0, | ||
width: border.width.thin, | ||
backgroundColor: semanticColor.surface.emphasis, | ||
}, | ||
}, | ||
}, | ||
|
||
active: { | ||
background: color.fadedBlue8, | ||
color: color.blue, | ||
color: color.activeBlue, | ||
cursor: "default", | ||
|
||
[":hover[aria-disabled=false]" as any]: { | ||
background: color.fadedBlue16, | ||
background: color.fadedBlue8, | ||
}, | ||
|
||
[":active[aria-disabled=false]" as any]: { | ||
background: color.fadedBlue24, | ||
background: color.fadedBlue8, | ||
}, | ||
}, | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed about DetailCell is that it is used by OptionItem. The updated styles aren't propagated to the OptionItems in the dropdown component because OptionItem overrides a lot of the styles. Is it expected that DetailCell and OptionItem have different styling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm good question... I would say that it's better to have different styling as
OptionItems
tend to be closer to "action" styles (like the button ones). My recommendation for now would be to keep it as it is for now. We could ask Design in case they want to revisit the styling for dropdowns if we want to be more consistent of course :).