Skip to content

Rewrite focus ring logic in View #1940

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.

Sign up for GitHub

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

Merged
matyasf merged 1 commit into from
May 8, 2025
Merged

Rewrite focus ring logic in View #1940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions packages/ui-flex/src/Flex/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,25 +279,28 @@ type: example
### Handling overflow

When `direction` is set to `column`, Flex.Items' `overflowY` property is automagically set
to `auto` to account for content overflow with a vertical scrollbar.
to `auto` to account for content overflow with a vertical scrollbar. Add padding, so focus rings are not cut off.
Copy link
Collaborator Author

@matyasf matyasf Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we are doing this automagically, but I think it would be a breaking change to remove


> To override this default, simply set `overflowY` on the Flex.Item to either `visible` or `hidden`.
```js
---
type: example
---
<Flex
withVisualDebug
direction="column"
>
<Flex.Item padding="small">
<Heading>Pandas are cute, right?</Heading>
</Flex.Item>
<Flex.Item size="150px" padding="small">
<Img src={avatarSquare} />
</Flex.Item>
</Flex>
<Flex
withVisualDebug
direction="column"
>
<Flex.Item padding="small">
<Heading>Pandas are cute, right?</Heading>
</Flex.Item>
<Flex.Item>
<TextInput name="name" renderLabel="If you dont add padding, the focus ring will be cut off!" />
</Flex.Item>
<Flex.Item size="150px" padding="small">
<Img src={avatarSquare} />
</Flex.Item>
</Flex>
```
### A few common layouts
Expand Down
133 changes: 87 additions & 46 deletions packages/ui-view/src/View/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,6 @@ type: example
`position` sets the CSS position rule for the component: `static`, `absolute`, `relative`,
`sticky`, or `fixed`.

> Note that `position="sticky"` is currently [not as fully supported](https://caniuse.com/#feat=css-sticky)
> as the other values.
```js
---
type: example
Expand Down Expand Up @@ -428,25 +425,19 @@ type: example

By default, if a `View` is rendered as a focusable element, a focus outline will display when it is focused for accessibility.

> Note that `position` must be set to `relative` for the focus ring to display.
> (This restriction exists because the focus ring requires styling a pseudo element
> that has absolute positioning.)
```javascript
---
type: example
---
<View
position="relative"
tabIndex="0"
role="button"
cursor="pointer"
display="block"
margin="large"
padding="small"
>
Tab here to see the focus outline
</View>
<Flex gap="medium" direction="column">
<View tabIndex="0" role="button" cursor="pointer">
Tab here to see the focus outline
</View>
<View focusWithin>
if the <code>focusWithin</code> prop is <code>true</code>, the View will display the focus ring if any of its descendants receives focus
<div tabindex="0" role="button" style={{outline: 'none'}}>Tab here to see the focus outline</div>
</View>
</Flex>
```

In some situations, you may want to manually control when the focus outline is displayed instead of leaving it up to the browser.
Expand All @@ -455,8 +446,7 @@ Be careful when overriding the display of the focus outline as it is essential f

The focus outline adjusts to account for the shape of the View. For example, the following values can be set for `borderRadius`:
`circle`, `pill`, `small`, `medium`, and `large`. In each case, the border radius of the focus outline will automatically adjust
to match the border radius of the corresponding View. For Views with irregular border radius (e.g., `borderRadius="small large none medium"`), the focus outline will appear with square edges. The color of the focus outline can be
changed for different contexts via the `focusColor` property.
to match the border radius of the corresponding View. The color of the focus outline can be changed for different contexts via the `focusColor` property.

- ```javascript
class FocusedExample extends React.Component {
Expand All @@ -465,7 +455,8 @@ changed for different contexts via the `focusColor` property.

this.state = {
isFocused: true,
inset: false
inset: false,
focusColor: undefined
}
}

Expand All @@ -477,6 +468,10 @@ changed for different contexts via the `focusColor` property.
this.setState({ inset: event.target.checked })
}

updateFocusRingColor = (event, value) => {
this.setState({ focusColor: value })
}

render() {
return (
<View as="div">
Expand All @@ -495,16 +490,32 @@ changed for different contexts via the `focusColor` property.
</ScreenReaderContent>
}
>
<Checkbox
label="withFocusOutline"
checked={this.state.isFocused}
onChange={this.updateFocused}
/>
<Checkbox
label="focusPosition = inset"
checked={this.state.inset}
onChange={this.updateInset}
/>
<Flex gap="small" direction="row">
<Flex gap="small" direction="column" width="15rem">
<Checkbox
label="withFocusOutline"
checked={this.state.isFocused}
onChange={this.updateFocused}
/>
<Checkbox
label="focusPosition = inset"
checked={this.state.inset}
onChange={this.updateInset}
/>
</Flex>
<RadioInputGroup
onChange={this.updateFocusRingColor}
name="focusColor"
defaultValue="info"
variant="toggle"
description="Focus ring color"
>
<RadioInput label="info" value="info" />
<RadioInput label="inverse" value="inverse" />
<RadioInput label="success" value="success" />
<RadioInput label="danger" value="danger" />
</RadioInputGroup>
</Flex>
</FormFieldGroup>
</View>
<View as="div">
Expand All @@ -517,6 +528,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="small"
borderWidth="small"
position="relative"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -530,6 +542,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="medium"
borderWidth="small"
position="relative"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -543,6 +556,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="large"
borderWidth="small"
position="relative"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -557,6 +571,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="circle"
borderWidth="small"
position="relative"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -583,7 +598,7 @@ changed for different contexts via the `focusColor` property.
borderWidth="small"
position="relative"
withFocusOutline={this.state.isFocused}
focusColor="inverse"
focusColor={this.state.focusColor}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
medium
Expand All @@ -597,9 +612,9 @@ changed for different contexts via the `focusColor` property.
borderRadius="pill"
borderWidth="small"
position="relative"
focusColor="success"
width="100px"
textAlign="center"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -612,8 +627,8 @@ changed for different contexts via the `focusColor` property.
background="primary"
borderWidth="small"
borderRadius="none large"
focusColor="danger"
position="relative"
focusColor={this.state.focusColor}
withFocusOutline={this.state.isFocused}
focusPosition={this.state.inset ? 'inset' : 'offset'}
>
Expand All @@ -624,17 +639,18 @@ changed for different contexts via the `focusColor` property.
)
}
}

