isFocusVisible is true when clicking label elements (e.g for Checkbox) inside elements with tabIndex

[peteragarclover]

🐛 Bug Report

Both useFocusRing and useFocusVisible report isFocusVisible as true when using a mouse to click a checkbox label element if the label has ancestor element with a tabIndex.

🤔 Expected Behavior

isFocusVisible should always be false when clicking with a mouse regardless of whether ancestors have a tabIndex.

😯 Current Behavior

If you click a label around a checkbox or radio input then isFocusVisible returns true. If you click the input element itself, the value is correctly false. If the label doesn't have an ancestor with a tabIndex the value is correctly false.

This behaviour exists in Chrome and FIrefox but not Safari.

💁 Possible Solution

Sorry, I'm not sure why this happens, other than finding the cause is the tabIndex (even if it's -1)

🔦 Context

The design of our app has a scrolling aside panel that can appear on the right of the screen. To scroll the main content of the page, instead of making the body scroll, I had to add scrolling to the main content element. To be able to scroll using the keyboard the main content needs to be focused (or contain a focusable element which is focused). If you click the main content and try to scroll using the keyboard, e.g. command + downarrow nothing happens. If I add a tabIndex={-1} to the main content then you can click and start scrolling. I also take advantage of main being focusable because I shift focus to main when you navigate in the SPA. Even without this, there are various other elements that use tabIndex to allow programmatic focus such as popovers. All radio and checkbox inside those elements are drawing keyboard focus when clicked with a mouse. We would prefer to not draw the focus unnecessarily.

💻 Code Sample

See Codesandbox reproduction using a Checkbox component:

https://codesandbox.io/s/serene-bird-51qhbv?file=/src/App.js:648-693

(Sorry, I didn't use the template, I created the sandbox before I raised this issue)

🌍 Your Environment

Software	Version(s)
react-aria	3.20.0
Browser	Chrome 106.0.5249.119, FIrefox 106.0
Operating System	MacOS 12.6

🧢 Your Company/Team

🕷 Tracking Issue (optional)

[snowystinger]

Thanks for the codesandbox to demonstrate. Issue is likely around here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/useFocusVisible.ts#L97 for whoever looks at this in more depth. We cover pointerdown, but the change to the faulty state happens on/after pointerup. On a checkbox,

As noted in the environment, this is only happening in Chrome and Firefox. I also expanded the events of the codesandbox a bit so we can see where focus is going https://codesandbox.io/s/jolly-grass-2wy15l?file=/src/App.js

[escapade1990]

Any updates on this?

[ayip8]

We're encountering the same focus ring issue with radio buttons and checkboxes. In our case it's happening when these components are placed inside a dialog. After finding this open issue I see that the useDialog hook is adding tabindex="-1" to the dialog container, which explains why this bug is being triggered for me.

Our implementation of radios/checkboxes/dialogs is pretty much straight from the React Aria docs, so I would expect this bug to be pretty common when placing these components inside a dialog. Just wanted to mention in case that helps with prioritization.

[aktanoff]

I've researched the reason why isFocusVisible becomes true, as @snowystinger said, problem is in handleFocusEvent method.
When we have any container with tabIndex='-1', focus event starts firing on this element, and hasEventBeforeFocus sets to false. After that focus event fires on checkbox/input/etc and we get into this if https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/useFocusVisible.ts#L97, which sets modality to 'virtual'

Maybe we should ignore tabIndex='-1' elements here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/useFocusVisible.ts#L91?

Possible solution:

function isNegativeEventTargetTabIndex(e: Event) {
    if(!(e.target instanceof Element)) {
        return;
    }

    const tabIndex = e.target.getAttribute('tabIndex');

    if(tabIndex === null) {
        return;
    }

    const numberedTabIndex = parseInt(tabIndex);
    return isNaN(numberedTabIndex) ? undefined : numberedTabIndex < 0;
}

// Additional rule to ignore the event
(isNegativeEventTargetTabIndex(e) ?? false)

[snowystinger]

Something interesting, this doesn't happen in our component
https://codesandbox.io/s/autumn-waterfall-fljg3n?file=/src/App.js

It would be worthwhile to see what the difference is before we make a code change

[aktanoff]

Something interesting, this doesn't happen in our component https://codesandbox.io/s/autumn-waterfall-fljg3n?file=/src/App.js

It would be worthwhile to see what the difference is before we make a code change

React Spectrum checkbox doesn't have this problem because it's input has styles:

position: absolute;
top: 0;
height: 100%;

and it overlaps the label

i've made an example with some another RS Components, and they also have bug:
https://codesandbox.io/s/zen-pine-mncm17?file=/src/App.js

[aktanoff]

@snowystinger, can you please have a look at the examples?

[snowystinger]

Hey, sorry for the delay. We were in release mode last week and this week we're winding down for the holidays. I took a quick look at the new examples and I see what you mean. Thank you so much for digging into that more. We likely will not get a chance to look at this until the new year now.

[aktanoff]

@snowystinger, hello again!
could you please take another look at this problem and examples?

[snowystinger]

Sorry for the delay and thanks for the reminder.
Looks like Safari is firing an extra click event on the label element itself, which is why it is not affected by this. I am not sure which browser is actually misbehaving in this case. MDN just says "When a user clicks or touches/taps a label, the browser passes the focus to its associated input (the resulting event is also raised for the input)".

Here is the difference in events, left is chrome, right is Safari:

Unfortunately, it appears that the unit testing environment we use is also unaffected by this, so I can't really test the fix in an automated way. However, I've added the code based on your helpful suggestion above and a story to cover the case to a PR #3914

Thanks for all the research you did on this.

[snowystinger]

Unfortunately, the code is failing a test. I'll have to look into it more.
If anyone gets to this before me, here's the failing test

  ● useDraggableCollection › screen reader › should support row actions

    expect(element).not.toHaveAttribute("aria-describedby") // element.hasAttribute("aria-describedby")

    Expected the element not to have attribute:
      aria-describedby
    Received:
      aria-describedby="react-aria-description-45"

      1131 |
      1132 |       fireEvent.focus(options[0]);
    > 1133 |       expect(options[0]).not.toHaveAttribute('aria-describedby');
           |                              ^
      1134 |
      1135 |       act(() => userEvent.click(options[0], {pointerType: 'touch'}));
      1136 |       expect(options[0]).not.toHaveAttribute('aria-selected', 'true');

      at Object.<anonymous> (packages/@react-aria/dnd/test/useDraggableCollection.test.js:1133:30)
          at runMicrotasks (<anonymous>)
      at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/@jest/core/build/runJest.js:404:19)
      at _run10000 (node_modules/@jest/core/build/cli/index.js:320:7)
      at runCLI (node_modules/@jest/core/build/cli/index.js:173:3)

[yarastqt]

Hello! Is there any status on this issue

[snowystinger]

No update, feel free to have a look at the PR and failing test. You're welcome to open a PR addressing the issue that would supersede mine.