refactor: rewrite the useFocusManagement hook with a new approach #2369

matuzalemsteles · 2019-09-02T21:08:57Z

This still has a design problem, our useFocusManagement is designed to control focusable elements within the Portal and an indicator element to link the structure between the Portal and the DOM. The problem here is that some DropDown APIs let you render elements, if you render a focusable element (e.g. Button) useFocusManagement will not compute the element in scope.

I've been thinking of making ClayDropDownContext a public API with restricted access to focusManager.createScope so that I can add to focusable elements within the Portal. Maybe we can use this same approach for DropDown's low-level component, so people don't have to create their own implementation.

const {focusManager} = useContext(ClayDropDownContext);

<ClayDropDownWithItems
	caption="Showing 7 of 203 Structures"
	footerContent={
		<>
			<ClayButton ref={ref => focusManager.createScope(ref, 'bCancel', true)} displayType="secondary">
				{'Cancel'}
			</ClayButton>
			<ClayButton ref={ref => focusManager.createScope(ref, 'dCancel', true)}>{'Done'}</ClayButton>
		</>
	}
	helpText="You can customize this menu or see all you have by pressing 'more'."
	items={items}
	spritemap={spritemap}
	trigger={<ClayButton>{'Click Me'}</ClayButton>}
/>

coveralls · 2019-09-03T13:52:35Z

Pull Request Test Coverage Report for Build 3431

35 of 131 (26.72%) changed or added relevant lines in 10 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 79.067%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-drop-down/src/DropDownWithItems.tsx	7	8	87.5%
packages/clay-shared/src/FocusScope.tsx	0	1	0.0%
packages/clay-form/src/Checkbox.tsx	7	9	77.78%
packages/clay-shared/src/useFocusManagement.ts	2	94	2.13%

Files with Coverage Reduction	New Missed Lines	%
packages/clay-shared/src/useFocusManagement.ts	1	2.3%

Totals
Change from base Build 3428:	-0.08%
Covered Lines:	2092
Relevant Lines:	2473

💛 - Coveralls

bryceosterhaus

Since this context is specifically concerned about the focusManger, what do you think about just making is FocusManagerContext?

Also, I'm a little worried we are making our focus management overly complex. At the moment I'm not totally sure how to remedy that, but I think its something we should keep an eye on. I wouldn't want consumers of Clay have to worry about it as much as we do or implement anything overly complex on their side just to get it to work.

matuzalemsteles · 2019-09-03T18:40:42Z

Since this context is specifically concerned about the focusManger, what do you think about just making is FocusManagerContext?

Hm, I just worry that only FocusManagerContext has the impression that it is not just DropDown. Maybe DropDownFocusManagerContext.

Also, I'm a little worried we are making our focus management overly complex. At the moment I'm not totally sure how to remedy that, but I think its something we should keep an eye on. I wouldn't want consumers of Clay have to worry about it as much as we do or implement anything overly complex on their side just to get it to work.

Hmm yeah, I'm worried about that too. I see that it seems easier to expose useFocusManagement for people to handle it in low-level components, but I think we should hold this a little longer.

I'm wondering how I could change the rule of not having to use createScope, maybe go through the children tree if I can and get the focusable elements...

But dealing with focus is rather complex especially for this case that is a manager, its logic becomes complex because it has many use cases. I will try to work on it to simplify things a bit. Since we are doing something "outside of React" and not in the core, we missed a few things, especially for the components that are Portals, which makes things more complex here.

bryceosterhaus · 2019-09-03T19:00:58Z

Um, I just worry that only FocusManagerContext has the impression that it is not just DropDown.

Could you expand on this a bit? Not totally sure what you mean. My concern is that by adding additional components or contexts that are outside of FocusManager, our footprint for maintain gets larger and slowly becomes harder to get rid of the focus manager component when React implements it into its API. If we can contain it to just the focus manager package, it will be easier to maintain and upgrade in the long run.

matuzalemsteles · 2019-09-03T19:14:50Z

Sorry, that was not very clear. But I'm just saying that creating a FocusManagerContext will have the impression that it can be used anywhere besides DropDown, maybe that's what you mean. But the focusManager instance has to be from DropDown useFocusManagement only, so you can create the scope. In other words, it will be restricted to use within DropDown only.

If we can contain it to just the focus manager package, it will be easier to maintain and upgrade in the long run.

You mean creating a package just to deal with focus management?

My concern is that by adding additional components or contexts that are outside of FocusManager, our footprint for maintain gets larger and slowly becomes harder to get rid of the focus manager component when React implements it into its API

Yes you are right about that, if we do that we will compromise maintenance and will have to deal with the depreciation of this when React implements its API.

I think it is better to on hold this decision to expose the context and I will try to eliminate the use of createScope to keep it internal.

bryceosterhaus · 2019-09-03T19:19:39Z

Oh sorry I didnt mean package, I mean the FocusManager component.

Maybe instead of restricting it to DropDown, we can just create a generic context for focus that would allow for nested structures like dropdown? Im sure this isnt the only place we will run into focus issues.

matuzalemsteles · 2019-09-03T21:31:09Z

Hmm, I'm not sure if it would be necessary to create a Context for this, most of the cases where we want to control focus is when a component uses React.Portal, and we only have this case for DropDown and other components end up using it. The most recommended would be to have one instance of useFocusManagement per component so we don't have control issues.

My view on having a Generic Context is that we would only have one instance of useFocusManagement for just about any component that uses it, which may come to break the manager because FocusManager follows the command of createScope to manage focus and if any element is not in createScope, this can disrupt management.

In the end FocusManager needs to create a scope or control area for it to manage focus and look for focusable edge elements to let the browser handle the rest.

bryceosterhaus · 2019-09-03T21:53:58Z

My concern is that if we limit this specifically for DropDown, we will eventually run into again whenever we use a context. For example, any modal or popover. The more generic we can make it and non-specific to dropdown will make it easier to refactor later when we want to utilize React's API rather than our own focus manager.

matuzalemsteles · 2019-09-04T17:57:34Z

My concern is that if we limit this specifically for DropDown, we will eventually run into again whenever we use a context. For example, any modal or popover. The more generic we can make it and non-specific to dropdown will make it easier to refactor later when we want to utilize React's API rather than our own focus manager.

Hmm, I think I'm starting to understand using a generic context for useFocusManagement. Because my concern with this is overloading useFocusManagement to control components that do not need... I will try to work on it.

bryceosterhaus · 2019-09-04T18:32:15Z

@matuzalemsteles yeah thats a valid concern, we would have to craft it to be an opt-in type of functionality. Thanks for looking into it!

matuzalemsteles · 2019-09-13T01:31:43Z

hey @bryceosterhaus I just refactored the useFocusManagement hook, I took another approach instead of having createScope to mark the elements that would be part of focusManager scope, I am now consuming this information of the Fiber. It ends up removing a lot of logic that was starting to get complex and limiting our work area.

So now it goes through Fiber looking for focusable elements within the children scope of the FocusScope component, it is still necessary to add an event to out-of-scope elements so that FocusManager can control the return, in case when element is in a React.Portal. Since we no longer need to mark the element to be part of the scope it is also no longer necessary to have a FocusManagerContext approach, I still do not want to leave this as a public API...

This approach also allows the low-level ClayDropDown component to be supported by FocusManager.

Another thing to note is that we are consuming Fiber's direct information this means that this API is a bit risky but I find React difficult to change the approach, if that changes it is likely to be a paradigm shift so I'm sure go with it.

The FocusManager API PR in React can be set aside but has a similar new specification being commanded by the React team that can be delivered with the new event engine... facebook/react#16009

bryceosterhaus · 2019-09-16T08:20:16Z

@matuzalemsteles I was just looking at our storybook and it looks like this doesn't work for dropdown?

Also, I really just don't know if this is the best way forward for us, not necessarily because I disagree but more just ignorance of not knowing what the best implementation is for our needs. I really don't have much knowledge of working with React internals fibers but it does worry me at first glance as being overly complex. I would be curious to get @wincent's thoughts and see what he thinks about accessing internals like this.

wincent · 2019-09-16T09:42:34Z

I would be curious to get @wincent's thoughts and see what he thinks about accessing internals like this.

Reaching into internals is definitely a liability that may come back to bite us in the future. It would be a shame if we wanted to move to a new version of React but found ourselves blocked because Clay was depending on an internal implementation detail in a previous version.

I don't have enough context to know whether there is another way to address #2357 — in this PR it isn't clear which bit is fixing that issue...

When we deprecated ClayDropDownWithBasicItems to ClayDropDownWithItems, we didn't add FocusScope and useFocusManagement this causes a regression on all components that use.

... and which bit is just refactoring. @matuzalemsteles: is there a more minimal approach that we could take while waiting for those upstream React changes to go in? I know we don't know when they'll go in, but we need to balance the risk being that they never go in against the risk of reinventing a pretty complex wheel.

matuzalemsteles · 2019-09-16T15:06:13Z

@matuzalemsteles I was just looking at our storybook and it looks like this doesn't work for dropdown?

Oh sorry guys, I noticed on Friday that I pushed something that broke the netlify CI to run the builds, I will try to force a new build for this PR, locally the Storybook with these changes work fine.

Reaching into internals is definitely a liability that may come back to bite us in the future. It would be a shame if we wanted to move to a new version of React but found ourselves blocked because Clay was depending on an internal implementation detail in a previous version.

Yeah I thought about it and it will be a risk we can take, I think if any change in this structure comes up it won't be a big problem because the information I'm consuming is the most crucial thing for React: go back and forth about the tree. And because they're based on Fiber, it's likely that big changes that hurt this aren't a problem for now until the new APIs for React's FocusManager arrive.

I don't have enough context to know whether there is another way to address #2357 — in this PR it isn't clear which bit is fixing that issue...

Focus control for DropDown was added in commit 20c388f... And actually I rewrite all the useFocusManagement so I am not sure what you mean by that...

is there a more minimal approach that we could take while waiting for those upstream React changes to go in?

That was the most ideal approach I found and less complex than before so I don't have to touch DOM directly, so it covers just about every use case, a Component in React.Portal, coming back with shift + tab for the Portal element, look for the out-of-scope focusable focus element, deal with tree changes... So I don't know how I could cover all the cases without controlling the focus of the elements, it's pretty generic because it cannot infer when the element is inside the portal or not, inferring this can break other use cases and can create more complexity as before.

Just to highlight the use of FocusManager here is to solve the problem of focus on elements within React.Portal... When there is no such element in our components we do not need FocusManager. Because we use React.Portal a lot, things start to get complex especially when they come together, like the cases of a DatePicker inside a DropDown or all this inside a Modal...

…se of createScope The `useFocusManagement` hook rewrite allows to remove the manual tagging API from items that should be added to the scope. Instead of consuming the DOM directly, now it traverse the Fiber for get focal elements, the Fiber is safer than the DOM since the use of `React.Portal` influences the tree structure and the fiber maintains the tree.

matuzalemsteles · 2019-09-17T00:04:24Z

Just a note about this, we may really have to look better at this approach to consuming Fiber information, it seems that the _owner of children works differently in the production environment, he comes sometimes null. I'm trying to think of how we could do something smarter but just what comes to mind is getting this information from the DOM but then we have the problem with React.Portal that the tree structure of the DOM is different, and I wanted to eliminate any manual marking on the elements to create the scope, any ideas?

I will try to look more tomorrow if I can understand this behavior of _owner.

matuzalemsteles · 2019-09-17T00:17:53Z

I've been looking a bit at the API that will be implemented in React, it looks like there will be a manual tagging with the ReactDOM.createAccessibleComponent API. Maybe we can draw scope creation from a Focusable component that will communicate with the nearest FocusScope context that deals with the focus of that scope.

So something like this:

<Focusable>
    <ClayButton tabIndex={-1}></ClayButton>
</Focusable>

And we would have to expose Focusable and perhaps FocusScope as a public API in its own package.

This is a pseudocode that React is thinking:

const FocusableDiv = ReactDOM.createAccessibleComponent((props, focusable) => {
  return <div tabIndex={focusable ? 0 : -1} {...props} />;
});

//...
<FocusScope onMount={() => focusManager.focusFirst()}>
  <FocusableDiv focusable={true} />
  <FocusableDiv focusable={true} />
  <div tabIndex={0}>You can't focus this</div>
</FocusScope>

bryceosterhaus · 2019-09-17T16:23:27Z

So I went back to the heart of the issue and was trying to understand it a bit better since I'm not fully convinced I know what proper focusing even looks like. But i was demoing one of the storybooks and didn't actually see the issue at hand. In this story I am able to focus throughout, even the user defined buttons at the bottom. Am I missing something with that example?

