[fixed] Ensure after-open css transitions work in Safari 14 & Mobile Safari

[benjaminpadula]

Fixes #846.

Changes proposed:

• Use requestAnimationFrame to delay the after-open class from being added until it mounts with inital CSS

Upgrade Path (for changed or removed APIs): n/a

Acceptance Checklist:

• The commit message follows the guidelines in CONTRIBUTING.md.
• Documentation (README.md) and examples have been updated as needed.
• If this is a code change, a spec testing the functionality has been added.
• If the commit message has [changed] or [removed], there is an upgrade path above.

Additional notes:

It struck me that perhaps the issue was that when the modal is mounted, it is immediately mounted with the open state and initially renders with the after-open class. It's actually kind of surprising, then that Chrome shows a transition, when technically nothing is really changing. Perhaps the old browser interpreter is inferring developer intent or doing something wrong. ¯\_(ツ)_/¯

So I thought it was worth a try to ensure that the modal mounts in the closed state first, and then apply the open class. (See modified sandbox) And that seems to do the trick! But since it's likely a problem affecting everyone, I decided to try fixing it directly in react-modal so everyone could benefit from working transitions in Safari 14 & mobile Safari.

Moral: When using CSS transitions, ensure that the component is rendered first before applying the class that triggers the transition. Test in Safari 14 to ensure you got it right, as other/older browsers seem to be more lenient.

[coveralls]

Coverage decreased (-0.5%) to 82.152% when pulling 848513e on benjaminpadula:fix/css-after-open-transition-in-safari-14 into c9d8e2d on reactjs:master.

[benjaminpadula]

Hey @diasbruno, it's been a while since I created this PR. Yes, using requestAnimationFrame() improves the behavior here, but I have still found that it appears to skip the transition maybe 10% of the time? I have had more success locally by using setTimeout() with a 10-15ms delay.

What are your thoughts on this? Would you like me to:
A) Open a new PR using the setTimeout approach?
B) Merge this PR as-is?
C) Solve the problem in some other way?

[diasbruno]

@benjaminpadula Can you try adding a class (with default properties like left: 0) to be animated?
Maybe, with the default properties to be animated, it can transitions correctly. (?)

[pwfisher]

Any updates on this issue? Is there another PR to watch for a different approach to fix this? Is there any workaround in the meantime?

[diasbruno]

@pwfisher You can try @benjaminpadula's branch...It would help validate the fix.

[pwfisher]

Seems I can't, and I think it's due to an error in react-modal's package.json: its "main" entry points to a nonexistent file.

"react-modal@github:benjaminpadula/react-modal#fix/css-after-open-transition-in-safari-14":
  version "3.12.1"
  resolved "https://codeload.github.com/benjaminpadula/react-modal/tar.gz/848513e3057711ba53be5e60c555965426beba75"

Error: Cannot find module '/Users/patrickfisher/Code/figs/syconium/node_modules/react-modal/lib/index.js'. Please verify that the package.json has a valid "main" entry

[diasbruno]

Use the https version:

On your package.json:

  react-modal: "https://github.com/benjaminpadula/react-modal#fix/css-after-open-transition-in-safari-14"

[atomanyih]

@pwfisher @diasbruno ran into a similar problem. lib is in the .gitignore, so the forked version is missing that and can't be consumed as a package. I made my own fork and committed with lib included: https://github.com/atomanyih/react-modal/tree/fix/css-after-open-transition-in-safari-14

There are no other changes from @benjaminpadula's branch

[atomanyih]

@diasbruno Sure, do you want me to make a PR against the master branch of react-modal?

[diasbruno]

@atomanyih Yes, that would be great.

[justinledelson]

@diasbruno Sure, do you want me to make a PR against the master branch of react-modal?

Any updates on the status of your branch? Will it be merged soon? Thnx

[diasbruno]

Sorry for the delay on this...I'm going to push this PR forward as it doesn't seem to cause problems.

[diasbruno]

Merged manually. See 827796d.

Thanks, @benjaminpadula.

[justinledelson]

@diasbruno are you planning to release a new version to NPM?