focus-lock icon indicating copy to clipboard operation
focus-lock copied to clipboard

Focus escapes when jail contains first or last focusable elements in the document

Open raquo opened this issue 5 years ago • 5 comments

Demo:

https://codepen.io/anon/pen/VJbJeR?editors=1011

In this example, focus jail is turned on, but does not work at all.

  • If you uncomment the first commented out <input>, focus will be jailed successfully when shift-tabbing.
  • If you uncomment the last commented out <input>, focus will be jailed successfully when tabbing.
  • The presence of those inputs should not be required, as it can not be relied upon.

This is a problem in a real application too, not just in CodePen. In a standalone application the escaped focus moves into browser navigation bar / tabs, which shouldn't happen.

I observed this using your dom-focus-lock project, but from what I can tell this is more likely a general focus-lock problem, so I'm filing the issue here.

raquo avatar Jun 24 '19 09:06 raquo

Sorry, but this is not the bug, but a (useful) feature. Sometimes you need to allow moving to browser chrome. In the same time - there is no way to prevent it, except:

  • adding keypress handler to handle tab manually. I don't quite like the idea.
  • adding "guards" around the Lock, and this is how it's working all this time for react- and vue- locks

I can tell this is more likely a general focus-lock problem, so I'm filing the issue here.

You were right, and this is how this tool is designed. And even "why" this library was created. You might try focus-trap, which might better fit your needs.

theKashey avatar Jun 24 '19 21:06 theKashey

Sorry, but this is not the bug, but a (useful) feature. Sometimes you need to allow moving to browser chrome.

That's really not a good excuse... The jail should always work, as developers can always disable it if/when they don't want it. Some random implementation detail forcing this unexpected jail failure on developers is not a feature.

If this implementation detail didn't exist, you wouldn't deliberately write code making the jail fail to contain the focus when it happens to contain the first or last focusable elements in the document, because obviously it's not actually a useful feature that should exist in this form.

If you don't want to implement guards or listen to tabs in dom-focus-lock, you should at the very least acknowledge this behaviour in https://github.com/theKashey/dom-focus-lock#behavior.

raquo avatar Jun 25 '19 01:06 raquo

If this implementation detail didn't exist, you wouldn't deliberately write code making the jail fail to contain the focus when it happens to contain the first or last focusable elements in the document, because obviously it's not actually a useful feature that should exist in this form.

The real problem is not so obvious - guards are expected to be placed around the lock, not around the document. Without then tab could lead to the page scroll, if following focusable element is located in another place.

The best solution for you would be to add these guards in the run time, on lock activation, and remove them later.

const hiddenGuard = {
  width: '1px',
  height: '0px',
  padding: 0,
  overflow: 'hidden',
  position: 'fixed',
  top: '1px',
  left: '1px',
};

const firstGuard = document.createElement('div');
firstGuard.tabIndex = 0;
Object.assign(firstGuard.style, hiddenGuard);

node.parentNode.insertBefore(firstGuard, node);
node.parentNode.insertBefore(lastGuard, node.nextSibling);

...
firstGuard.parentNode.removeChild(firstGuard);

But that would be always acceptable. I should think about it.

theKashey avatar Jun 25 '19 02:06 theKashey

Hrm, if guards are needed for every jail as opposed to the whole document, it's probably best to just document this problem and solution in dom-focus-lock, and let developers create the guards.

raquo avatar Jun 25 '19 03:06 raquo

They should not be active(tabbable) unless lock is active. That's already a constraint.

theKashey avatar Jun 25 '19 04:06 theKashey

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.

stale[bot] avatar Apr 30 '23 08:04 stale[bot]