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

RFC: Focus Management API #109

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@devongovett
Copy link

devongovett commented Feb 27, 2019

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 added some commits Feb 27, 2019


## FocusManager

In addition to defining the focus scopes in an application, components need an API to programmatically move focus around. The singleton `FocusManager` API exported by `react-dom` provides this interface.

This comment has been minimized.

@j-f1

j-f1 Feb 27, 2019

How would this work with multiple React roots on the page?

This comment has been minimized.

@devongovett

devongovett Feb 27, 2019

Author

Only one root can have focus at a time. The FocusManager APIs would apply to the root (and scope) which currently has focus.

@tjallingt

This comment has been minimized.

Copy link

tjallingt commented Feb 27, 2019

Wonderful, I love how this is making accessibility more accessible 😄

Not sure if this is in the scope of this RFC but I feel like FocusManager is missing a way to move focus around a grid-like ui (left/right/up/down) such as a table. As the RFC stands it seems the solution would be that you make each row a separate focus group but then you need to tab over rows and it will automatically select the first column every time. Not sure what the accessibility guidelines are for this use case though so maybe that is what works best on screen readers etc.

Perhaps the FocusManager methods could be changed to focusNext(moveForwardBy: number) and focusPrevious(moveBackwardsBy: number) so focus can "jump" a certain number of elements. Wrapping every row in a FocusScope you would be able to do:

FocusManager.focusNextTabStop();
FocusManager.focusNext(currentColumnIndex);
@theKashey

This comment has been minimized.

Copy link

theKashey commented Feb 27, 2019

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.
@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Feb 27, 2019

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.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Feb 28, 2019

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.

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Feb 28, 2019

I mean - component and everything below.

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

@AlmeroSteyn

This comment has been minimized.

Copy link

AlmeroSteyn commented Feb 28, 2019

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?

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Feb 28, 2019

@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.

@AlmeroSteyn

This comment has been minimized.

Copy link

AlmeroSteyn commented Mar 1, 2019

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?

@sylvhama

This comment has been minimized.

Copy link

sylvhama commented Mar 1, 2019

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

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Mar 1, 2019

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.

@bryanrsmith bryanrsmith referenced this pull request Mar 1, 2019

Open

[RFD] Dropdown Menu #141

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 2, 2019

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.

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Mar 2, 2019

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.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 2, 2019

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!

@AlmeroSteyn

This comment has been minimized.

Copy link

AlmeroSteyn commented Mar 2, 2019

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

@jquense

This comment has been minimized.

Copy link

jquense commented Mar 2, 2019

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

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Mar 2, 2019

@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.

@jquense

This comment has been minimized.

Copy link

jquense commented Mar 2, 2019

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!

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Mar 2, 2019

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?

@jquense

This comment has been minimized.

Copy link

jquense commented Mar 2, 2019

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.

@AlmeroSteyn

This comment has been minimized.

Copy link

AlmeroSteyn commented Mar 2, 2019

@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.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 2, 2019

@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.

@devongovett

This comment has been minimized.

Copy link
Author

devongovett commented Mar 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.