Fix race condition in drawer animations

[mollykreis]

Pull Request

🤨 Rationale

Resolves #1937

👩‍💻 Implementation

Based on #1937, the issue is that the animationend handler isn't being called. That is true, but the root cause of the bug is that the close animation isn't starting (and therefore not ending). Specifically, the timing exposed in this test is:

• Open the drawer
• Wait for the drawer's open animation to complete
• Attempt to immediately start closing the drawer
  • The drawer doesn't actually close because the browser doesn't have time to fully process the animating class being removed from drawer in order to remove the animation in order for re-adding the animating class to cause the animation to start again.

I tried a few different things to try to resolve the issue:

• Update the test to have a delay between the drawer opening and closing
  • This fixed the test, but I decided that ultimately, the issue wasn't with the test but with the drawer element. There shouldn't be a time window (even if it is short) in which calling close doesn't work.
• Call await DOM.nextUpdate before starting the animation.
  • This worked as long as I called DOM.nextUpdate twice (not sure why it was needed twice). I decided against this because (1) the call was needed twice, which isn't ideal, (2) it forced previously synchronous code paths to be async, and (3) it would break a number of tests in nimble and likely even more client tests. While none of those are insurmountable issues, collectively, it was enough to make me question whether or not that was the best solution.
• Wrap the code that starts the animation in setTimeout with a timeout of 0.
  • Similar to the previous alternative, this would break a number of tests in nimble and likely even more client tests. This isn't insurmountable, but ideally we can find a solution that doesn't break client tests.
• Re-write the animation code
  • This isn't a feasible solution because Firefox doesn't support programmatic animations of the ::backdrop psuedo element.

Therefore, I landed on the solution to trigger a reflow by reading the offsetHeight of the dialog before adding the animating class to start the animation. One major downside of this approach is that the code itself doesn't make it clear why it is needed, but I think a comment explaining the line goes a long way in mitigating that concern. The big positive is that it doesn't have a large impact on the timing from a client or test perspective. Therefore, I expect significantly less (hopefully no) client repercussions associated with the change.

🧪 Testing

Ran all the drawer tests in chrome, FF, and webkit to make sure the change didn't introduce any more issues.

✅ Checklist

• I have updated the project documentation to reflect my changes or determined no changes are needed.

[mollykreis]

@msmithNI, will you buddy this PR for me? I'm interested if you have any other ideas for how to solve this problem that are better than what I've implemented.

[msmithNI]

@msmithNI, will you buddy this PR for me? I'm interested if you have any other ideas for how to solve this problem that are better than what I've implemented.

The only other thought I had was that we could explore using the (JavaScript) Web Animations API instead of the current CSS animation approach. But I took a look at the dialog styles and that's probably not feasible due to the animations on the backdrop (I'm not aware of a JS equivalent to the ::backdrop pseudo-selector).

[rajsite]

@msmithNI, will you buddy this PR for me? I'm interested if you have any other ideas for how to solve this problem that are better than what I've implemented.

The only other thought I had was that we could explore using the (JavaScript) Web Animations API instead of the current CSS animation approach. But I took a look at the dialog styles and that's probably not feasible due to the animations on the backdrop (I'm not aware of a JS equivalent to the ::backdrop pseudo-selector).

There is a pseudoElement option for element.animate: https://caniuse.com/mdn-api_element_animate_options_pseudoelement_parameter

Haven't tried it with ::backdrop specifically