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

Add View Traits and transitions #426

Merged
merged 22 commits into from Jul 28, 2021
Merged

Add View Traits and transitions #426

merged 22 commits into from Jul 28, 2021

Conversation

carson-katri
Copy link
Member

@carson-katri carson-katri commented Jul 10, 2021

This adds _ViewTraitKey and related internal modifiers. Then transition builds on top of these traits.

Here's the transition demo:
https://user-images.githubusercontent.com/13581484/126044318-b2856cbb-56ed-4196-a0fd-6ea5c6c606c7.mov

It works by applying the transition's modifier to the a View on mount/update/unmount.

@carson-katri carson-katri added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 10, 2021
@carson-katri carson-katri added the DOM/HTML renderer Tokamak in the browser label Jul 17, 2021
@carson-katri carson-katri marked this pull request as ready for review July 17, 2021 16:56
@carson-katri carson-katri requested a review from a team July 17, 2021 17:06
dom: DOMNode,
additionalAttributes: [HTMLAttribute: String],
transaction: Transaction
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to split the body of this function into multiple functions or some helper types to resolve the linter warning?

@@ -98,24 +98,29 @@ public class MountedElement<R: Renderer> {
}
}

public internal(set) var viewTraits: _ViewTraitStore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these traits somehow different from SwiftUI accessibility traits? I can't find anything else about SwiftUI traits, just wondering what's used as a reference here.

Copy link
Member Author

Choose a reason for hiding this comment

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

SwiftUI has _ViewTraitKey internally, and uses them for transition, onDelete, onMove and some other modifiers (I think drag & drop?). From what I can tell, a trait is data attached to a single view, and in many cases seems to be a way to get the trait out of SwiftUI and into the host platform (for instance, for UITableView swipe actions or NSItemProvider-based drag & drop).

Although I'm really just speculating as to how SwiftUI uses them by looking at the swiftinterface. In Tokamak they are meant to be passed to the nearest host element so renderers can access them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Traits are also used for zIndex(_:) and layoutPriority(_:). I haven't look into whether Tokamak has implemented these modifiers. If they're not implemented, traits would be really helpful.

@ezraberch
Copy link
Member

  1. If you remove all but one of the items in the demo (leaving Text(".slide").transition(AnyTransition.slide), for example), the text will fade in and out instead of showing the proper transition.
  2. For the following, the SwiftUI behavior is the first line slides while the second and third lines fade. With your code, the second and third lines both slide and fade.
Text(".slide")
  .transition(AnyTransition.slide)
VStack {
  Text(".slide").transition(AnyTransition.slide)
  Text(".slide").transition(AnyTransition.slide)
}
  1. For the following, all 3 lines slide. However, unlike in SwiftUI, line 1 and lines 2-3 do not slide together when sliding out.
Text(".slide")
  .transition(AnyTransition.slide)
VStack {
  Text(".slide")
  Text(".slide")
}.transition(AnyTransition.slide)
  1. If you click fast enough, you can see the same element multiple times.

_ style: String,
on dom: DOMNode,
with animation: Animation
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize these linter warnings are a bit annoying, but is it still possible to get it a bit slimmer and under 50 lines? Maybe would work better from code readability perspective too.

@carson-katri
Copy link
Member Author

1-3 should be fixed.

@carson-katri
Copy link
Member Author

My idea to fix 4 atm is to keep a list of unmountingViews (Views that haven't called the callback after unmounting yet), and keep those available for reconciliation, then cancel their removal if they are required to mount again.

@ezraberch
Copy link
Member

1-3 should be fixed.

1 and 2 look good.

For 3, the slide-out looks good. However, for the slide-in, lines 2-3 just appear rather than sliding.

@ezraberch
Copy link
Member

onDisappear is triggering before the animation rather than after. With the following additions/changes to the Demo:

@State private var showText = "Show"
...
Button(isVisible ? "Hide" : showText) {
...
Text(".opacity")
  .transition(AnyTransition.opacity)
  .onDisappear(perform: {
    showText = "Show Again"
})

In SwiftUI, the button text changes to "Show" when you click "Hide", then changes to "Show Again" when the animation is complete. With your code, it changes from "Hide" to "Show Again" immediately upon clicking "Hide".

@carson-katri
Copy link
Member Author

For 3, the slide-out looks good. However, for the slide-in, lines 2-3 just appear rather than sliding.

Seems like it only affects .move (.slide)...

@carson-katri
Copy link
Member Author

4 should be fixed now too.

@carson-katri
Copy link
Member Author

carson-katri commented Jul 23, 2021

onDisappear is triggering before the animation rather than after.

There's already a FIXME for both onAppear and onDisappear to be renderer-specific, so maybe we could defer that to a separate PR (#175 using something like the Intersection Observer API to check if the element is visible in the viewport)?

Copy link
Member

@ezraberch ezraberch left a comment

Choose a reason for hiding this comment

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

For 3, the slide-in is still behaving incorrectly.

For 4, the fix is causing another problem. If you switch from Transition to a different section of the demo while a transition is happening, the animating HTML will appear in the new section for the remaining duration of the transition animation.

@ezraberch
Copy link
Member

Also, I'm now getting an error when rapidly pressing the inner button:

Fatal error: Attempted to read an unowned reference but object 0x733ee0 was already deallocated
Uncaught RuntimeError: unreachable

@carson-katri
Copy link
Member Author

The slide-in bug should be fixed. Seems like the getComputedStyle function returns the transformation matrix in pixel values instead of percentages, which are incorrect because the children haven't been mounted yet. That's probably why it worked for single elements but not VStack. I disabled computing the start keyframe when the element is mounting to work around this.

@carson-katri carson-katri requested a review from a team July 24, 2021 13:44
@carson-katri carson-katri requested a review from a team July 26, 2021 03:30
ezraberch
ezraberch previously approved these changes Jul 26, 2021
@ezraberch
Copy link
Member

Looks good!

You are getting a couple of linter warnings now, so you may want to refactor a bit before merge.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

👏

@carson-katri carson-katri merged commit 9a568ab into main Jul 28, 2021
@carson-katri carson-katri deleted the transition branch July 28, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DOM/HTML renderer Tokamak in the browser SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

None yet

4 participants