rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

RFC: Focus Management API

Open devongovett opened this issue 6 years ago • 46 comments

This RFC proposes adding a builtin FocusScope component and FocusManager API to react-dom, along with internal logic to properly manage focus in React applications. Along with focus scopes declared in the application, it will take into consideration portals and other React concepts which may cause the React tree and the DOM tree to differ in relation to focus management.

Related discussion: #104.

View Rendered Text

devongovett avatar Feb 27 '19 18:02 devongovett

  • https://github.com/reactjs/reactjs.org/pull/1601
  • https://github.com/facebook/react/issues/14540

You are mixing 3 separated things

  • Trapping focus (which, so far could be implemented in user space), with return focus, but without autofocus.
  • Controlling, which is not quite "component" friendly. Focus Manager should not allow controlling focus outside of a current component, and should provide API to move a focus into the current element. Plus the difference between focusNext and focusNextTabStop isn't clear. I am not sure this API should be exposed, at least in the beginning.
  • Traversing. Ie managing portals and digging some information from a React Tree. This should be a separate RFC, as long as there are other tasks, like a cousin ScrollLock, which requires the same structure to traverse.

theKashey avatar Feb 27 '19 21:02 theKashey

I think all of these are related, and should be considered together.

Trapping focus can kind of be implemented in user space, but not fully. It's not possible to support portals correctly.

Controlling focus works within the scope, not the component. In a compound component like a list or table, you might want to handle keyboard events at the top and move focus in response. That might be several levels deep rather than a direct child of the component (e.g. within a row and cell). I think it would be inconvenient and not much better than we have now to have to do that focus marshalling manually.

The difference between focusNext and focusNextTabStop is that focusNext focuses the next focusable element in the scope, whereas focusNextTabStop focuses the next tabbable element in the nearest locked scope (or the root scope). There are definitions of those terms in the RFC. I'm open to better naming and methods for the FocusManager API though, as long as both options are possible.

Traversing is quite related to all of this as well. I suppose it could be separated but it would end up being implemented together so I thought it would be better to consider all of this holistically.

devongovett avatar Feb 27 '19 22:02 devongovett

Trapping focus can kind of be implemented in user space, but not fully. It's not possible to support portals correctly.

Possible, but might require deeper integration into the browser (read - hook on Tab key)

Controlling focus works within the scope, not the component.

I mean - component and everything below. Not above. For example - "focusNext" on the last element should move focus to the first element of the current scope, not to the "next" in the document scope. As you written - Once focus is inside the scope, components can use the FocusManager API to programmatically move focus within the scope.

The difference between focusNext and focusNextTabStop is that focusNext focuses the next focusable element in the scope, whereas focusNextTabStop focuses the next tabbable element in the nearest locked scope (or the root scope).

Probably we shall not rely on tab index here, and use some other abstraction. For example - in Safary Links and Buttons are Tab and Option-Tab focusables. It's like two different groups of tabbables, and we, probably, should not affect browser behavior (just imagine - there is no React on the page)

Traversing is quite related to all of this as well. I suppose it could be separated but it would end up being implemented together so I thought it would be better to consider all of this holistically.

Travesing is needed for other tasks, and should be considered in a broader scope.

theKashey avatar Feb 28 '19 02:02 theKashey

I mean - component and everything below.

Right, that's defined by rendering a FocusScope. And yes, focusNext cycles within the current scope, not outside.

devongovett avatar Feb 28 '19 03:02 devongovett

Read through the RFC and great to read something that is so detailed and well thought out.

There are some things I really think will help. As I read, the following questions came up. Some of this I have mentioned before but for clarity will mention it again here.

Focus trapping

For me the idea of focus trapping is pulled broader in the RFC than it should. Stating the obvious, but focus trapping is the keyboard equivalent of not being able to click on anything else for a mouse user. If the keyboard gets trapped while mouse and touch users can still navigate other elements, it is no longer inclusive design.

So it seems reasonable that when focus is trapped, there is one container trapping focus and it is modal, i.e. any user cannot interact with anything else on the screen aside from what is in the trapped container.

Even if we Inception all the things and have modals inside modals, only the current active group of elements should be navigable.

