theKashey / react-focus-lock

It is a trap! A lock for a Focus. 🔓
MIT License
1.27k stars 67 forks source link

`crossFrame={false}` has cross-browser Safari issues #249

Closed cee-chen closed 1 year ago

cee-chen commented 1 year ago

Not sure if this is just me (on Safari v16.5.1), would love it if people could confirm if they can repro!

Reproducible CodeSandbox (using react-focus-on, but it should in theory be the same for react-focus-lock?):

theKashey commented 1 year ago

bug confirmed 😭

cee-chen commented 1 year ago

Thanks for the quick repro Anton!

Not sure if this is helpful or not, but @tkajtoch and I were testing crossFrame in react-focus-on locally in our codebase's Storybook, and were also getting that same behavior as well even in Chrome and FF and couldn't figure out why or what was happening (code was almost identical to the above CodeSandbox). We eventually figured out it was the setTimeout 0 in onWindowBlur race condition:

https://github.com/theKashey/react-focus-lock/blob/master/src/Trap.js#L185-L187

We changed the above setTimout duration from 0 to 1 and the crossFrame behavior started working for us locally. Maybe give it a shot on your end and see if it fixes Safari behavior as well? 🤞

theKashey commented 1 year ago

😲 onBlur is executed "a little later" using a deferAction helper of mine - https://github.com/theKashey/react-focus-lock/blob/master/src/util.js#L7 It works exactly via setTimeout(..,1) - you have found the root case. Well, it uses setTimeout only in the absence of setImmediate, which may or may not be polyfilled by a bundler.

cee-chen commented 1 year ago

Oooh! It all makes sense now 🤦 All the credit goes to @tkajtoch for figuring out that changing setTimeout fixed the behavior!

Would changing onWindowBlur to use deferAction be a clean solution, or would just changing the 0 to a 1 on its timeout suffice? 🤔

theKashey commented 1 year ago

deferAction is a way to go. Tested in Chrome and safari - works great.

theKashey commented 1 year ago

Released as v2.9.5.

Change is not yet propagated to https://github.com/theKashey/react-focus-on, please help me understand https://github.com/theKashey/react-focus-on/pull/74#issue-1790431467 first

cee-chen commented 1 year ago

I think @tkajtoch is working on that - will keep you posted! Thanks as always for being amazingly responsive and speedy Anton!