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

feat(runtime-dom): adjust transition class process #2597

Merged
merged 4 commits into from
Dec 2, 2020

Conversation

luwuer
Copy link
Contributor

@luwuer luwuer commented Nov 12, 2020

Close #2531, Close #2593

Make sure that *-enter/leave-from is the beginning of transition.

@luwuer luwuer changed the title feat(runtime-dom): transition component effect on visibility (#2531), transition component does not work with "*-leave-from" (#2593) feat(runtime-dom): adjust transition class process Nov 12, 2020
@luwuer
Copy link
Contributor Author

luwuer commented Nov 13, 2020

May i ask is there any information of reviewing time? @LinusBorg @posva

I need you help to verify if I've handled both issues correctly.

@@ -59,7 +59,6 @@ describe('e2e: Transition', () => {
// leave
expect(await classWhenTransitionStart()).toStrictEqual([
'test',
'v-leave-active',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These existing test cases should not change. That behavior is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed some of transform logic at Transition.ts , so that the test cases should follow the changes. I'm going to analyze it a little bit that why in this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that the fix should not require to change those tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll explain why I'm doing this and please wait a minute.

Copy link
Contributor Author

@luwuer luwuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before modification

image

The transition class test-enter-active and test-enter-from are added to el in onBeforeEnter, and then the class test-enter-from is removed in next tick(animation frame).

For example, Reproduction link of issue #2531.

  1. The el has a default style opacity: 1.0 mdn
  2. We set opacity: 0 and transition to the el when beforeEnter, so the el will start transition from opacity 1.0 to opacity 0.
  3. But we remove opacity: 0; from el by remove class .tooltip-enter-from in the next frame, so transition stop.

So that the main problem in these cases is that .tooltip-enter-from is not the initial style.

.tooltip-enter-from,
.tooltip-leave-to {
  transform: translateY(-30px) scale(0.96);
  opacity: 0;
}

after modification

image

The transition class test-enter-active is added to el in 'onEnter' which is triggered next tick.

Also Reproduction link of issue #2531.

  1. The el has a default style opacity: 1.0
  2. We set opacity: 0 to el when beforeEnter, but the el will not start transition because of no css transition property.
  3. Then we set transition and opacity: 0 to the el in the next frame, the el start transition from opacity 0 to opacity 1.0.

The same as leave.

summary

I changed tests because of i think the .tooltip-enter/leave-active should be set in the next frame.

@@ -59,7 +59,6 @@ describe('e2e: Transition', () => {
// leave
expect(await classWhenTransitionStart()).toStrictEqual([
'test',
'v-leave-active',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll explain why I'm doing this and please wait a minute.

@posva
Copy link
Member

posva commented Dec 1, 2020

What I meant is that -active should be set on the first frame, as it is right now. The change you want to do is a breaking change and won't happen

@luwuer
Copy link
Contributor Author

luwuer commented Dec 1, 2020

What I meant is that -active should be set on the first frame, as it is right now. The change you want to do is a breaking change and won't happen

Well, i am not understand why its breaking change, there has no change about external APIs.

I think about it another way just now:

  1. Add a workaround class at the same time when add -active on the first frame.
  2. Remove the workaround class on the beginning of the next frame.

Is that what we need ?

@posva
Copy link
Member

posva commented Dec 1, 2020

Maybe this is clearer: https://v3.vuejs.org/guide/transitions-enterleave.html#transition-classes
Changing how the classes are added is a breaking change

@luwuer
Copy link
Contributor Author

luwuer commented Dec 1, 2020

Thanks for your replying. According to the document, its really a breaking change.

I have aslo a question: If i use a workround class to fix it and its also have to change tests(because a new class has been added on the first frame), is that a breaking change ?

Just like below:

    onBeforeEnter(el) {
      onBeforeEnter && onBeforeEnter(el)
      addTransitionClass(el, enterActiveClass)
      addTransitionClass(el, enterFromClass)
      // add
      addTransitionClass(el, 'transition-workaround')
      nextFrame(() => {
        removeTransitionClass(el, 'transition-workaround')
      })
    },

Or change inline style.

@luwuer
Copy link
Contributor Author

luwuer commented Dec 2, 2020

How about this way @posva

Disable transition on the first frame and enable it on the next frame.

Copy link
Contributor Author

@luwuer luwuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason of why changed seemingly unrelated tests.

)
await transitionFinish()
await nextFrame()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add await nextFrame() ?

  const buffer = 5
  const transitionFinish = (time = duration) => timeout(time + buffer)

Because the 5 milliseconds can not ensure the next frame refresh.

Why there is no problem before ?
Because the leave transition will be broken when effect on Suspense component. Tell me if need a reproduction link.

@@ -123,6 +123,7 @@ export function resolveTransitionProps(
const resolve = () => finishEnter(el, isAppear, done)
hook && hook(el, resolve)
nextFrame(() => {
enableTransition(el)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and disableTransition calls in beforeEnter and beforeAppear) seems unnecessary. When elements get a class in a none-rendered state (display: none or off-DOM) they do not trigger transitions anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok I see you are doing this so it can support visibility toggles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this locally, I find that this actually alters the behavior of v-show transitions when you hit toggle really fast.

The current behavior smoothly transitions half-way into the opposite direction, but with this change it hard resets to the enter from state.

packages/runtime-dom/src/components/Transition.ts Outdated Show resolved Hide resolved
@yyx990803 yyx990803 merged commit e2618a6 into vuejs:master Dec 2, 2020
@yyx990803
Copy link
Member

Note: I removed the transition disabling for enter/appear transitions because it breaks v-show's current behavior. Whether we should or how to support visibility: hidden in transitions should be a separate discussion/PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants