adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.53k stars 1.08k forks source link

Video Controls are not respected when using FocusScope #6729

Open kirstenxrauffer opened 1 month ago

kirstenxrauffer commented 1 month ago

Provide a general summary of the issue here

When using a native HTML video element inside of FocusScope with contains={true}, we are unable to tab into the individual native video controls like fullscreen, volume, etc.

πŸ€” Expected Behavior?

Focus trapping an element should still allow for the native video controls to be accessible via keyboard using the tab key.

😯 Current Behavior

If you place a native HTML video element inside of FocusScope like this pseudo-code:

<FocusScope contain={ true }>
  <video controls> ... </video>
<FocusScope/>

Then you are unable to tab into the individual video controls using a keyboard. The video element is in focus, but the focus never goes deeper.

πŸ’ Possible Solution

No response

πŸ”¦ Context

This is a breaking scenario for keyboard-only users for native video elements inside of FocusScope.

πŸ–₯️ Steps to Reproduce

  1. Place a native HTML video element inside of FocusScope like this pseudo-code:
    <FocusScope contain={ true }>
    <video controls> ... </video>
    <FocusScope/>
  2. Try to navigate to the fullscreen or volume controls. by pressing the "tab" key. Expected: You are able to tab into the controls Actual: Focus stays locked on the overall video player

If I comment out contain={ true } for FocusScope, I am able to access the video controls as expected, but of course the focus trapping does not work at that point.

Version

3.17.1

What browsers are you seeing the problem on?

Firefox, Chrome, Safari

If other, please specify.

(I have not been able to test on Edge)

What operating system are you using?

Mac Sonoma 14.3.1

🧒 Your Company/Team

No response

πŸ•· Tracking Issue

No response

snowystinger commented 1 month ago

Thanks for the issue. I was hoping that our work on ShadowDOM support would help this, unfortunately it appears not.

Instead, we might be able to use something like

 || e.target.tagName === 'VIDEO'

in this check https://github.com/adobe/react-spectrum/blob/92775875627ef8a038a4708b1a7addf11809bf35/packages/%40react-aria/focus/src/FocusScope.tsx#L331

Though I'm worried that the focus will get stuck in a loop on the video element if it's the last element in the containing focus scope.

kirstenxrauffer commented 1 month ago

@snowystinger Your solution worked for me in a local sandbox of the code! Do you think it'd be okay for me to write a test for this and put a PR up for it? Is this something that would be okay to have as a long term fix?

Edit to add: I realized that to properly test this I'll have to add a dummy video to the test data. Would this be okay? Any recommendations such as file size limitations, where the dummy asset should be placed, etc?

Edit to add: Through more testing, this fix does have the side effect of causing an infinite focus loop on a video element so it is not a viable fix

snowystinger commented 1 month ago

We can probably just reference someone elses video https://www.w3schools.com/html/html5_video.asp for example, you can just use the URL of that sample video

LFDanLu commented 1 month ago

It would be good to figure out why exactly it is still looping back to the video with the changes made in the PR. One possible idea that came up would be to specifically detect via a focus/blur listener when focus is entering/leaving the video element and control the focus marshalling in FocusScope around that specifically.

kirstenxrauffer commented 1 month ago

@LFDanLu @snowystinger I did try to add a focus listener to the video element to see what's happening with the looping bug, but interestingly, the controls other than the very first control do not trigger the focus change event listener. I don't know if I'll have time to do the blur listener but I do like that approach, that's essentially how I have seen other libraries like react-focus-lock handling it. Just observing the focus and only refocusing on the next appropriate element when needed, like when the focus is about to leave the focus locking scope.