render(<FocusedExample />)
```

- ```javascript
const FocusedExample = () => {
const [isFocused, setIsFocused] = useState(true)
const [inset, setInset] = useState(false)
const [focusColor, setfocusColor] = useState(undefined)

const updateFocused = (event) => setIsFocused(event.target.checked)
const updateInset = (event) => setInset(event.target.checked)
const updateFocusRingColor = (event) => setfocusColor(event.target.value)

return (
<View as="div">
Expand All @@ -653,16 +669,32 @@ changed for different contexts via the `focusColor` property.
</ScreenReaderContent>
}
>
<Checkbox
label="withFocusOutline"
checked={isFocused}
onChange={updateFocused}
/>
<Checkbox
label="focusPosition = inset"
checked={inset}
onChange={updateInset}
/>
<Flex gap="small" direction="row">
<Flex gap="small" direction="column" width="15rem">
<Checkbox
label="withFocusOutline"
checked={isFocused}
onChange={updateFocused}
/>
<Checkbox
label="focusPosition = inset"
checked={inset}
onChange={updateInset}
/>
</Flex>
<RadioInputGroup
onChange={updateFocusRingColor}
name="focusColor_2"
defaultValue="info"
variant="toggle"
description="Focus ring color"
>
<RadioInput label="info" value="info" />
<RadioInput label="inverse" value="inverse" />
<RadioInput label="success" value="success" />
<RadioInput label="danger" value="danger" />
</RadioInputGroup>
</Flex>
</FormFieldGroup>
</View>
<View as="div">
Expand All @@ -675,6 +707,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="small"
borderWidth="small"
position="relative"
focusColor={focusColor}
withFocusOutline={isFocused}
focusPosition={inset ? 'inset' : 'offset'}
>
Expand All @@ -689,6 +722,7 @@ changed for different contexts via the `focusColor` property.
borderWidth="small"
position="relative"
withFocusOutline={isFocused}
focusColor={focusColor}
focusPosition={inset ? 'inset' : 'offset'}
>
medium
Expand All @@ -702,6 +736,7 @@ changed for different contexts via the `focusColor` property.
borderWidth="small"
position="relative"
withFocusOutline={isFocused}
focusColor={focusColor}
focusPosition={inset ? 'inset' : 'offset'}
>
large
Expand All @@ -716,6 +751,7 @@ changed for different contexts via the `focusColor` property.
borderWidth="small"
position="relative"
withFocusOutline={isFocused}
focusColor={focusColor}
focusPosition={inset ? 'inset' : 'offset'}
>
<Flex
Expand All @@ -741,6 +777,7 @@ changed for different contexts via the `focusColor` property.
borderWidth="small"
position="relative"
withFocusOutline={isFocused}
focusColor={focusColor}
focusColor="inverse"
focusPosition={inset ? 'inset' : 'offset'}
>
Expand All @@ -758,6 +795,7 @@ changed for different contexts via the `focusColor` property.
focusColor="success"
width="100px"
textAlign="center"
focusColor={focusColor}
withFocusOutline={isFocused}
focusPosition={inset ? 'inset' : 'offset'}
>
Expand All @@ -772,6 +810,7 @@ changed for different contexts via the `focusColor` property.
borderRadius="none large"
focusColor="danger"
position="relative"
focusColor={focusColor}
withFocusOutline={isFocused}
focusPosition={inset ? 'inset' : 'offset'}
>
Expand Down Expand Up @@ -936,7 +975,9 @@ props.

### Debugging

Set the `withVisualDebug` prop to see the View's boundaries.
Set the `withVisualDebug` prop to see the View's boundaries. Use this only for debugging.

> This effect uses a CSS box-shadow, so the `shadow` prop will be overridden
```js
---
Expand Down
33 changes: 0 additions & 33 deletions packages/ui-view/src/View/__new-tests__/View.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,39 +234,6 @@ describe('<View />', () => {
expect(newStyles.maxWidth).toEqual('200px')
})

describe('withFocusOutline', () => {
it('should warn when withFocusOutline is true without position=relative', () => {
render(
<View withFocusOutline>
<h1>View Content</h1>
</View>
)
const expectedErrorMessage =
'Warning: [View] the focus outline will only show if the `position` prop is `relative`.'

expect(consoleErrorMock).toHaveBeenCalledWith(
expect.stringContaining(expectedErrorMessage),
expect.any(String)
)
})

it('should warn when withFocusOutline is `true`, display is set to `inline`, and focusPosition is set to `offset`', () => {
render(
<View withFocusOutline display="inline" focusPosition="offset">
<h1>View Content</h1>
</View>
)

const expectedErrorMessage =
'Warning: [View] when display is set to `inline` the focus outline will only show if `focusPosition` is set to `inset`.'

expect(consoleErrorMock).toHaveBeenCalledWith(
expect.stringContaining(expectedErrorMessage),
expect.any(String)
)
})
})

it('should meet a11y standards', async () => {
const { container } = render(<View>View Content</View>)

Expand Down
Loading
Loading