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

[Motion] Can't use both Container transition and Shared element #1224

Closed
ColtonIdle opened this issue Apr 20, 2020 · 26 comments
Closed

[Motion] Can't use both Container transition and Shared element #1224

ColtonIdle opened this issue Apr 20, 2020 · 26 comments

Comments

@ColtonIdle
Copy link

Description: I'm trying to implement transition #1, which is from the new material motion system.

If I use sharedElementEnterTransition = MaterialContainerTransform() then I get a really nice looking container transform, BUT my issue is that my container has an image and I'd like to share that image. There doesn't seem to be clear guidance on how to do this, so I tried my best with just getting the container (outer viewgroup) and the contained imageView, and it doesn't seem to work. I can verify that my shared element is working, because if I remove my container transition and just put the transition on the image view via TransitionInflater.from(context).inflateTransition(android.R.transition.move) then I get a nice animated shared element. My issue is that I want both.

Looking at the gif from the material team below, it does seem like they include a package deal of a container transition + a shared element imageview.

https://miro.medium.com/max/1400/1*0vL7z6wwb1X9tvR6_rdQLw.gif
image

Expected behavior: As far as I can understand, MaterialContainerTransform() should allow you to chain another shared element.

@dsn5ft
Copy link
Contributor

dsn5ft commented Apr 20, 2020

Hi @ColtonIdle, thanks for the feedback! In the GIF you linked, that's actually just a pure container transform. It looks like the image is a shared element because it's in the same relative position in both the start and end containers.

We believe that the container transform pattern is simpler and more generally applicable than the traditional shared element pattern, while providing an effect that works well and emphasizes the navigation hierarchy. With that said, supporting the container transform with a traditional shared element seems like a reasonable request, so I'll leave this open.

One potentially tricky thing about combining the two patterns is that the image shared element would have to be excluded from the container transform animation, which currently animates the container and all of its children. Do you have a mock of what you'd like your final container transform + image shared element to look like?

@ColtonIdle
Copy link
Author

Nice to hear that it's just a container transform! I guess my current container transform just looked really bad when I had a similar setup (card expanding into a fragment) and it seemed to look bad to me because the image and elements didn't seem all that natural. I agree that in the gif I linked, the 1st animation looks pretty darn perfect.

But yes, I would love to say (in example 1) that "Hey this is a container transform, but bundled inside of it I want the imageView and textView to also land here" and so it would make sure to smooth the animation of those two items.

I'll try to come up with a sample app animation that shows the container transform looking "bad". I was working on hitting a deadline and so I didn't have much time to refine what I was working on. The shared element docs aren't that great in my point of view and so maybe I was doing something wrong? I'll try to check in back here later tonight with an example.

@ColtonIdle
Copy link
Author

@dsn5ft maybe another good question to ask (because this is essentially what I'm working with) is how would you try to build this animation... today?

Would this be something that container transform would be better suited for or shared element?

material

@ColtonIdle
Copy link
Author

ColtonIdle commented Apr 21, 2020

In addition to the question above, here's an example container transform that looks pretty bad in my opinion.

one_1

Fragment A

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        val hold = Hold()
        hold.duration = 3000
        exitTransition = hold

    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)

        view.findViewById<View>(R.id.cardView).setOnClickListener { card ->
            val extras = FragmentNavigatorExtras(card to "shared_element_container")
            findNavController().navigate(R.id.action_FirstFragment_to_SecondFragment, null, null, extras)
        }
    }

Fragment B

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        val transform = MaterialContainerTransform()
        transform.duration = 3000
        sharedElementEnterTransition = transform
    }

Am I misusing the MotionContainerTransform?

Here is how it looks with a shared element transition instead:

            val extras = FragmentNavigatorExtras(
                view.findViewById<ImageView>(R.id.imageView) to "sharedImage",
                view.findViewById<TextView>(R.id.textView1) to "sharedText1",
                view.findViewById<TextView>(R.id.textView2) to "sharedText2")
                findNavController().navigate(R.id.action_FirstFragment_to_SecondFragment, null, null, extras)

two_2

This looks pretty good, but unfortunately my designer doesn't like how the material card just "goes away". This is something that the motion container transform is good at.

