-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Components: Remove CircleIndicatorWrapper focus-visible
outline
#45758
Conversation
|
8347cc1
to
e5756c9
Compare
Size Change: +17 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
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.
Thank you for working on this!
I think that an even better fix, on top of removing that glitchy outline, would be to add better :focus-visible
styles, maybe an outer ring around the control?
Maybe something like this
diff --git a/packages/components/src/angle-picker-control/styles/angle-picker-control-styles.js b/packages/components/src/angle-picker-control/styles/angle-picker-control-styles.js
index 29b38850c0..6d97a9aeee 100644
--- a/packages/components/src/angle-picker-control/styles/angle-picker-control-styles.js
+++ b/packages/components/src/angle-picker-control/styles/angle-picker-control-styles.js
@@ -33,7 +33,6 @@ export const CircleRoot = styled.div`
box-sizing: border-box;
cursor: grab;
height: ${ CIRCLE_SIZE }px;
- overflow: hidden;
width: ${ CIRCLE_SIZE }px;
`;
@@ -42,9 +41,10 @@ export const CircleIndicatorWrapper = styled.div`
position: relative;
width: 100%;
height: 100%;
+ border-radius: 50%;
:focus-visible {
- outline: none;
+ outline: ${ COLORS.ui.theme } auto 1px;
}
`;
I really like that idea, and I've proposed a couple of modifications to it in ecccc61
What do you think? |
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.
After having a better look at this, it looks like the "knob" input is only focusable when clicking it, and is otherwise not focusable via keyboard and hidden to screen readers (aria-hidden="true"
).
Having said that, we basically need to understand which behaviour we want to pick:
- show an outline only while clicking on the knob (i.e. when
:active
) - show an outline not only while clicking, but also after releasing the mouse button, until another element of the page receives focus (i.e. when
:focus-visible
)
Maybe @jasmussen can help us decide?
Forgive me if I'm missing something, but I'm not seeing any focus indicator for the angle control in trunk: I guess it's only in storybook. But yes I not expect the angle control to receive a focus style, I'm not sure it's even tabbable. Happy to be educated if I'm in the wrong here, but it seems like the tabbable control is the explicit degree input field, which is a far more useful keyboard control than an angle picker, which doesn't even appear to work with the arrow keys. |
Apologies for not being as clear, @jasmussen ! Let me leave a few more details:
Kapture.2022-11-17.at.18.19.56.mp4As you also point out, these focus styles may not be currently visible in the editor because they get overridden by other styles, but we want to fix this glitch in the "vanilla" version of the component anyway — and potentially even make them visible in the editor, as they would offer a better UX to who is interacting with the know. Instead of a glitchy outline, we'd like to show a proper outline — but the question is: should the outline be visible:
It's a small detail but we thought you may have a preference :) |
Right, sounds good, but if the control is only meant to be mouse-interactable, does it need a focus style? Asking genuinely and happy to learn — it can't be tabbed to, so you have to find its location and move the cursor there. The control then acts upon your input. So it doesn't seem like it needs any further emphasis. |
Accessibility-wise, I believe the only situation a focus indicator here would be useful is like:
So, although I don't think it's absolutely necessary that there be a focus indicator at all, it wouldn't make sense to make it |
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.
As agreed above, let's go with having an outline showing when the know is :active
🚀
Thank you all for your patience and the feedback ❤️
packages/components/CHANGELOG.md
Outdated
@@ -28,6 +28,7 @@ | |||
- `Draggable`: Convert to TypeScript ([#45471](https://github.com/WordPress/gutenberg/pull/45471)). | |||
- `MenuGroup`: Convert to TypeScript ([#45617](https://github.com/WordPress/gutenberg/pull/45617)). | |||
- `useCx`: fix story to satisfy the `react-hooks/exhaustive-deps` eslint rule ([#45614](https://github.com/WordPress/gutenberg/pull/45614)) | |||
- `AnglePickerControl`: remove `:focus-visible` outline on `CircleOutlineWrapper` ([#45758](https://github.com/WordPress/gutenberg/pull/45758)) |
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.
We'll likely need to move this entry to the most recent Unreleased
section, since there has been a package release in the meantime
Just to make sure, I'm saying that the outline should be visible on focus, not Accessibility-wise, I think it would also be fine to not show the outline at all (think |
Also for what it's worth, I would lean towards not showing the focus outline, or embrace focus-visible. I think the mover control uses that. |
Thank you all for the extra clarifications — after reading everyone's comments, I believe that the best/simplest solution is to remove the outline completely, as per @chad1008 's initial implementation — we can always add a focus outline back in case the knob ever becomes keyboard-interactable. |
ecccc61
to
3c4c1a9
Compare
Thanks for sharing all of that input everyone! I've reverted back to just removing the Just waiting on CI now! |
Late to this party but I'd like to add that the reason the knob became focusable (in #40735) was to fix a separate issue with an existing workaround for the old behavior of |
Followup to #45289
What?
Removes the outline that appears while dragging the CircleIndicatorWrapper contained in the AnglePickerControl component.
Why?
In some browsers (i.e. Chrome) the outline is generated by user agent styles. It previously didn't stand out much, because the browser's default blue almost matches the default WP accent color.
As the Components package adopts theming, however, the accent color can change, leading to an unpleasant contrast.
How?
Add to the CircleIndicatorWrappers styling to remove the
focus-visible
outline effect.Testing Instructions
AnglePickerControl
component and use theCircleIndicator
on the right hand side to change the angle valueCircleIndiator
's boundaries.Screenshots or screencast