Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sliding animation on table elements in Safari #5922

Closed

Conversation

chrisirhc
Copy link

@chrisirhc chrisirhc commented Jan 24, 2021

This is a proof-of-concept PR for comments. It is not yet cleaned up. See below for what's incomplete.
This PR fixes #4712 and fixes #4948 .

The approach is as follows:

  • Add the ability to add non-animatable CSS (called staticCss in this PR) that is present throughout animation
  • Before animation starts, the staticCss is applied through a className on the element
  • After the animation is complete, the animation is cleaned up.

For #4712 : Since the overflow property's Animation type is discrete, the behavior is actually defined as changing overflow at the 50% mark if it wasn't overflow: hidden to begin with. However, the sliding animations require that overflow: hidden throughout the animation. This PR moves the overflow: hidden into a staticCss which is applied throughout the animation.

For #4948 : The display property is not animatable, so it can't be applied through animation keyframes. This PR adds a display: block if it detects that the element's display property contains `table. It could instead use a whitelist but I wanted to keep this simple and address #4948 before attempting to address all scenarios that the display property.

I didn't think this was a large enough change to warrant an rfc. Please let me know if it is.

PR is now ready for review.

I plan to add tests since this behavior adds an additional step to animations and needs to be tested.

  • Get a rough OK to see if this approach even makes sense before moving ahead with clean up.
  • Include a test that fails without this PR but passes with it.
  • Fix behavior for out-transitions (this PR does not address staticCss API for that).
  • Refactor/clean up style_manager to be able to handle "static" rules cleanly. Currently, this is a hack with some copy-pasta.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@flymedllva
Copy link

When will the release be released?

@chrisirhc chrisirhc force-pushed the fix-slide-animation-static-css branch from 7e2b03a to 153a116 Compare March 29, 2021 02:23
@chrisirhc chrisirhc changed the title WIP: Fix sliding animation on table elements and in Safari Fix sliding animation on table elements and in Safari Mar 29, 2021
@chrisirhc chrisirhc marked this pull request as ready for review March 29, 2021 03:22
@chrisirhc
Copy link
Author

Added a test and cleaned up the PR.
Wanted to use window.getComputedStyle to get the computed styles for component, but the tests would fail only in the hydration flow. Not sure if that's because the stylesheet doesn't exist through hydration flow.

@chrisirhc chrisirhc changed the title Fix sliding animation on table elements and in Safari Fix sliding animation on table elements in Safari Apr 3, 2021
@pngwn pngwn added bug compiler Changes relating to the compiler and removed feature: transitions labels Jun 27, 2021
@vercel
Copy link

vercel bot commented Apr 19, 2023

@benmccann is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm dummdidumm added this to the one day milestone May 4, 2023
@rkofman
Copy link

rkofman commented Nov 12, 2024

Just ran into #4948 and found this old open PR. Would it be useful for somebody to dust it off and get it ready to merge? If not, it would be lovely if it was closed w/ a rejection reason.

@benmccann
Copy link
Member

Yeah, as far as I'm aware, the main reasons it hasn't been reviewed is because there are merge conflicts and it's buried down the queue. Sending a cleaned up version would address both of those issues

@Rich-Harris
Copy link
Member

Thank you for the PR — closing in favour of #14930 and #14936

@Rich-Harris Rich-Harris closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler Changes relating to the compiler transition/animation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transition:slide looks different (buggy) on Safari Transitions don't work on table row addition
8 participants