So then my designer says, cant a shared element just be the card? And so I try that

            val extras = FragmentNavigatorExtras(
                view.findViewById<ImageView>(R.id.imageView) to "sharedImage",
                view.findViewById<TextView>(R.id.textView1) to "sharedText1",
                view.findViewById<TextView>(R.id.cardView) to "shared_element_container",
                view.findViewById<TextView>(R.id.textView2) to "sharedText2")
                findNavController().navigate(R.id.action_FirstFragment_to_SecondFragment, null, null, extras)

three_3

but that looks really bad again.

This is what led me to the conclusion that it would be great to have a Container Transform with optional shared elements. =)

Maybe I'm just misusing container transform (or theres some fine tuning in there I could do to make it look better).

All of the code can be found here: https://github.com/ColtonIdle/ContainerTransformOrSharedElement

@dsn5ft
Copy link
Contributor

dsn5ft commented Apr 22, 2020

Thanks for all of the info!

I would definitely recommend a container transform (without traditional shared element) for that Kodaline transition. It wouldn't result in the exact same staggered effect as in the GIF, but when set up correctly (with the correct container colors in place), I think it would look really good. And you could probably still have some of that staggered polish effect by having the fab be GONE initially and then fading/scaling it in, potentially even while the container transform animation is happening.

Re: the "bad" container transform, I think that's a relatively common issue we're seeing when the incoming screen/view doesn't have a background, but it should be easily fixable. I'd suggest either:

  • Setting a background (something like android:background="?attr/colorSurface") on your root view in the incoming screen, that way you don't get any overlap due to transparency.

OR

  • Setting a container color on the transform itself and changing its fade mode to be a "fade through", that way a persistent container background will be drawn as part of the transition animation, and the outgoing and incoming content will be faded out and in sequentially. Something like:
    transform.setContainerColor(...<read in R.attr.colorSurface>...);
    transform.setFadeMode(MaterialContainerTransform.FADE_MODE_THROUGH);

(I actually did the latter in a recent update to our Catalog app - d5faac5)

@ColtonIdle
Copy link
Author

I would definitely recommend a container transform (without traditional shared element) for that Kodaline transition. It wouldn't result in the exact same staggered effect as in the GIF, but when set up correctly (with the correct container colors in place), I think it would look really good. And you could probably still have some of that staggered polish effect by having the fab be GONE initially and then fading/scaling it in, potentially even while the container transform animation is happening.

👍

Tried option 1 and it Looks better
seven

Option 2 looks kinda bad because of the fade through
eight

Looks like I'll probably go with option 1? Unless you have any other suggestions. I will say that I'm a little disappointed by the output. it doesn't look like #1 from the original sample gif I posted at all.

Also, whats the difference between settings a background color on rootview vs transform.setContainerColor(...<read in R.attr.colorSurface>...);?

@jonasnaimark
Copy link

Hey @ColtonIdle
Option 1 is your best bet. Regarding not looking like #1 from the sample, there's a few things to consider:

  • The recordings are in super slow motion, so frames that are meant to be so fast they're not really noticeable end up being highlighted in an unrealistic way. With default durations the transition will look much better and closer to the sample. Slow mo is great for debugging, but to get a sense of the design I'd suggest testing on a device using normal durations.

  • The incoming view has a purple app bar which is a bit distracting. The sample details page uses a transparent app bar which creates a more seamless fade between views.

  • The card shadow disappears the instant it starts expanding which creates a small visual glitch. This is a bug we're working on.

It's also worth checking out the catalog app which has an implementation of #1 from the sample. Scroll to the bottom and tap "Transitions" then "Demo".

@dsn5ft
Copy link
Contributor

dsn5ft commented Apr 24, 2020

Thanks @jonasnaimark! Re: that last question:

Also, whats the difference between settings a background color on rootview vs transform.setContainerColor(...<read in R.attr.colorSurface>...);?

Calling setContainerColor will result in that color being drawn behind both the start and end container views for the duration of the transition, while a background on the root view in the incoming destination would only apply to that view, which in your case should be drawn on top of the start view.

@ColtonIdle
Copy link
Author

@jonasnaimark and @dsn5ft Wow. Thank you for the help. This has all been really great and eye-opening.

@jonasnaimark

  1. Good point. I'm sure at a real speed the animation will look better. I just posted them in slow mo in order to have it be easier to show. 😄
  2. Any tips for removing the bar? My apps architecture is a single activity and multiple fragments. Each fragment is in control of its toolbar. I wonder if I can just nest the transition one viewgroup down so that the bar stays at the top of the screen? I'll give it a whirl.
  3. Thanks for the heads up on the bug. I can definitely see this being even smoother when that's worked out.

