-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Fix issue with Tab+alt not being considered as isGlobalFocusVisible c… #15679
Conversation
What's this for please? |
It's for focus type tracking - to be precise to determine which pointerType has originated focus. Without distinguishing this check (altKey) per platform you incorrectly lose info about Tab+altKey triggering focus on Mac. I could understand something in the wrong way, but according to my quick tests it would be an issue. |
5781918
to
cc9541c
Compare
Could you add some tests please? :) |
Please can you also provide adequate context for this patch. For example, explaining why this is applied only to Mac, i.e., what's the difference in expected behaviour on Windows and Linux. |
I've added a test for this particular scenario. I wanted to add an "opposite" test - for non-Mac - that it doesn't set this property etc, but because tests are relying on the manual event dispatching this is tricky. I'm not particularly fond of how I have added this test (it results in quite a duplication of the setup/teardown code), but I've tried bunch of possibilities and in the end I've decided this is the most straightforward one. Can't add a conditional setup to a single test in the middle of the setup chain. If you want me to refactor this anyhow - let me know. |
@@ -327,3 +327,65 @@ describe('Focus event responder', () => { | |||
expect(Focus.displayName).toBe('Focus'); | |||
}); | |||
}); | |||
|
|||
describe('Focus event responder - Mac', () => { |
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.
You shouldn't need to create another top-level describe
, setup the feature flags, etc. Put the test into the relevant onFocus
test block that already exists above.
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.
Totally agree, I wasn't sure how to approach mocking navigator for this single test. The problem is that setup for each test is done in beforeEach and ideally I would like to mock this BEFORE common beforeEach, but this is not quite possible - or I don't know how to do it, but after searching through jest documentation I couldn't find a way to do this. Any ideas?
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.
Oh because you need the navigator
mocked before Focus
is evaluated? Hmm, not sure. I'll look into 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.
yeah, that's exactly the "problem" with putting this into existing describe block
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.
Have a look at how it's being done here: #15761. Once you reuse that pattern for this PR we can merge it. Thanks!
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.
See
react/packages/react-events/src/__tests__/Press-test.internal.js
Lines 39 to 46 in 0f7cc2b
function init() { | |
ReactFeatureFlags = require('shared/ReactFeatureFlags'); | |
ReactFeatureFlags.enableEventAPI = true; | |
React = require('react'); | |
ReactDOM = require('react-dom'); | |
Press = require('react-events/press'); | |
Scheduler = require('scheduler'); | |
} |
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.
Thanks for the tip! Done.
@@ -327,3 +327,65 @@ describe('Focus event responder', () => { | |||
expect(Focus.displayName).toBe('Focus'); | |||
}); | |||
}); | |||
|
|||
describe('Focus event responder - Mac', () => { |
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.
See
react/packages/react-events/src/__tests__/Press-test.internal.js
Lines 39 to 46 in 0f7cc2b
function init() { | |
ReactFeatureFlags = require('shared/ReactFeatureFlags'); | |
ReactFeatureFlags.enableEventAPI = true; | |
React = require('react'); | |
ReactDOM = require('react-dom'); | |
Press = require('react-events/press'); | |
Scheduler = require('scheduler'); | |
} |
5cb9ade
to
0dbc79d
Compare
@necolas should be ready to merge now (if CI passes), thanks for the help :) |
No significant bundle size changes to report. Generated by 🚫 dangerJS |
cc @trueadm @necolas