Setting tabIndex="-1" on everything that falls outside the focus trap will potentially create partial keyboard traps for keyboard users as other users can still freely interact with everything. Instead one would typically create an overlay so that mouse and touch users can't interact with the background. And we then need to add aria-hidden to the parts that do not form part of the focus trap for screen reader users.

This is still an imperative task. So I feel that the RFC is helping us halfway here while still leaving some imperative tasks to the user. This makes me wonder of it should not form part of the focus trap implementation.

Auto focus on unmount

I don't think that React should automatically set focus to the previous FocusScope when a current one unmounts. As @theKashey mentioned above, I think it should return the suggested focusable leaving it up to the developer to set focus or not. Otherwise it will probably create focus race conditions in cases where the predicted focus is not correct. Or in cases where that element no longer exists. For example if you opened a modal from a menu item and the menu auto-collapsed.

I do not think that outside of handling focus-setting events, React should ever do the actual focus setting. Leave that to the implementing developer.

Positive tabIndex

Although I understand why you propose using this, I would plead for another way.

As you state: it could be controversial. In fact this practice is strongly condemned in every accessibility document that deals with this subject matter. It is also heavily validated against in automated accessibility tools: You already mentioned aXe-core , but also in others, including our test API. This means that React applications will suddenly start creating unfixable a11y findings in audits. Which would be a first and therefore odd coming from an RFC that aims to increase accessibility.

On top of this, developers taking a cue from React on how to implement their own things could start implementing positive tabIndex elsewhere.

I believe that React should either be impartial to how a11y features are implemented or, when it does become opinionated, follow the specifications and recommendations of the a11y community.

I also share the concern of what it would do in mixed-mode applications.

When it comes to implementation of the positive tabIndex I did a quick test with Firefox and NVDA.

In a barebones create-react-app application I did the following:

 <div className="App">
        <label htmlFor="input1">Input 1</label>
        <input id="input1" tabIndex="1" />
        <label htmlFor="input3">Input 3</label>
        <input id="input3" tabIndex="3" />
        <label htmlFor="input2">Input 2</label>
        <input id="input2" tabIndex="2" />
        <label htmlFor="input4">Input 4</label>
        <input id="input4" tabIndex="4" />
      </div>

With NVDA on, the keyboard interaction in Focus Mode followed the tabIndex, as expected. In Browse Mode, however, the tabIndex order is ignored again and the elements are read out as they are encountered in the DOM.

Also, when opening the rotor the elements are listed based on their DOM position and not their tabIndex:

Elements list in NVDA

As a sighted user I am a mediocre screen reader tester at best, but I cannot see the benefits of autocalculating tabIndex over just repairing focus. Am I missing something here?

AlmeroSteyn avatar Feb 28 '19 08:02 AlmeroSteyn

@AlmeroSteyn thanks for your insightful feedback! Some responses below.

Setting tabIndex="-1" on everything that falls outside the focus trap will potentially create partial keyboard traps for keyboard users as other users can still freely interact with everything.

Yeah if used incorrectly. However, everything can be used incorrectly. I'm basically proposing react set the equivalent of inert on elements outside the locked focus scope. It's still on authors of e.g. dialog components to add a backdrop to deal with clicks etc.

I don't think that React should automatically set focus to the previous FocusScope when a current one unmounts.

You're right that I left out some details of this. What happens if the previously focused element doesn't exist anymore? Sometimes we cannot know where focus should go automatically. However, I think it's a reasonable default. Perhaps we could support onFocus on the FocusScope itself, which would allow the user to do something custom when the scope receives focus, like re-target focus to a different element.

For your menu case, I think what would actually happen is that when the menu closes, its focus scope would unmount and restore focus to the target that opened the menu (e.g. button). When the dialog unmounted, it would restore focus to the button. So it might still just work by default in that case. But you're right that there will always be cases where we cannot predict the correct item, and for those we'd need a hook to let the user override.

With NVDA on, the keyboard interaction in Focus Mode followed the tabIndex, as expected. In Browse Mode, however, the tabIndex order is ignored again and the elements are read out as they are encountered in the DOM.

That's interesting information. I am very aware of the issues with positive tabIndex and hesitated strongly about suggesting it as an implementation strategy. I was worried that reimplementing browser behavior by overriding the Tab key would be worse. We could maybe handle the tab key correctly, but perhaps it works differently across platforms or browsers or something.

Also, what about hardware that doesn't have a tab key, like touch screen devices, or game consoles? In those cases, tabIndex may be the only way to expose the information about tab order that we know to the platform.

Positive tabIndex would only be needed in a very narrow case: when portals are used, and while focus in inside a React root. That should work around the issue with mixed-mode applications as you say.

That said, if there is a better way of implementing this that I'm just unaware of, please let me know.

devongovett avatar Feb 28 '19 17:02 devongovett

Yeah if used incorrectly. However, everything can be used incorrectly.

Granted! As long as any changes React make to the elements give an inclusive result then all good.

You're right that I left out some details of this. What happens if the previously focused element doesn't exist anymore? Sometimes we cannot know where focus should go automatically. However, I think it's a reasonable default. Perhaps we could support onFocus on the FocusScope itself, which would allow the user to do something custom when the scope receives focus, like re-target focus to a different element.

I like the idea in the last sentence here. An event that gives the use the ability to do something with it.

I would still not want React to decide for me when and where to go set focus, but rather have React give me the tools so I can easily do it. If I have an event indicating that focus should be set and something else (FocusManager) that gives me the tools to easily determine what the expected default is, I am in the position to use it or not. This sounds like a good default to me.

Also, what about hardware that doesn't have a tab key, like touch screen devices, or game consoles? In those cases, tabIndex may be the only way to expose the information about tab order that we know to the platform.

Should we not look at it as the effect caused by moving focus. Whether you use Tab or arrows or swipe to move focus is inconsequential, rather that the result is correct.

So I have some event that tells me focus is about to move and I have a destination I need that focus to go to which may be different from the next in the DOM order and then I act.

So in a modal, activating the button that opens it would be the event trigger to move focus into the modal and the close action of the modal would the trigger to return focus to the controlling element. I don't see that it matters where this action comes from, the fix is to repair focus when it is disturbed.

I would expect this to still work in portals?

That said, if there is a better way of implementing this that I'm just unaware of, please let me know. I was worried that reimplementing browser behavior by overriding the Tab key would be worse.

For fairness I confirmed my suspicions with our own auditors at Tenon before answering. Out of that discussion a pretty real world use case came up: If you set positive tabIndex it does not stop a screen reader user from quickly navigating to other areas of the page, unless the entire thing is aria-hidden="true". This user would then expect the taborder to continue from this place, however, positive tabIndex could likely hijack that and throw the user back to where they wanted to navigate away from. This would be the case for any non-modal portal.

Not to mention that React will need to keep track of all managed and unmanaged (parts that may not be coded in React) tabIndexes on the entire page to calculate an order that would work. Otherwise very unexpected results could arise.

Based on these kind of unexpected behaviours sites that use positive tabIndex will more than likely not pass audits. Which, as I mentioned before, would be the first time that something internal to React fails a11y audits. If React starts doing things that causes unfixable audit findings this could mean that React gets dropped from projects where this required.

