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

Clickable elements below the fold in Safari

Open colebemis opened this issue 5 years ago • 17 comments

Problem

In macOS Safari, clicking links below the fold inside the FocusLock component causes focus to jump to the first link instead of going the destination of the link.

Here's a codesandbox that reproduces the issue: https://codesandbox.io/s/responsive-navigation-safari-bug-6hvj4

Notes

  • Removing the FocusLock component from the Drawer component fixes this issue.

  • Removing tabIndex="-1" from the wrapper div created by @reach/router also fixes this issue:

    image

Related to https://github.com/primer/doctocat/issues/56

colebemis avatar Aug 19 '19 23:08 colebemis

focus-lock has the following logic - if the "distance" between last focused component and the current one is "more" than 1 - move focus back to the last.

And exactly this moment is not working. Let's try to find out why

theKashey avatar Aug 20 '19 05:08 theKashey

It seems to me - this is a pure Safari issue - https://codesandbox.io/s/responsive-navigation-safari-bug-jjf1m - clicking on the button does not select it, it focuses on the parent div.

theKashey avatar Aug 20 '19 05:08 theKashey

https://codesandbox.io/s/responsive-navigation-safari-bug-jjf1m - for now the only way to fix Safari behaviour is to apply focus by yourself

// help browser to focus element you just clicked
document.addEventListener(
  "click",
  event => {
    event.target.focus();
  },
  true // to be confirmed, probably overpower.
);

theKashey avatar Aug 20 '19 14:08 theKashey

Ok, here is documentation - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus

Does clicking on a

No (even with a tabindex)

Does tapping on a

No (even with a tabindex)


So the only ways to fix the problem (for now) are:

  • don't use tabIndex on the parent, so it would not be focused, and focus would not be activated.
  • whitelist parental focus, ie allow it
  • using event handlers focus buttons and links munually, however it does not working really well, and breaking default browser behavior.

So - look like the only valid way, but still not quite cool, is .2 - https://codesandbox.io/s/responsive-navigation-safari-bug-wnwv3 the problem - no "focus guards" around that parent, so you might "tabout" it, thus escape the lock.

theKashey avatar Aug 20 '19 22:08 theKashey

@theKashey Thank you so much for the quick response! Approach #2 seems like the most reasonable solution. I'll give it a shot 😄

colebemis avatar Aug 21 '19 04:08 colebemis

You know - try to combine .2 with that "click-focus" hack I've posted above. You still need to focus the element somehow, or background element will retain the focus.

theKashey avatar Aug 21 '19 05:08 theKashey

I had the same issue and solved it this way (found this solution by someone else):

const focusLockWhiteList = el => !(el.getAttribute('role') === 'dialog' && el.tabIndex === -1);

<FocusLock whiteList={focusLockWhiteList}>

gersomvg avatar Aug 26 '19 13:08 gersomvg

@theKashey Is there a way to use the "whitelist" approach in react-focus-on? It doesn't look like FocusOn exposes a whitelist prop

colebemis avatar Aug 26 '19 23:08 colebemis

I am not sure it's the right solution for a problem.

Actually, I am thinking about a better approach:

// help browser to focus element you just clicked
document.addEventListener(
  "focus",
  event => {
    if(eventTargetContainsFocusLock) { 
      event.preventDefault(); // don't let focus to be set "trought" focus lock.
    }
  },
  true,
);

In the same time - there is already such logic - https://github.com/theKashey/react-focus-lock/blob/master/src/Trap.js#L121-L159

The problem:

without whitelist

  • the original event would be prevented, but focused element would be changed (this issue)

with white list

  • the original event would pass thought, focus lock will ignore focus change (that's not a solution)

what shall be done

  • the original even should be suspended.

where? Or what is causing the initial issue

  • focus-lock is enumerating all focusables and act depending on their relative positions.
  • the distance between last element and the new is too "far", thus focus lock is activating the first element
  • 😎focus lock shall threat "parents" in a different way. If the focus is received at parent - we shall bring it back to the last focusable element.

So - the real problem is assumtion that the "focus order" does not include your own parent. And it does not almost in all cases, except reach-router.

I think this is what shall be changed.

theKashey avatar Aug 27 '19 01:08 theKashey

Ok ^^^ that did not work. I could not preserve "focused elements" as long as they are not focusable in Safari. But I've found a solution.

https://codesandbox.io/s/responsive-navigation-safari-bug-kblg2

 <div
// remove mouse down propagation from the lock
  onMouseDown={e => e.preventDefault()}
// restore just lost "default" behaviour
  onClick={e => e.target.focus()}
>
  <FocusLock>
    ....

I would gently ask you to test this solution:

  • wrap with onMouseDown suspender
  • compensate suspension with onClick handler

theKashey avatar Aug 27 '19 11:08 theKashey

I would gently ask you to test this solution:

  • wrap with onMouseDown suspender
  • compensate suspension with onClick handler

Clicking on links below the fold works with this approach! But now tabbing and hitting enter/space doesn't work on any browser 😢 Any ideas how to fix that?

colebemis avatar Aug 27 '19 16:08 colebemis

Please double check - running the provided example in Chrome and Safari - everything works as it should.

theKashey avatar Aug 27 '19 22:08 theKashey

Oof, my bad. I was expecting the space key to work on links like it does on buttons 🤦‍♂

Your solution works great. Thank you!

colebemis avatar Aug 27 '19 23:08 colebemis

Let's keep it open unless it's properly documented, or even implemented on the focus lock side.

theKashey avatar Aug 27 '19 23:08 theKashey

https://zellwk.com/blog/inconsistent-button-behavior/

Proposed in that article fix is the same I've tried before

document.addEventListener('click', event => {
  if (event.target.matches('button') {
    event.target.focus()
  }
})

In other words - we could recomend using this hack more widely.

theKashey avatar Sep 26 '19 22:09 theKashey

I've come across an issue regarding this fix

 <div
// remove mouse down propagation from the lock
  onMouseDown={e => e.preventDefault()}
// restore just lost "default" behaviour
  onClick={e => e.target.focus()}
>
  <FocusLock>
    ....

So I added this fix to the parent div but the issue I was having was that e.preventDefault() was bubbling down to native form elements and causing them to not perform there default actions such as a select field showing its options onClick.

I tried to implement this fix but no dice

 <div
// remove mouse down propagation from the lock
  onMouseDown={e => e.preventDefault()}
// restore just lost "default" behaviour
  onClick={e => {
    if (e.target !== e.currentTarget){
      return
    }
    e.target.focus()
 }}
>
  <FocusLock>
    ....

SMccarrick avatar Jan 15 '20 14:01 SMccarrick

The only "working" solution, as described above

  • list target div in shards, ie allow it to be focused
  • or use white list to "hide" it
  • or use className to "hide" it
  • or "prevent default", which breaks more than it fixes.

No real solution today :(

Except for the way adopted by reakit - DON'T USE "BUTTONS" - see https://twitter.com/diegohaz/status/1176998100495491072

theKashey avatar Jan 16 '20 06:01 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 12:04 stale[bot]