InputBase: Simplify focus styles #60226
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Enhancement
mirka
commented
Mar 27, 2024
Comment on lines
+26
to
+36
| export const Prefix = styled.span` | ||
| box-sizing: border-box; | ||
| display: block; | ||
| `; | ||
|
|
||
| export const Suffix = styled.span` | ||
| align-items: center; | ||
| align-self: stretch; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| `; |
There was a problem hiding this comment.
No changes, just moved higher up the file so we can refer to them earlier.
| } | ||
|
|
||
| if ( disabled ) { | ||
| borderColor = isBorderless ? 'transparent' : COLORS.ui.borderDisabled; |
There was a problem hiding this comment.
The important priority logic in here is for border-color, where isFocused > isBorderless > disabled (accounting for the fact that a disabled control will never be focused). This priority logic still holds true in the refactored styles.
mirka
commented
Mar 27, 2024
| ${ rootFocusedStyles } | ||
|
|
||
| &:focus-within:not( :has( :is( ${ Prefix }, ${ Suffix } ):focus-within ) ) { | ||
| z-index: 1; |
There was a problem hiding this comment.
Not sure why this z-index is necessary, but will leave it in for the scope of this refactor PR.
mirka
commented
Mar 27, 2024
| @@ -109,8 +108,6 @@ export function InputBase( | |||
| { ...getUIFlexProps( labelPosition ) } | |||
| className={ className } | |||
| gap={ 2 } | |||
| isFocused={ isFocused } | |||
| labelPosition={ labelPosition } | |||
There was a problem hiding this comment.
Removed because the labelPosition prop was not being used.
| </li> | ||
| ) ) } | ||
| </ul> | ||
| </div> | ||
| </InputBase> |
There was a problem hiding this comment.
Rejiggers the DOM structure a bit so the focus styles can be managed with CSS only.
|
Size Change: -121 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I'm in the middle of reviewing this, please leave it to me (in case someone was thinking of taking over) :) |
tyxla
reviewed
Apr 3, 2024
There was a problem hiding this comment.
Leaving a couple of drive-by comments as I was glancing through it.
I still haven't done a thorough review or testing as of yet.
I'm in the middle of reviewing this, please leave it to me (in case someone was thinking of taking over) :)
Sounds great, go for it @DaniGuardiola 🙌
| border-color: ${ backdropBorderColor }; | ||
| border-radius: inherit; | ||
| border-style: solid; | ||
| border-width: 1px; |
There was a problem hiding this comment.
Should this be using CONFIG.borderWidth?
There was a problem hiding this comment.
I don't see the point of having that be a global variable to be honest, it may be on the chopping block for #43997.
Comment on lines
+91
to
+93
| // Windows High Contrast mode will show this outline, but not the box-shadow. | ||
| outline: 2px solid transparent; | ||
| outline-offset: -2px; |
There was a problem hiding this comment.
We have this contrast mode enhancement at least in a few places already - should we make it reusable?
There was a problem hiding this comment.
Since we're migrating away from emotion I'd suggest holding off on creating any abstractions, at least until a unified styling solution is decided.
There was a problem hiding this comment.
Good that it's easy to grep, since they always have a code comment 😂
DaniGuardiola
approved these changes
Apr 9, 2024
| ${ rootFocusedStyles } | ||
|
|
||
| // Focus within, excluding cases where auxiliary controls in prefix or suffix have focus. | ||
| &:focus-within:not( :has( :is( ${ Prefix }, ${ Suffix } ):focus-within ) ) { |
There was a problem hiding this comment.
I'm unfamiliar with the browser versions we're targeting for support but beware that :has is recent enough to still not be supported for some potential users: https://caniuse.com/css-has
There was a problem hiding this comment.
Related thought but, are we only targeting the input for focus here? Because, in that case, a simpler selector like :has(input:focus) might work and be easier both on maintainers and the browser itself.
There was a problem hiding this comment.
in that case, a simpler selector like
:has(input:focus)might work
It's tempting, but technically the prefix/suffix slots can also contain input elements, so no 🥲 Also the primary focusable element in the InputBase isn't always an input, as is the case with SelectControl.
DaniGuardiola
approved these changes
Apr 9, 2024
|
tyxla
approved these changes
Apr 16, 2024
| @@ -47,6 +47,17 @@ exports[`DimensionControl rendering renders with custom sizes 1`] = ` | |||
| min-height: 0; | |||
| } | |||
|
|
|||
| .emotion-5:focus-within:not( :has( :is( .em5sgkm7, .emotion-18 ):focus-within ) ) { | |||
There was a problem hiding this comment.
Doesn't look like we particularly care about those styles. Would be nice if we alter the snapshot test to something more specific. Not a blocker for this PR.
There was a problem hiding this comment.
Agreed. I'm even questioning the validity of DimensionControl as a standalone component, but that's another story 🙃
packages/components/CHANGELOG.md
Outdated
| @@ -13,6 +13,7 @@ | |||
| ### Internal | |||
|
|
|||
| - `Popover`, `ColorPicker`: Obviate pointer event trap #59449 ([#59449](https://github.com/WordPress/gutenberg/pull/59449)). | |||
| - `InputBase`: Simplify management of focus styles. Affects all components based on `InputControl` (e.g. `SearchControl`, `UnitControl`), as well as `SelectControl` and `CustomSelectControl` ([#60226](https://github.com/WordPress/gutenberg/pull/60226)). | |||
There was a problem hiding this comment.
Should we list them all? May be easier if someone is looking for the changelog in the case of a regression.
There was a problem hiding this comment.
It's impractical to list all of them because so many compound components include some form of these inputs, but I added a few more to cover all the relatively primitive ones. I think that should be sufficient because those are the ones more likely to involve consumer overrides.
|
Thanks for the reviews and testing! 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Simplifies how we coordinate focus styles in the
InputBaseprimitive component so it can be done "automatically" with just CSS, without relying on React props/state.No visual/behavioral changes are intended.
Why?
InputBase is an internal component used to style the standard borders for an input, as well as handle the layout for prefix/suffix elements.
I'd like to reuse InputBase for styling CustomSelectControlV2, like we currently do with v1. This is a good opportunity to simplify this focus handling in there, lest we add even more unnecessary state management.
How?
Use
:focus-withinselectors to detect inner focus declaratively, rather than keeping track of it imperatively through focus/blur listeners.Testing Instructions
There shouldn't have been any visual changes in focus behavior.
Affected components worth testing
disabledprop enabled.Affected but low chance of regressions
Affected through style overrides of relevant elements
Testing Instructions for Keyboard
Test Tab and arrow keys to navigate elements, especially UnitControl and CustomSelectControl (v1).