CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Remove forced focus of InputControl
on mousedown
#41118
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
Conversation
Size Change: -39 B (0%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
} ); | ||
|
||
describe( 'Ensurance of focus for number inputs', () => { | ||
it( 'should focus its input on mousedown events', () => { |
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.
Maybe a silly question, but — shouldn't we keep this unit test?
Technically the code in this PR is not necessary, given change in #40518 — and therefore I would keep the unit tests as they are, and expect the to still pass.
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.
The test was added along with the code in #29305. I expected such a test to never fail #29305 (comment) because inputs are focused on mousedown
(except Firefox when mousedown
is on the spin buttons). Apparently that focus is not synchronous. So without the code this test fails.
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.
I see, thank you for the context!
I tried using user-event
and it seems to fix the issue:
describe( 'Ensurance of focus for number inputs', () => {
it( 'should focus its input on mousedown events', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );
const spy = jest.fn();
render( <InputControl type="number" onFocus={ spy } /> );
const input = getInput();
await user.click( input );
expect( spy ).toHaveBeenCalledTimes( 1 );
} );
} );
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.
Pardon me for dropping this. I still think the test should be removed. If we "fix" it as suggested then it's testing that focus happens when clicked and that's merely testing UA behavior.
5150ade
to
3ac6020
Compare
Hi, @stokesman I'm doing old PR cleanup, can we close this draft as "stale"? |
b3780ab
to
868add0
Compare
Thanks for asking. I'd like if this can still land so I rebased it but it's not by any means necessary. |
Flaky tests detected in 868add0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/15288377263
|
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. |
868add0
to
5cb5cb8
Compare
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.
🚀 LGTM
- âś… Code changes look good
- âś… The unit test being removed would pass anyway
Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
What?
Removes code obviated by #40518.
Why?
It is no longer needed as it was only required (for Firefox) when
InputControl
had to be focused in order to update itself.How?
Removes the code and a unit test that was specific to it.
Testing Instructions
NumberControl
can be clicked to increment/decrement without causing the input to gain focus.InputControl
still behave as expected.Screen Recording
input-control-no-forced-focus.mp4