matuzalemsteles · 2019-09-17T17:37:38Z

So I went back to the heart of the issue and was trying to understand it a bit better since I'm not fully convinced I know what proper focusing even looks like. But i was demoing one of the storybooks and didn't actually see the issue at hand. In this story I am able to focus throughout, even the user defined buttons at the bottom. Am I missing something with that example?

Oh yeah, in this particular example you won't be able to see the problem because components are rendered within an iframe, so its order is being respected even if DropDown is rendered in the body, you can test this by adding focusable elements (ClayButton) with DropDown's sibling, so you'll see the problems.

Other examples like MultiStepNav, Pagination... are more visible and they become more visible in the application.

This is the current behavior without a focus manager:

With the focus manager:

The problem gets bigger when you start having many components like this on the same screen and putting it all together into one Modal...

bryceosterhaus · 2019-09-18T09:56:33Z

Ah I see, that is definitely an issue. Your changes definitely make the accessibility better and more intuitive and I think the usage of the focus manager is a bit cleaner. I am still unsure about the implementation of focus manager itself, but since its inside of "shared" I am more apt to not worry about it a ton. I do like that the usage is more in line with pseudocode that react is thinking as well.

So overall, I think I would be fine with merging this and going this direction, particularly because it moves the code footprint into FocusScope and away from our components themselves.

What do you think? Do you feel comfortable with this implementation, the code itself doesnt look too concerning, but you know the nature of the react internals a bit better than I do. Are there any particular edge cases we might run into issues with this?

…FocusScope to have reference from element

matuzalemsteles · 2019-09-18T23:51:25Z

What do you think? Do you feel comfortable with this implementation, the code itself doesnt look too concerning, but you know the nature of the react internals a bit better than I do. Are there any particular edge cases we might run into issues with this?

I'm fine with consuming the information from react internals, of course it's a risk... but I'm safe because the structure is the tree, it's very difficult for them to do some rewriting of this, so it's likely to be slow and if it happens might be a breaking change in a major version of React, so I'm fine until React releases their focus manager.

I like this current implementation because we leave all the internal implementation and people don't need to know about it.

bryceosterhaus · 2019-09-19T09:45:48Z

I like this current implementation because we leave all the internal implementation and people don't need to know about it.

Yeah I agree. As long as we can keep the messy internal part within shared and create a relatively simple API for consuming, it should be okay.

I am good with pushing this forward. Any final thoughts on this @wincent?

wincent · 2019-09-19T10:17:58Z

Any final thoughts on this @wincent?

Nope. I just hope the upstream focus management gets merged/released soon so that we can reduce the exposure (to breakage) and the complexity that comes with reaching into framework internals. It's nasty, but sometimes you have to do it.

matuzalemsteles · 2019-09-19T13:56:02Z

Yeah I agree. As long as we can keep the messy internal part within shared and create a relatively simple API for consuming, it should be okay.

Yeah, we can expose an API soon and we can see how it will work.

Nope. I just hope the upstream focus management gets merged/released soon so that we can reduce the exposure (to breakage) and the complexity that comes with reaching into framework internals. It's nasty, but sometimes you have to do it.

Yes, I'm following the Focus Manager implementation, hope we can see something stable soon about it. I also agree that comes the complexity of working with internals. Hope we can remove this soon.

matuzalemsteles · 2019-09-19T14:08:25Z

Merged, approved offline

matuzalemsteles force-pushed the issue-2357 branch 2 times, most recently from 9b1f3d7 to 293e74c Compare September 3, 2019 13:47

bryceosterhaus reviewed Sep 3, 2019

View reviewed changes

matuzalemsteles force-pushed the issue-2357 branch 3 times, most recently from 3d736a1 to f1daf3a Compare September 13, 2019 01:31

matuzalemsteles changed the title ~~fix(@clayui/drop-down): add useFocusManagement to ClayDropDownWithItems~~ refactor: rewrite the useFocusManagement hook with a new approach Sep 13, 2019

matuzalemsteles force-pushed the issue-2357 branch from f1daf3a to 320255b Compare September 16, 2019 21:51

matuzalemsteles added 5 commits September 16, 2019 19:10

chore(@clayui/form): add forwardRef to Radio

529a32f

chore(@clayui/form): add forwardRef to Checkbox

191f6e0

fix(@clayui/time-picker): use FocusScope component

2a36f27

fix(@clayui/navigation): use FocusScope component

743d9c0

matuzalemsteles force-pushed the issue-2357 branch from 320255b to e80199a Compare September 16, 2019 22:10

matuzalemsteles added 3 commits September 16, 2019 19:11

fix(@clayui/form): use FocusScope component

e6f13f6

fix(@clayui/drop-down): use FocusScope component

4c79f9c

chore(@clayui/drop-down): update snapshots

cc1dab8

matuzalemsteles added 2 commits September 16, 2019 19:11

fix(@clayui/autocomplete): use FocusScope component

5e481d4

chore(@clayui/autocomplete): update snapshots

0cfd7cf

matuzalemsteles force-pushed the issue-2357 branch from e80199a to 0cfd7cf Compare September 16, 2019 22:11

fix(@clayui/shared): get the Fiber from the ref

9980545

matuzalemsteles added 3 commits September 18, 2019 19:32

fix(@clayui/autocomplete): adds React.forwardRef to Autocomplete for …

0bbde5a

…FocusScope to have reference from element

fix(@clayui/shared): add reference to children ref

38896b6

fix(@clayui/shared): add focus control by left and right arrow keys

e314419

chore(@clayui/shared): remove unused hook useKeyHandlerForList

e566d0f

matuzalemsteles merged commit a892704 into liferay:master Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rewrite the useFocusManagement hook with a new approach #2369

refactor: rewrite the useFocusManagement hook with a new approach #2369

matuzalemsteles commented Sep 2, 2019

coveralls commented Sep 3, 2019 •

edited

Loading

bryceosterhaus left a comment

matuzalemsteles commented Sep 3, 2019 •

edited

Loading

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 3, 2019

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 3, 2019

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 4, 2019

bryceosterhaus commented Sep 4, 2019

matuzalemsteles commented Sep 13, 2019

bryceosterhaus commented Sep 16, 2019

wincent commented Sep 16, 2019

matuzalemsteles commented Sep 16, 2019 •

edited

Loading

matuzalemsteles commented Sep 17, 2019 •

edited

Loading

matuzalemsteles commented Sep 17, 2019

bryceosterhaus commented Sep 17, 2019

matuzalemsteles commented Sep 17, 2019

bryceosterhaus commented Sep 18, 2019

matuzalemsteles commented Sep 18, 2019

bryceosterhaus commented Sep 19, 2019

wincent commented Sep 19, 2019

matuzalemsteles commented Sep 19, 2019

matuzalemsteles commented Sep 19, 2019

refactor: rewrite the useFocusManagement hook with a new approach #2369

refactor: rewrite the useFocusManagement hook with a new approach #2369

Conversation

matuzalemsteles commented Sep 2, 2019

coveralls commented Sep 3, 2019 • edited Loading

Pull Request Test Coverage Report for Build 3431

💛 - Coveralls

bryceosterhaus left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Sep 3, 2019 • edited Loading

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 3, 2019

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 3, 2019

bryceosterhaus commented Sep 3, 2019

matuzalemsteles commented Sep 4, 2019

bryceosterhaus commented Sep 4, 2019

matuzalemsteles commented Sep 13, 2019

bryceosterhaus commented Sep 16, 2019

wincent commented Sep 16, 2019

matuzalemsteles commented Sep 16, 2019 • edited Loading

matuzalemsteles commented Sep 17, 2019 • edited Loading

matuzalemsteles commented Sep 17, 2019

bryceosterhaus commented Sep 17, 2019

matuzalemsteles commented Sep 17, 2019

bryceosterhaus commented Sep 18, 2019

matuzalemsteles commented Sep 18, 2019

bryceosterhaus commented Sep 19, 2019

wincent commented Sep 19, 2019

matuzalemsteles commented Sep 19, 2019

matuzalemsteles commented Sep 19, 2019

coveralls commented Sep 3, 2019 •

edited

Loading

matuzalemsteles commented Sep 3, 2019 •

edited

Loading

matuzalemsteles commented Sep 16, 2019 •

edited

Loading

matuzalemsteles commented Sep 17, 2019 •

edited

Loading