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.95k stars 1.12k forks source link

Users unable to select text on the page due to transitioning elements being removed from the DOM before transitionend event is handled which blocks restoreTextSelection from running #1848

Open briluu opened 3 years ago

briluu commented 3 years ago

๐Ÿ› Bug Report

Our app is preventing users from selecting text on the page ever since we introduced some new React Spectrum code. We're going through an AngularJS to React migration so things are in an imperfect state in regards to the code environment. I tried to make a sandbox to repro the issue but wasn't able to do it properly. It is linked below.

๐Ÿค” Expected Behavior

We expect our users to be able to select text on the page for their selecting and copy and pasting needs.

๐Ÿ˜ฏ Current Behavior

Users are not able to select text on our page.

๐Ÿ’ Possible Solution

I had a discussion with @LFDanLu and we believe to have narrowed down the issue. We've determined that it seems like our app uses AngularJS's built-in CSS transition classes - e.g. components have transition styling applied (Angular docs to get an idea of the css styling we have, e.g. ng-leave and ng-enter), that isn't being correctly removed from the list of transitioning elements tracked here. According to Mozilla web docs, it says the transitionend event will not fire if the transition has not completed, leading to the conclusion that our css transition styling handled by AngularJS is not allowing for the transitionend event to be fired properly.

This results in a non empty callback queue where the textSelection's restoreTextSelection() callback function isn't ever called, thus preventing users from selecting text on the page.

๐Ÿ”ฆ Context

Inability to copy/paste text on our app has reported in customer complaints so the goal is to hopefully restore that functionality ASAP.

๐Ÿ’ป Code Sample

I wasn't able to get a reproducing code sandbox. It's a work in-progress - https://codesandbox.io/s/zealous-haibt-zwk0t?file=/src/App.tsx. I can see the transitionend event not being fired when the element is removed though, however I'm unable to reproduce the inability to select text. Feel free to make changes to it.

The steps to reproduce (in theory): 1) On startup, React Spectrum will track all transition events. 2) At some point, a website will have elements with transitions and fire transitionrun events properly and be tracked by React Spectrum 3) The same component will be removed from the DOM and not fire the transitionend event. 4) Click a React Spectrum button, which will disable and text selection on the html element. On button release, React Spectrum will attempt to restore text selection but fail to do so because it thinks some elements are still transitioning.

๐ŸŒ Your Environment

Software Version(s)
react-spectrum 3.8.0
Browser Chrome
Operating System MacOS

๐Ÿงข Your Company/Team

Adobe Admin Console

briluu commented 3 years ago

I found the AngularJS line of code that is responsible for this weird behavior for our app.

AngularJS is actually preventing the transitionend event from being propagated to the React Spectrum handler.

See AngularJS code here.

I put a breakpoint and was able to confirm that the transitionend event was being stopped here.

So core issue looks like it is the incompatibility of AngularJS and React Spectrum. But it does seem like React Spectrum makes an incorrect assumption that the transitionend will always be fired properly, even though there are non-AngularJS cases when that won't always happen.

Is it possible React Spectrum can address this edge case?

LFDanLu commented 3 years ago

@briluu I made a slight modification to your sandbox (https://codesandbox.io/s/beautiful-sun-4owvj?file=/src/App.tsx) to demonstrate that transitioncancel fires when the box is removed from the DOM. Our runAfterTransition hook uses this to handle the "node removal in mid transition" case: https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/utils/src/runAfterTransition.ts#L36-L39. Perhaps we could add something that periodically checks that elements within the transitionsByElement map still exist in the DOM and remove them if they don't but I'll need to check what cases exist where transitionend doesn't fire.

briluu commented 3 years ago

Ah I overlooked the transitioncancel event. Thanks for pointing that out.

It's starting to sound like this might be a very specific bug given our AngularJS / React environment since the AngularJS source code is actually stopping the transitionend from propagating when it typically should โ˜น๏ธ

LFDanLu commented 3 years ago

@briluu Remind me, do you have a workaround/would you be able to replace that problematic spinner? Trying to set priority for this or close it out depending on severity.

briluu commented 3 years ago

As an FYI, we learned that the core issue actually spans across multiple components in our codebase that uses AngularJS's built-in CSS animation library, so not just that one spinner :/

We were waiting to see if React Spectrum would handle this issue before we attempt to write our permanent long term fix.

We've written a temporary bug fix to set the user-select styling back to '' on every pointerup event to emulate what React Spectrum would do to restore text selection. This helps us unblock our users, but the intent is to solve the root issue (which is our transitioning components blocking up the callback queue) either via code in our codebase or React Spectrum cleaning it up (if you believe that RS is responsible for this behavior).

We were considering two options for our long term fix: 1) Prevent transitionstart events from propagating to React Spectrum so that we don't block the queue. An issue with this approach is that React Spectrum won't properly know when our components finish transitioning, which could lead to some buggy behavior. Based on a quick search through the RS repo, it looks like we could potentially have improper focus behavior. 2) We send out a copy of the transitionend event that is stopped by AngularJS, so that React Spectrum is able to properly keep track of transitions and execute their code properly. We would delay the event propagation with setTimeout() and hope that AngularJS tears down their event listeners by then so that it doesn't get stopped.

Feel free to suggest any alternative solutions.

Do you feel like it is React Spectrum's duty to clean up this behavior? If so, having your fix would be great. If not, we can close this issue out and it'll be our responsibility to address this in our codebase.

devongovett commented 3 years ago

Maybe we should try using a capturing event listener instead of relying on bubbling? That should avoid issues where propagation is stopped by another script.