fix(mixins): improve focus-visible visualisation#4022
fix(mixins): improve focus-visible visualisation#4022LucyChyzhova wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughReplaces box-shadow-based :focus-visible indicators with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4022/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/style/mixins.scss`:
- Around line 611-615: The :focus-visible:active rule duplicates the
:focus-visible block (same outline and outline-offset); remove the redundant
&:focus-visible:active selector (or merge by combining selectors) so only a
single rule defines the outline styles—update the selectors near the existing
:focus-visible rule to keep one declaration for outline and outline-offset to
avoid duplicate styles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ed52b29-d1b3-4391-b9f9-21547fbdd1ab
📒 Files selected for processing (1)
src/style/mixins.scss
| box-shadow: var(--shadow-depth-8-focused); | ||
| outline: 0.125rem solid | ||
| color-mix(in srgb, var(--mdc-theme-primary) 60%, transparent); | ||
| outline-offset: -0.25rem; |
There was a problem hiding this comment.
| outline-offset: -0.25rem; | |
| outline: 1px solid | |
| color-mix(in srgb, var(--mdc-theme-primary) 60%, transparent); | |
| outline-offset: 0.125rem; |
LucyChyzhova
force-pushed
the
fix/3907-improve-selected-and-focused-states
branch
from
c804fc9 to
161786b
Compare
LucyChyzhova
force-pushed
the
fix/3907-improve-selected-and-focused-states
branch
from
161786b to
bef1f5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/style/mixins.scss`:
- Around line 18-19: Replace the hardcoded outline-offset in the mixin with the
shared CSS variable by changing the outline-offset: 0.03125rem line to use
var(--outline-focus-visible-offset); update the mixin in src/style/mixins.scss
where outline: var(--outline-focus-visible) is set so it references the new
token (--outline-focus-visible-offset) instead of the literal value, ensuring
consistency with other focus styles.
- Around line 605-607: The focus indicator in the `@mixin`
the-3d-element--clickable currently uses outline-only (outline:
var(--outline-focus-visible); outline-offset:
var(--outline-focus-visible-offset);) which can be clipped by overflow: hidden
consumers; restore the box-shadow focus ring by reintroducing box-shadow:
var(--shadow-depth-8-focused), var(--button-shadow-hovered) alongside the
outline (for :focus and/or :focus-visible states) so keyboard focus remains
visible even when the element is clipped, ensuring the variables
--shadow-depth-8-focused and --button-shadow-hovered are used and kept in the
same focus rule as the outline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72806c09-fcf8-4685-baee-ab76d67435a2
📒 Files selected for processing (2)
src/style/mixins.scsssrc/style/shadows.scss
| outline: var(--outline-focus-visible); | ||
| outline-offset: 0.03125rem; // 0.5px |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the shared outline-offset token here for consistency.
Line 19 hardcodes offset while the PR introduces --outline-focus-visible-offset. Reuse the token to keep focus visuals uniform across mixins.
♻️ Suggested patch
- outline-offset: 0.03125rem; // 0.5px
+ outline-offset: var(--outline-focus-visible-offset);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| outline: var(--outline-focus-visible); | |
| outline-offset: 0.03125rem; // 0.5px | |
| outline: var(--outline-focus-visible); | |
| outline-offset: var(--outline-focus-visible-offset); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/style/mixins.scss` around lines 18 - 19, Replace the hardcoded
outline-offset in the mixin with the shared CSS variable by changing the
outline-offset: 0.03125rem line to use var(--outline-focus-visible-offset);
update the mixin in src/style/mixins.scss where outline:
var(--outline-focus-visible) is set so it references the new token
(--outline-focus-visible-offset) instead of the literal value, ensuring
consistency with other focus styles.
| // box-shadow: var(--shadow-depth-8-focused), var(--button-shadow-hovered); | ||
| outline: var(--outline-focus-visible); | ||
| outline-offset: var(--outline-focus-visible-offset); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mixin consumers and check for overflow clipping risk.
rg -nP '@include\s+the-3d-element--clickable\b' -g '**/*.scss' -C2
rg -nP 'overflow\s*:\s*hidden\s*;' -g '**/*.scss' -C2Repository: Lundalogik/lime-elements
Length of output: 12528
🏁 Script executed:
# Search for the mixin definition
rg -n '@mixin the-3d-element--clickable' src/style/mixins.scss
# Check info-tile.scss to see how it actually uses styles
cat -n src/components/info-tile/info-tile.scss | head -100
# Search for the mixin in all files with variations
rg -n 'the-3d-element--clickable' -g '**/*.scss'Repository: Lundalogik/lime-elements
Length of output: 4483
🏁 Script executed:
# Check the context around line 403 in info-tile.scss
sed -n '395,415p' src/components/info-tile/info-tile.scss
# View the mixin definition
sed -n '589,620p' src/style/mixins.scssRepository: Lundalogik/lime-elements
Length of output: 1196
🏁 Script executed:
# Check for --shadow-focused-state variable
rg -n '--shadow-focused-state' -g '**/*.scss'
# Check for --shadow-depth-8-focused variable
rg -n '--shadow-depth-8-focused' -g '**/*.scss'
# Look for shadow variable definitions
rg -n 'shadow.*focused|focused.*shadow' -g '**/*.scss' | head -20Repository: Lundalogik/lime-elements
Length of output: 2832
🏁 Script executed:
# Check other focus-visible implementations
rg -B2 -A2 '&:focus-visible' src/components -g '*.scss' | head -60
# Also check the shadows file to understand the variables better
sed -n '10,80p' src/style/shadows.scssRepository: Lundalogik/lime-elements
Length of output: 7458
Outline-only focus indicator can be clipped in overflow: hidden consumers.
@mixin the-3d-element--clickable is used by components like src/components/info-tile/info-tile.scss (which sets overflow: hidden on the <a> element). With the focus ring moved to outline-only on lines 606–607, keyboard focus visibility can regress in those cases.
♻️ Suggested fix (restore box-shadow for focus state)
&:focus-visible {
- outline: var(--outline-focus-visible);
- outline-offset: var(--outline-focus-visible-offset);
+ box-shadow: var(--shadow-depth-8-focused), var(--button-shadow-hovered);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // box-shadow: var(--shadow-depth-8-focused), var(--button-shadow-hovered); | |
| outline: var(--outline-focus-visible); | |
| outline-offset: var(--outline-focus-visible-offset); | |
| &:focus-visible { | |
| box-shadow: var(--shadow-depth-8-focused), var(--button-shadow-hovered); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/style/mixins.scss` around lines 605 - 607, The focus indicator in the
`@mixin` the-3d-element--clickable currently uses outline-only (outline:
var(--outline-focus-visible); outline-offset:
var(--outline-focus-visible-offset);) which can be clipped by overflow: hidden
consumers; restore the box-shadow focus ring by reintroducing box-shadow:
var(--shadow-depth-8-focused), var(--button-shadow-hovered) alongside the
outline (for :focus and/or :focus-visible states) so keyboard focus remains
visible even when the element is clipped, ensuring the variables
--shadow-depth-8-focused and --button-shadow-hovered are used and kept in the
same focus rule as the outline.
| } | ||
|
|
||
| &:focus-visible { | ||
| outline: none; |
There was a problem hiding this comment.
Quick bad not… just writing so we don't forget. Many components don't use our "clickable" mixins, and replicate them. They also add transitions for box-shadow. Now that we are changing this to outline, we need to make sure that those transitions are updated if necessary. I'll elaborate later
|
while testing noticed this behaviour: Screen.Recording.2026-04-20.at.11.23.51.mov |
2 participants
fix: #3907
Screen.Recording.2026-04-14.at.17.51.31.mov
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: