1486365 - The City selection page slightly bounces after opening

Status: NEW

(Core :: CSS Transitions and Animations, defect, P3)

Description

[Karl Dubost💡 :karlcow]

1. Visit the site
2. Tap on either the Departure City or Destination list. In this case, the defaults are "Jakarta (JKTA)" and "Bali / Denpasar (DPS)" respectively
3. This opens a selection of cities. After the opening animation is finished, the page slightly moves down and returns to normal.



(This doesn't always reproduce but this is visible most of the time.)

Comment 1

[Brian Birtles (:birtles)]

Looks like something there is triggered additional transitions. i.e. there's an extra transition just before it settles transitioning from e.g. 'translateY(0.35%)' to 'none'.

Comment 2

[Brian Birtles (:birtles)]

s/is triggered/is triggering/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

This seems more like a glitch in how we're rendering the animation, and not a performance issue per se. qf-'ing for now.

Comment 4

[Brian Birtles (:birtles)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)
> This seems more like a glitch in how we're rendering the animation, and not
> a performance issue per se. qf-'ing for now.

I suspect it's not a glitch in our rendering but a bug in the content that happens to look fine in Chrome because Chrome is buggy.

Comment 5

[Brian Birtles (:birtles)]

I dug into this a bit further and it looks like Chrome also generates two transitions. This appears to be because the content uses CSSTransitionGroup (from an old version of react-transition-group) and specifies a transition timeout equal to the duration of the transition.

Now, the transition is always going to take a little longer than the specified duration since we'll need to layerize the content first (it's a 'transform' transition). So the timeout actions are going to run before the transition finishes. That's true of Chrome and Firefox. So, ultimately the content is buggy.

(It's further compounded by the fact that the classes it uses for the neutral state differ in their definitions: one has 'transform: translateY(0)' and one has no transform, i.e. 'transform: none' so you end up triggering transitions from 'translateY(0)' to 'none' even if the timeout is extended to be longer than the transition duration.)

What differs between Firefox and Chrome, however, is that Chrome does a better job of updating animations running on the compositor. For example, if you extend the transition duration in Chrome (look for the `._3d1ih` rule in 'Sources' -- in Firefox you'll want to turn off "Show original sources": that feature seems to make this impossible to find), e.g. make it .7s or .8s, then you'll see it clearly starts a new transition mid-way through and does it seamlessly.

Firefox also does a pretty good job (we added special code for this for Firefox OS) but appears to be not quite right when the transition finishes in the middle of updating. Specifically we should be hitting this code:

  https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/layout/style/nsTransitionManager.cpp#873-896

But either we're not, or it's not working as expected in this case. (I wonder if we're actually in a case where by the time the timeout runs, the animation has finished on the compositor but we haven't updated style on the main thread yet so we end up using the latest value from the main thread as the start value?)

Comment 6

[Brian Birtles (:birtles)]

Adding a bit of logging here it looks like our transition updating code is working as expected:

  Triggering transform transition
      oldPT 1
      oldPT current? 1
      oldPT running on compositor? 1
      oldPT has start time? 1
      oldPT has same timeline? 1
  ...
  Updating start value from replaced transition
  Updated computed progress is 0.878576
  Updated value position is 0.991499
  Successfully updated start value

So the problem seems to be simply that our approach is to do everything on the main thread.

Originally I believe we were going to do this on the compositor itself--i.e. get it to recognize updated transitions and update accordingly. See bug 1167519 comment 0.

But then I was worried about drift (bug 1167519 comment 9) and suggested we could do this on main thread (noting that this "wouldn't fix the problem if painting was expensive"). Presumably in this case either painting is expensive or, more likely, there is some non-negligible IPC overhead or overhead on the compositor.

So I think in order to fix this in Gecko we'll need some non-trivial work for compositor animations (basically equivalent to moving the current "replaced transitions" code there, I think). Without a project like Firefox OS depending on it though I'm not sure if we are likely to get to that in the near future, however.

On the content side, I'd advise the site to put their 'transition' rules inside their 'active' classes. (See the example: in https://reactcommunity.org/react-transition-group/transition-group). That way, even if the timeout runs before the transition finishes it will actually have the intended effect: canceling the transition as opposed to triggering a new transition. In addition, they should also extend the timeout values to be slightly longer than the transitions themselves or else the transition might be cut short.

Comment 7

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 8

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.