So I am really excited about some of the features in this rfc but it really needs to be solved without tabIndex :-(

That said, if there is a better way of implementing this that I'm just unaware of, please let me know.

Just from my experience with refs and focus management I think aiming to make repairing focus easier with something like FocusManager could be enough.

As mentioned above I cannot yet see why this would not work for portals.

Perhaps a first pass of the rfc should focus on tools making it easier for the developer while leaving the actual actionables to the developer. Once that is nailed down and working it would be easier to crystalize out the bits where React could potentially do a little more. What do you think?

AlmeroSteyn avatar Mar 01 '19 14:03 AlmeroSteyn

At Ubisoft we have developed a simple Focus Manager to handle gamepad navigation in our SPA used on X1 and PS4. Sadly I've not succeeded yet to open source our solution. I will look at your RFC and try to help as much as I can. You can have a look to a pres I've made with some code previews: https://github.com/sylvhama/bringing-the-www-to-the-aaa

sylvhama avatar Mar 01 '19 16:03 sylvhama

I would still not want React to decide for me when and where to go set focus, but rather have React give me the tools so I can easily do it.

The point of this RFC is so we can stop manually managing focus imperatively, and just declare what we want to happen. I agree that we might need manual overrides for some edge cases, but I think we can come up with a good default that works automatically for most uses.

If you set positive tabIndex it does not stop a screen reader user from quickly navigating to other areas of the page

That's fine, and expected. We will be setting tabIndex="-1"/aria-hidden/inert on everything that shouldn't be focused (outside the locked focus scope), so the screen reader will ignore those elements. Positive tabIndex will be set only for elements that can be focused.

Just from my experience with refs and focus management I think aiming to make repairing focus easier with something like FocusManager could be enough.

Sure if you are just repairing focus, but that's not possible without some context about the user's intent. In the portals example in the RFC without the positive tab indexes, focus would go from input 1, input 3, input 2. If you wanted to repair that, when input 3 focused, you could instead move focus to input 2. However, you only want to do that if it came from the tab key or some other focus movement interaction. You don't want to repair it if the user directly clicked or tapped on input 3.

So, we need some info on the user's intent. The tab key is just one of those, but I don't think it's the only one. From @sylvhama's presentation, it looks like game consoles move focus around using some non-standard key codes. I'm not sure if mobile browsers with previous/next buttons to move focus fire keyboard events or not. And there are probably more. The point is that we can't reproduce this reliably in JS. It has to be done by the browser. And the only way to expose information about the correct tab order to the browser as far as I know is via positive tabIndex.

We can perhaps prototype this and test it out across platforms/browsers/screen readers, and see how bad it is. If it's just causing issues with audits, then those audits should probably be improved. Blanket banning a browser feature because it can be used incorrectly doesn't seem very productive. If it is causing actual issues with usability, and we determine that it isn't possible to implement the desired behavior, than perhaps we will need to drop that part of the RFC.

devongovett avatar Mar 01 '19 17:03 devongovett

It sounds like - forget about the Tab key. In the react-focus-lock I am not emulating the Tab, but just observing focus behavior:

  • onfocus and onblur events could trigger a check to return a focus where it should be
  • there are a special markers before and after lock, to track sequential tabbing - you are tabbing into the "trap" and it trigger a check
  • the same traps could be placed around portals(drop downs) - div focusable divs (add tabindex) - once focus it's looking for a portal inside and moving focus to a distant node

So - it has nothing with emulating focusing, playing with tabindexes and intercepting browser behavior - just observing the focus related events, and nothing more.

theKashey avatar Mar 02 '19 02:03 theKashey

the same traps could be placed around portals

Yeah that's possible, but I don't think it's a good idea for react to be inserting extra DOM nodes for you that you don't render. That could potentially mess up things like CSS selectors that depend on the structure you actually render.

devongovett avatar Mar 02 '19 03:03 devongovett

So - there are three options:

  • tab in the browser defined order (portal last). This is how it works today.
  • override browser behavior for tab (add here gamepads and any other system). That's not so bad, but very fragile.
  • team up with a browser, and probably it's the best way.

Why it would not lead to the issue you've pointed on - because any nested node would not exist - it's portaled! But that would mean - if you want you portal to be "transparently tabbable" - you should wrap it with some special markup - ReactDOM.forwardPortal, which may create an invisible, not tabbable node, just to catch a user focus. Even more - that invisible node might contain a reference to a portal destination, to let other tools teleport without use of React internals.

const forwardPortal = (children, targetNode) => (
  <div data-portal={targetNode} styles={portalTrapStyles}>
     {ReactDOM.createPortal(children, targetNode)}
  </div>
)

Done! In a user space!

theKashey avatar Mar 02 '19 05:03 theKashey

That's fine, and expected. We will be setting tabIndex="-1"/aria-hidden/inert on everything that shouldn't be focused (outside the locked focus scope), so the screen reader will ignore those elements. Positive tabIndex will be set only for elements that can be focused.

A locked focus scope should be the result of a user action. They should never just occur during normal navigation as this wil be a serious a11y violation in itself. So a user activates some trigger. This is modal behaviour and setting the rest as inert/aria-hidden="false" supports that. So if you want React to do this all it will need to be aware of the trigger anyways. Once the modal closes that is another event. In both cases focus can be repaired.

There are also cases where the popup is not modal. A menu is a perfect example. One simply cannot make the rest of the application inert just because a menu is open. It would be as good as placing the menu on a new page and navigating there for screen reader users. So just like every other user, the screen reader user could and should be able to interact with the rest of the page. If they decide to jump to an ARIA landmark such as a <nav> at the top of the page or a <footer> at the bottom, the positive tabIndex will probably mean that the user's next Tab is hijacked and pulled into the menu again.

A solution based on repairing focus may require more intervention from the developer, sure, but it is 100% guaranteed not to suffer from this. I like so much of what this RFC suggests that the support it would give me will already be such an enhancement even if I have to be the one to still do the focus setting.

We can perhaps prototype this and test it out across platforms/browsers/screen readers, and see how bad it is. If it's just causing issues with audits, then those audits should probably be improved. Blanket banning a browser feature because it can be used incorrectly doesn't seem very productive.

If I came across as advocating a blanket ban of a normal feature or saying we need to avoid this simply to please audits, I am sorry. This was not the intention. Allow me to clarify. If I say things you already know, apologies, but I think it should be on record here.

Browsers have functions that have proven to be harmful for accessibility, like the <marquee> tag. From the wealth of support, positive tabIndex appears to be another one.

Far be it from me to say that things cannot be improved. The WCAG and a11y audits are changing. In fact we just saw that with version 2.1 of the WCAG. However this document and the audits that stems from it comes from many years of testing and encompasses a huge range of combinations of users, user agents and assistive tech. It help us so that we do not have to do this testing every time ourselves. Good audits from reputable experts dig into this knowledge to assist, not to dictate.

If React becomes opinionated about something that every good accessibility source advises against, the testing base will need to be super huge. This means users with specific disabilities testing in every user agent known with every possible form of assistive tech. So it needs to cover screen readers, single switches, motion tracking, eye tracking, speech recognition... the list goes on. Once that is all tested and works for all cases that users can come up with using the new things this RFC aims to create it will be in a position to contradict the burden of evidence out there that positive tabIndex is a bad idea.

Keep in mind that right now I can use React to create a website that conforms to all accessibility requirements out there. This is an incredible strength and is because React stays impartial to controversial accessibility issues and leaves that to user land.

Some references on (positive) tabIndex: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex https://developer.paciellogroup.com/blog/2014/08/using-the-tabindex-attribute/ https://webaim.org/techniques/keyboard/tabindex https://dequeuniversity.com/rules/axe/3.0/tabindex http://www.karlgroves.com/2018/11/13/why-using-tabindex-values-greater-than-0-is-bad/ https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex

AlmeroSteyn avatar Mar 02 '19 10:03 AlmeroSteyn

I'm not sure I understand the problems with Portals and focus flow for traps. In all focus traps I've implemented you listen at the root element and check for blurs that aren't paired with a bubbled focus. This works for portals too since events bubble through the component tree not the DOM tree

jquense avatar Mar 02 '19 15:03 jquense

@AlmeroSteyn

There are also cases where the popup is not modal. A menu is a perfect example.

Yeah, and in that case you wouldn't use the lock prop on the FocusScope. Then the rest of the app wouldn't be inert.

In reply to the rest of your response, I understand the issues with positive tabIndex that you keep repeating, but no one has offered an alternative suggestion that would be able to implement what we need in a reliable way across platforms. This isn't about locking focus - that can be done without positive tab index - this is about correcting the tab order when a user tabs into a portal. If someone can come up with an alternative implementation suggestion to fix the example in the portals section of the RFC, I am all ears! But as far as I can tell, positive tabIndex is the only way.

Again, this is only necessary when a portal is present, and the user is focused within the react root. This way it shouldn't cause usability issues with the rest of the page. However, if we determine through prototyping that it does still cause problems, and there is no other way to implement proper tab ordering through portals, then we may just have to drop that part of the RFC, leave the behavior as it is today, and work with browsers to make it possible to implement reliably.

@jquense

I'm not sure I understand the problems with Portals and focus flow for traps.

Focus flow in portals doesn't have to do with traps at all, just in how focus flows around portals in general.

For traps specifically, you need a way to query whether an element is "inside" a FocusScope, which is why it needs to be done in react itself. Checking for blurs without a focus doesn't seem reliable - what if the user is actually blurring and not moving focus? Then you'd get a blur event without a focus, and cause focus to move even though it wasn't the user's intent.

devongovett avatar Mar 02 '19 19:03 devongovett

Checking for blurs without a focus doesn't seem reliable

not to sideline the conversation on a specific thing, but it is reliable, if you are trapping focus in a particular div and you get a blur bubble up without a subsequent focus then an element outside the div was focused or focused dropped out of the window, e.g. you can't have "just a blur" focus always moves somewhere.

In any case my point was more about RFC scope, it's not obvious to me that focus traps are something React can implement better than i can already, even with portals. THat's fine too, i'm happy to not have to implement stuff, but do want to get clearish on the line between "what is possible to implement in a browser", "what is possible for React to implement", and finally "what is possible for a user of React to implement in the context of React". It helps understand which aspects are worth bike shedding vs others is all.

Awesome work tho, really happy to see this have such lively discussion!

jquense avatar Mar 02 '19 19:03 jquense

Imagine a dialog. It contains some inputs.

function Dialog() {
  return (
    <div>
      <input placeholder="input 1" />
      <input placeholder="input 2" />
    </div>
  );
}

If input 1 is focused, and a user then clicks outside either of the inputs, whether that's still somewhere within the dialog or completely outside, the input will blur and there will be no corresponding focus event. Then you'll end up moving focus back to one of the inputs, which would be wrong. Also, based on that information, how would you know which input to focus? You don't know if the user is moving forward or backward in the tab order. Do you have an example of an implementation of this working somewhere I could see?

devongovett avatar Mar 02 '19 20:03 devongovett

Then you'll end up moving focus back to one of the inputs,

I think we have different expectations of behavior for a focus trap. If you blur off an input the focus should drop to "body", in this case the outer div, not the first or last focusable item. If you tab out of the trap (forward or backwards) focus should move to the browser chrome. It should be noted that with a native dialog, blurring to nothing moves focus to the actual body (demo not in an iframe)

In any case I did confuse my various implementations :P the most common one i've employed is in react-overlay's Modal: https://react-bootstrap.github.io/react-overlays/#modals which definitely isn't perfect, and does suffer from not supporting portals b/c it listens to the document directly (something i've been meaning to fix). So i'm sympathetic here! I just often feel that the limitations polyfilling this are more browser related than not-enough-hooks-into-react related.

jquense avatar Mar 02 '19 20:03 jquense

@devongovett

In reply to the rest of your response, I understand the issues with positive tabIndex that you keep repeating, but no one has offered an alternative suggestion that would be able to implement what we need in a reliable way across platforms. This isn't about locking focus - that can be done without positive tab index - this is about correcting the tab order when a user tabs into a portal. If someone can come up with an alternative implementation suggestion to fix the example in the portals section of the RFC, I am all ears!

You are right, I have said my peace here.

As for an alternative implementation, no I don't have one and am personally not phased that as a developer I have to still do something here.

Because focus is only a part of the story. In the case of the menu I have to know to add aria-haspopup="menu" to the trigger and to add aria-expanded="true | false depending on the state of the popup (or portal in this case). Then I may want to add aria-controls to the trigger which will mean generating a unique page ID for the portal. (Although there is controversy regarding aria-controls : http://www.heydonworks.com/article/aria-controls-is-poop).

If I am coding a modal my ARIA states and properties are different again so that rules out having them set by default.

So if the FocusManager has the next focusable ready for me, using it is a small extra trouble while setting up the proper ARIA. At this stage it would already be a great help and cut out a LOT of code and refs juggling.

Stab in the dark: You mentioned the need for more declarative focus setting. What if FocusScope carries something like a focusOn prop that can take a boolean value or a function. Then I can still reasonably declaratively set focus, and can tie it to the same actions that would set my ARIA?

In cases of tabbing into an open portal, this could also possibly be solved with such a focusOn prop as the user can then define the exact parameters for entering the open portal. But IMO non modal popups that stay open should really be the exception as, unless they are displayed inline and not as a popover, they often create issues for keyboard users when they obscure other tabable elements, especially with reflow. Which oddly enough would be exacerbated if it is, in fact, so easy to automatically tab in and out of them. So, from an accessibility perspective, I consider non modal popups that stay open an edge case. Again that is not saying it's not being done but it is not intrinsically a very accessible pattern.

AlmeroSteyn avatar Mar 02 '19 20:03 AlmeroSteyn

@jquense - the problem with portals is not about catching focus/blur events from them, but about making them tabbable in the "right", and not "browser-defined" order. It's like a focus trap around drop down, which would move focus to the distant(portal) node once needed, and providing some markup-level API to make these operations be discoverable by focusNext API. Ie - could not be solved by the imperative code.

theKashey avatar Mar 02 '19 21:03 theKashey

I think we have different expectations of behavior for a focus trap.

Ah I see. The aria practices spec for modals says that focus should cycle within the modal, not go back to the browser chrome. Typically there are other keyboard shortcuts for that.

devongovett avatar Mar 02 '19 21:03 devongovett

So, we need some info on the user's intent. The tab key is just one of those, but I don't think it's the only one. From @sylvhama's presentation, it looks like game consoles move focus around using some non-standard key codes. I'm not sure if mobile browsers with previous/next buttons to move focus fire keyboard events or not. And there are probably more. The point is that we can't reproduce this reliably in JS. It has to be done by the browser. And the only way to expose information about the correct tab order to the browser as far as I know is via positive tabIndex.

We can perhaps prototype this and test it out across platforms/browsers/screen readers, and see how bad it is. If it's just causing issues with audits, then those audits should probably be improved. Blanket banning a browser feature because it can be used incorrectly doesn't seem very productive. If it is causing actual issues with usability, and we determine that it isn't possible to implement the desired behavior, than perhaps we will need to drop that part of the RFC.

@devongovett thank you for driving this feature and this discussion (hi @AlmeroSteyn !!).

A positive tab index approach gives me pause as well. Why not just avoid the collision with a browser/DOM capability and introduce a focusOrder prop?

Of course, another way to take this is to put a callback on the FocusScope that returns a data shape with information about the scopes focusable elements with a few callbacks to invoke a reaction. Something like this:

import { FocusScope } from 'react-dom';

function Dialog({ children, onFocusChange }) {
  return (
    <Portal>
      <FocusScope lock onFocusChange={onFocusChange}>
        <div className="dialog">
          {children}
        </div>
      </FocusScope>
    </Portal>
  );
}

type TFocusTarget = {
  element: HTMLElement, // or maybe a Node. It shouldn't matter to the developer.
  data: string, // data from a prop like focusManagerData, defined for custom use. Could be used to supply grid location coordinates, for example
};

function onFocusChange(event: {
  focusedTarget: TFocusTarget,
  focusedTargetPathIndex: number, // the index of the focused target in the path
  focusPath: Array<TFocusTarget>, // the linear path of all focusable elements in this scope
  moveFocusTo(target: TFocusTarget) => void, // This let's FocusManager deal with moving focus
}) {}

function App() {
  let [showDialog, setShowDialog] = useState(false);

  return (
    <div>
      <button onClick={() => setShowDialog(true)}>Open Dialog</button>
      {showDialog &&
        <Dialog onFocusChange={onFocusChange}>
          <input placeholder="input 1" />
          <input placeholder="input 2" />
          <button onClick={() => setShowDialog(false)}>Close Dialog</button>
        </Dialog>
      }
    </div>
  );
}

Then you avoid obliging developers to specify an order ahead of time (which might become incomplete or incorrect as the DOM updates).

jessebeach avatar May 03 '19 00:05 jessebeach

@jessebeach thanks for your feedback!

I'm less concerned about developers needing to specify a custom focus order to React, though that could certainly be revisited, but how React itself can expose its computed tab order to the browser. The React tree already contains enough information to compute the expected tab order by default, just by the order it is constructed. Mostly that's the same as DOM order, but takes portals into account.

The main question is how to implement that in React. The obvious way is to handle the Tab key, call preventDefault, and manually focus the next element in the computed tab order. However, I was concerned about things other than the tab key moving focus around, like the previous and next buttons on mobile, gamepads, etc., but perhaps those things could also be handled in other ways as well. I think some experimentation is needed on non-desktop browsers to determine if there is a feasible strategy to implement this in JS, or whether browser APIs would be needed.

I suppose if we additionally wanted to allow developers to override this default tab order, a focusOrder prop could work, but honestly that could create the same issues as positive tabIndex by allowing unexpected tab ordering by users.

devongovett avatar May 03 '19 02:05 devongovett

On further thought, I suppose handling the Tab key in React wouldn't be too bad. If other platforms move focus around without firing a Tab key event, React simply wouldn't detect it and the behavior would be exactly as it is today - following DOM order. For those browsers that do fire Tab key events, React would handle them and correct the tab order to match the React tree. Perhaps workarounds could be added to React for other platforms as well (e.g. non-standard key codes), but the worst case behavior wouldn't be any different than it is today. And most of the time, the React order and the DOM order will be identical - the only differences are when portals are involved.

devongovett avatar May 03 '19 03:05 devongovett

Just for related context, we recently landed a FocusScope event component and responder into the React repo for the new experimental events API (that we're testing internally at FB): https://github.com/facebook/react/blob/master/packages/react-events/src/FocusScope.js. It's not a fully finished implementation and we still want to add a bunch of features going forward (including exposing some form of controller/manager to imperatively control focus within a scope).

trueadm avatar May 03 '19 09:05 trueadm

@devongovett - relying on Tab and key pressing is a dead end. In Safari, for example Tab, and Option+Tab has different behaviour (by default), thus it's not possible to properly predict how browser behaves, and a customer, using this browser, expects. If you want a custom focus order - probably we need some semi-automatic way to handle tabIndex to "order" nodes in a way we want.

@trueadm - again - this implementation is bound to tab key management. I would propose to use sentinels as they are known in MUI, or guards in focus-lock. The logic is simple:

  • once activeElement is out of the target, or hit sentinel - determine nextElement and update a focus. Working with portals as well, if these sentinel got automatically injected into the DOM around portals.
  • if activeElement is inside - don't do anything. As a result, FocusScope perform any action only when the focus is trying to escape, not fighting with a default browser behaviour.

theKashey avatar May 03 '19 23:05 theKashey

Sentenel elements are a dead end for react core since they would require React to change the DOM structure from what the user specified. That could have strange consequences for things relying on that DOM structure, e.g. CSS selectors. Handling the tab key may be the best option we have. Perhaps we could just ignore anything except Tab and Shift+Tab and defer to the browser?

devongovett avatar May 03 '19 23:05 devongovett

That's true only if FocusScope would not require a separate dom node, and look like it does not require it. That's something not quite possible without allowance to traverse fiber, and I am not sure it's a mandatory thing. It's not a big deal to scope focus inside a real dom node, as we all were used to do it before. And, from some point of view, it's even something that shall be done, using dialog, article or section. In the same time, any portals would have "css-selectors-issue-free" top-level node by their nature.

theKashey avatar May 04 '19 00:05 theKashey

@theKashey I've already explored tracking the focus the active element, aka sentinels. It's fragile at best because you get an inconsistent behaviour where parts of the UI show focus for a frame, only for it to flicker back after you control the sentinel. What if you nest multiple FocusScopes and want the focus to propagate when leaving each FocusScope? Dealing with this using sentinels makes it far harder to handle; which is also why I think focus management shouldn't be global either (rather it should be contained per FocusScope somehow and each FocusScope should be independent from one-another). The ones in the core of React today work this already by default, by using special APIs that are baked into React's fiber system.

Ultimately, managing focus via tab is the only cross-platform applicable way of dealing with it (it's easy enough to get working with Safari too), without running into other issues. Plus it seems like this approach to apply this to other surfaces like React VR and React Native in the future.

trueadm avatar May 04 '19 07:05 trueadm

It's fragile at best because you get an inconsistent behaviour where parts of the UI show focus for a frame, only for it to flicker back after you control the sentinel.

Not sure I follow. I didn't notice any issues with this approach.

What if you nest multiple FocusScopes and want the focus to propagate when leaving each FocusScope?

In this case, you have to manage sentinels as well, but usually, only one lock is active in a single point of time.

Ultimately, managing focus via tab is the only cross-platform applicable way of dealing with it

What about splitting Focus Management and Tab Management? One part emulates focus change, and could be reimplemented in user space(to support Gamepads?), while another one just controls it.

theKashey avatar May 05 '19 06:05 theKashey