1408152 - Throbbers get out of sync when dragging tabs

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P5)

Description

[Adam Roach [:abr]]

If you drag a tab while it has a loading throbber in it, the throbber loses sync with other throbbers in the same window.

STR:

- Create a bookmark folder with a fairly large number of tabs (e.g. 20 or so)
- From that folder, perform an "Open All in Tabs" 
- While the pages are still loading, drag one tab to any location other than its starting one
- Observe that throbber on the dragged tab is no longer in sync with the other tabs' throbbers

(I discovered this in a less contrived but harder-to-reproduce situation involving only two tabs -- so it does arise even without a large number of tabs loading at the same time)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I looked in to this and simply adding a (ugly) call to `this.tabbrowser.mTabProgressListener()._syncThrobberAnimations();` at the end of onTransitionEnd within the "drop" handler[1] didn't fix this for me.

[1] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/browser/base/content/tabbrowser.xml#7589-7602

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1408152 - Sync the throbber animations again after the tab has finished moving.

This feels pretty ugly to dig in to tabbrowser's tabprogresslistener so we can call syncThrobberAnimations. Alternatively, syncThrobberAnimations could be pulled out of the tabprogresslistener now that it would be called from another site. I looked at the CSS changes that the tab moving does and nothing jumped out at me as to why the animation gets out of sync.

Review commit: https://reviewboard.mozilla.org/r/200364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200364/

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8929061 [details]
Bug 1408152 - Sync the throbber animations again after the tab has finished moving.

I'd be curious to know what re-starts the animation. Perhaps something clobbers the animation-name property? We could set a breakpoint in nsAnimationManager.cpp to find out or even just listen for 'animationcancel' events (assuming the animation is actually being re-created).

In any case, _syncThrobberAnimations should be safe to call repeatedly so this seems fine to me.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8929061 [details]
Bug 1408152 - Sync the throbber animations again after the tab has finished moving.

https://reviewboard.mozilla.org/r/200364/#review205672

Yeah, looks like this could be pulled out as a method that just takes a single tab as a param? That seems cleaner than this...

Otherwise this looks OK to me given bbirtles is happy? Up to you if you want to re-request review and/or do more digging.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I pulled out the function and put it on tabbrowser with a `tab` argument. I'll land this now since the refactoring is pretty straightforward.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8929061 [details]
Bug 1408152 - Sync the throbber animations again after the tab has finished moving.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200364/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f020bc218c8b
Sync the throbber animations again after the tab has finished moving. r=Gijs

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/f020bc218c8b