@dsn5ft thanks. That makes sense too!

@dsn5ft
Copy link
Contributor

dsn5ft commented Apr 24, 2020

@ColtonIdle np! You might be able to remove the bar from the animation by adding an id to your LinearLayout and using transform.addTarget(<id>).

@ColtonIdle
Copy link
Author

Now I'm basically working on removing the toolbar as per @jonasnaimark suggestion.

Here's the anim with a background set on my second fragment, and android:transitionName="shared_element_container" being set to my root view in my second fragment. This is the same as I showed earlier.

toolbar1

Here's the anim but I moved android:transitionName="shared_element_container" down to the linear layout as I basically am hoping to skip the toolbar from also being animated. This looks bad for two reasons. 1. Surprisingly the text looks worse? (didn't expect that) 2. The toolbar "pops" in at the end.

toolbar2

Here's me removing the bar as per @dsn5ft suggestion. I understood the suggestion as android:transitionName="shared_element_container" should still be on my root view, but if I add an id to my LL (android:id="@+id/myActualLayout") and then did transform.addTarget(R.id.myActualLayout) but I got a really weird result. It's as if my duration doesn't do anything anymore? Not sure if this is a bug.

toolbar3

@daniel-stoneuk
Copy link

Re: the "bad" container transform, I think that's a relatively common issue we're seeing when the incoming screen/view doesn't have a background, but it should be easily fixable.

Have spent ages trying to figure out why my container transform wasn't scaling and this was the issue! Thanks for this.

@dsn5ft
Copy link
Contributor

dsn5ft commented May 7, 2020

Hi @ColtonIdle, sorry for the delay. I took a quick look at the toolbar issue but wasn't able to figure it out with your setup. Just curious if it's still an issue or if you have any other questions.

@ColtonIdle
Copy link
Author

@dsn5ft I'm unfortunately still having the issue I posted a few days ago.

I hope that I can still get some help with removing the toolbar from the animation though, as @jonasnaimark has said I think this is the trick to really making the animation look perfect and it's currently what's stopping me from shipping this to production.

I don't know if this is an issue with my code, or with the material motion library. I suppose that I can start a new bug report or create a new project that just shows that toolbar issue. Whatever is easier for you. I do feel like I'm 98% of the way there, and just need a way to omit this toolbar. My team has considered rearchitecting our app to only have a toolbar on the activity, but having a toolbar in our fragment just makes so much more sense, so we did not go with this approach.

@dsn5ft
Copy link
Contributor

dsn5ft commented May 7, 2020

Ah ok, so to fix the text looking worse issue, you have to make sure to move your android:background="?attr/colorSurface" down to the LinearLayout, in addition to the android:transitionName="shared_element_container". This is because the root view is no longer being included in the transform animation, so you need the background to be on the view that's being animated.

To fix the Toolbar popping issue, you could use a regular enterTransition to animate the Toolbar however you like, e.g., with a Fade. Also, it looks better if you set the drawingViewId on the transform sharedElementEnterTransition to the LinearLayout as well, because then the dimming effect won't be on top of the Toolbar. Putting that all together we get:

        val enterTransform = MaterialContainerTransform()
        enterTransform.duration = 3000
        enterTransform.drawingViewId = R.id.linearLayout
        sharedElementEnterTransition = enterTransform

        val returnTransform = MaterialContainerTransform()
        returnTransform.duration = 3000
        sharedElementReturnTransition = returnTransform

        val fade = Fade()
        fade.duration = 3000
        fade.addTarget(R.id.materialToolbar)
        enterTransition = fade

@ColtonIdle
Copy link
Author

Thanks @dsn5ft !

I would have never gotten the trick with enterTransform.drawingViewId = R.id.linearLayout

Don't want to be that person, but it looks like there's one last thing 😄

The returnTransform seems to have an overlay over the toolbar. Any hints on getting this looking right in the return transition?

For anyone following along, I made the background and transitionName change as per the above comment, and then I added:

        val enterTransform = MaterialContainerTransform()
        enterTransform.duration = 3000
        enterTransform.drawingViewId = R.id.linearLayout
        sharedElementEnterTransition = enterTransform

        val returnTransform = MaterialContainerTransform()
        returnTransform.duration = 3000
        sharedElementReturnTransition = returnTransform

The Fade transition on the toolbar didn't really seem to have an effect + the effect I'm really going for is to make it seem like the toolbar is part of the activity and not the fragment. And so I just want it to stay up there without any overlays or scrims or anything like that.

