Skip to content
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

Merged
merged 3 commits into from
May 31, 2019

Conversation

Andarist
Copy link
Contributor

@necolas
Copy link
Contributor

necolas commented May 19, 2019

What's this for please?

@Andarist
Copy link
Contributor Author

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.

@Andarist Andarist force-pushed the fix/focus-mac-tab-with-alt branch from 5781918 to cc9541c Compare May 19, 2019 21:26
@trueadm
Copy link
Contributor

trueadm commented May 20, 2019

Could you add some tests please? :)

@necolas
Copy link
Contributor

necolas commented May 20, 2019

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.

@Andarist
Copy link
Contributor Author

Could you add some tests please? :)

Sure thing, gonna add them in following days.

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.

Sorry that I haven't explained it properly right away!

I've encountered this problem last week while implementing custom radio/checkboxes, our QA has reported that focusing with Tab doesn't work properly on Safari for those (radio/checkbox). To my big surprise I have found out that Safari (follow Mac behaviour) doesn't handle this like other browsers.

When pressing Tab alone it just jumps to the next text-like input (and probably some lists too), not to next focusable element. To jump to the next focusable element you have to press Tab+alt. I've checked it on the system level and I've checked out all major browsers on Mac - they all handle Tab OK (except Safari) but they also allow for Tab+alt (like the host system - Mac).

Seems like this behaviour can be configured on Mac, but the default is what I have described. Take a look at the bottom of the keyboard's Shortcus section of the system preferences:
Screen Shot 2019-05-21 at 00 15 56

I've checked out at home how exactly it behaves on Windows on my wife's computer - and each additional key with Tab triggers different action, so the condition as of before this PR was OK for Windows - I yet have to check Linux, but I expect it to behave exactly the same as Windows in this regard.

@Andarist
Copy link
Contributor Author

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', () => {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

function init() {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableEventAPI = true;
React = require('react');
ReactDOM = require('react-dom');
Press = require('react-events/press');
Scheduler = require('scheduler');
}

Copy link
Contributor Author

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.

packages/react-events/src/Focus.js Outdated Show resolved Hide resolved
@@ -327,3 +327,65 @@ describe('Focus event responder', () => {
expect(Focus.displayName).toBe('Focus');
});
});

describe('Focus event responder - Mac', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

function init() {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableEventAPI = true;
React = require('react');
ReactDOM = require('react-dom');
Press = require('react-events/press');
Scheduler = require('scheduler');
}

@Andarist Andarist force-pushed the fix/focus-mac-tab-with-alt branch from 5cb9ade to 0dbc79d Compare May 31, 2019 21:07
@Andarist
Copy link
Contributor Author

@necolas should be ready to merge now (if CI passes), thanks for the help :)

@sizebot
Copy link

sizebot commented May 31, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@necolas necolas merged commit 63fe08e into facebook:master May 31, 2019
@Andarist Andarist deleted the fix/focus-mac-tab-with-alt branch May 31, 2019 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants