Skip to content

[TrapFocus] Fixed focus theft #2681

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 5, 2020
Merged

[TrapFocus] Fixed focus theft #2681

merged 4 commits into from
Feb 5, 2020

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Jan 28, 2020 •
edited
Loading

WHY are these changes introduced?

Fixes #2641

WHAT is this pull request doing?

  • Creates focus manager to handle multiple trap focuses
  • Create useFocusManager to handle listing and delisting trap focuses and letting the component know when its safe to focus
  • Adds new application state (focus manager)
  • Adds focus manager to app provider
  • Use useFocusManager in TrapFocus
  • Tests

Screenie

Screen Shot 2020-01-28 at 5 58 46 PM

How to 🎩

Use the performance tab and make sure focus first focusable node is not running rampant. Sample code with two trap focuses below

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {Card, TextContainer, TextField, TrapFocus} from '../src';

export function Playground() {
  return (
    <div>
      <Card sectioned>
        <TrapFocus>
          <TextContainer>
            <TextField label="test" autoFocus />
          </TextContainer>
        </TrapFocus>
        
        <TrapFocus>
          <TextContainer>
            <TextField label="test" autoFocus />
          </TextContainer>
        </TrapFocus>
      </Card>
    </div>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2020 •
edited
Loading

🟡 This pull request modifies 13 files and might impact 14 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 14)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/AppProvider.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/tests/AppProvider.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/FocusManager/FocusManager.tsx (total: 2)

Files potentially affected (total: 2)

🧩 src/components/FocusManager/index.ts (total: 2)

Files potentially affected (total: 2)

🧩 src/components/FocusManager/tests/FocusManager.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/PolarisTestProvider/PolarisTestProvider.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/TrapFocus/TrapFocus.tsx (total: 8)

Files potentially affected (total: 8)

🧩 src/components/TrapFocus/tests/TrapFocus.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/focus-manager/context.tsx (total: 14)

Files potentially affected (total: 14)

🧩 src/utilities/focus-manager/hooks.ts (total: 13)

Files potentially affected (total: 13)

🧩 src/utilities/focus-manager/index.ts (total: 12)

Files potentially affected (total: 12)

🧩 src/utilities/focus-manager/tests/hooks.test.tsx (total: 0)

Files potentially affected (total: 0)

AndrewMusgrave
AndrewMusgrave commented Jan 28, 2020
View reviewed changes
src/components/PolarisTestProvider/PolarisTestProvider.tsx Outdated
</FrameContext.Provider>
<FocusManagerContext.Provider
value={mergedFocusManager}
>
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we just use FocusManager and not allow any overrides?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now we probably should. Not sure we'll ever need to override it. We can do it when we do.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a different pattern than we've previously been using, however FocusManager has some overhead we would need to replicate in mocks and it's easier to use the component. Since this is a public API let's go with using FocusManager since adding to an API is easier than removing.

@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review January 28, 2020 23:11
@AndrewMusgrave
Copy link
Member Author

Let me know what you think @dleroux I changed it up slightly from what we looked at this morning. Listing and delisting logic now lives inside the hook.

dleroux
dleroux approved these changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

added a comment on merging but code looks good. It looks like it does fix the issue in web 🎩

@AndrewMusgrave
Copy link
Member Author

Codecov messed up and included files that weren't touched. Coverage is over 90 so I'm going to 🚢

@AndrewMusgrave AndrewMusgrave merged commit 505e59a into master Feb 5, 2020
@AndrewMusgrave AndrewMusgrave deleted the trapfocus-stealing branch February 5, 2020 16:39
@sijad sijad mentioned this pull request Feb 28, 2020
Closed
@tmlayton tmlayton temporarily deployed to production March 7, 2020 05:24 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dleroux dleroux dleroux approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[TrapFocus] causes infinite loop when used in a Dialog

3 participants

@AndrewMusgrave @dleroux @tmlayton