Here are the gifs of what I have now with speed set to 3000 and the other set to 300. You can see that the enter transition is pretty much 100% perfect in my opinion, but the return animation is "jarring" because of the instant scrim on the toolbar. I don't know if the issue is in the firstFragment or secondFragment.

3000
3000

300
300

In the 300 image you can see how the the bar is a bit more jarring.

@dsn5ft
Copy link
Contributor

dsn5ft commented May 18, 2020

I think drawingViewId should be able to help with that problem on the return transform as well. It's probably a little trickier, because you'd need a FrameLayout wrapper in your first fragment for everything below the Toolbar.

@ColtonIdle
Copy link
Author

@dsn5ft awesome. Thanks for the tip. I was able to just set an id on my first fragment on the material card, and just used that for the return drawingViewId and I think I basically got the end result I'm looking for!

Here's the final product (thanks to the help of @dsn5ft and @jonasnaimark)
af4

I think it looks pretty great. If I had to nitpick I still think the bug that @jonasnaimark mentioned:

The card shadow disappears the instant it starts expanding which creates a small visual glitch. This is a bug we're working on.

is still maybe one issue and I can't wait to see how this would look like without that bug.

And maybe another small issue is the mis-color of the card on the return transition as you can see here. The card looks like the ripple color on the bottom. I wonder if there's anything I can do about that, but maybe on the other hand it's just a background color thats missing? Not sure yet... but again. These two things are so minor, someone would really need a magnifying glass to notice it.

Screen Shot 2020-05-18 at 3 34 58 PM

I can say that this looks 100x better than what I started out with. I really can't say thanks enough for the help. I was able to get the exact effect I was looking for and I can't wait to integrate this into into a recyclerview of mine.

@jonasnaimark
Copy link

That looks great, glad it worked out!

The second issue is interesting. I notice the scrim doesn't appear on your return transition, I wonder if that's related to this bug? The enter transition does show the scrim (the black overlay that fades in over the background content).

@dsn5ft
Copy link
Contributor

dsn5ft commented May 18, 2020

Also glad it worked out! 😊

I think that mis-color of the card is actually the scrim being drawn only in the bounds of the card. That's what would happen when setting the drawingViewId to just the card, since that tells the transition only to draw in that space.

Regarding the shadows, that's been fixed recently! (1094b48) It will be included in the next release coming soon, or if you want to try out our nightly builds it should be available there.

@ColtonIdle
Copy link
Author

@dsn5ft Oh! So that's why I should still set a frameLayout for everything else besides the toolbar?

Maybe I'll try that once the next release of the library is out so I can try out the shadow fix as well!

@dsn5ft
Copy link
Contributor

dsn5ft commented May 18, 2020

Yep exactly, that's what I was thinking with the FrameLayout.

@dsn5ft dsn5ft closed this as completed Jun 16, 2020
@dsn5ft
Copy link
Contributor

dsn5ft commented Jun 16, 2020

Closing for now since a lot of the original questions have been answered.

@ColtonIdle
Copy link
Author

@dsn5ft sounds good. I will still give this a whirl when I get a chance to try the newest updates. Thanks again!

@ColtonIdle
Copy link
Author

@dsn5ft finally got around to retesting with latest alpha. It looks a lot better! There is still a slight jump in the UI with the card bounds that's apparent even with a 300ms delay. Worth filing a new bug or would it be good to keep this thread going?

@dsn5ft
Copy link
Contributor

dsn5ft commented Aug 3, 2020

Thanks @ColtonIdle! Let's file separate issues for anything specific that is happening.

grahammendick added a commit to grahammendick/navigation that referenced this issue Sep 5, 2021
The MaterialContainerTransform expects the id to be a resource?! The React Native ids aren't resources so android throws error when it can't find it. Could get round it by overriding getResources of SharedElementView, providing custom Resources and catching error from getResourceName. Not sure of consequences.
Also, it didn't work that well - the scrim was only applied over the drawing view but the from shared element disappeared completely at start of animation and then came back (not sure why - set duration to 5000 so could see it).
Also not sure of the timings. Maybe setting the drawingViewId from findNodeHandle will happen too late? Can't make it part of the postpone enter transition.
All in all, simplest to remove for now.
See material-components/material-components-android#1224 for why drawing view id is helpful - this example prevents the scrim appearing over the top of the toolbar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants