1893568 - Every other new tab does not smoothly slide in

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Gregory Pappas [:gregp]]

Attachment: tab_animation_bug.webm

Steps to reproduce:

1. Open a new tab
2. Open another new tab
3. Open another new tab
4. Open another new tab
5. Open another new tab
6. Open another new tab

Actual results:
Every other tab will not smoothly slide in

Expected results:
Every tab will smoothly slide in

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1848715

:mak, since you are the author of the regressor, bug 1848715, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Pascal Chevrel:pascalc]

Could this bug get a severity set? Thanks

Comment 3

[Marco Bonardo [:mak]]

it's a visual problem (missing animation), thus I'd call it S4. We must look into why it's happening, maybe input change causing a reflow?
If it depends on bug 1848715 activation (browser.urlbar.untrimOnUserInteraction.featureGate) it's Nightly only, but it would be better to also verify that.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1848715

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Marco Bonardo [:mak] from comment #3)

it's a visual problem (missing animation), thus I'd call it S4. We must look into why it's happening, maybe input change causing a reflow?
If it depends on bug 1848715 activation (browser.urlbar.untrimOnUserInteraction.featureGate) it's Nightly only, but it would be better to also verify that.

I can reproduce in current Nightly, under all the following configurations:

• fresh profile
• browser.urlbar.trimHttps disabled
• browser.urlbar.untrimOnUserInteraction.featureGate disabled
• both prefs disabled

Comment 6

[Gregory Pappas [:gregp]]

[Tracking Requested - why for this release]:
Since the problem still occurs when the pref is disabled, should the bug 1848715 be backed out from beta to avoid shipping a visual regression to release?

Comment 7

[Daniel Holbert [:dholbert]]

I'm inclined to agree. I confirmed the regression range is just bug 1848715, running mozregression with browser.urlbar.trimHttps set to false.

mak: kind of a tight turnaround given that 127 goes to release next week, but given this regression, does it seem feasible to back it out (probably from Nightly and beta), given that it had unforeseen visual regression on release builds beyond the nightly-specific-for-now browser.urlbar.trimHttps configuration that I think bug 1848715 was meant to target?

Comment 8

[Daniel Holbert [:dholbert]]

(side note: it looks like it's not an entirely cleanly-backout-able change, so it'll require some merge-conflict resolution, it seems.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: screencast, comparing Firefox release (126.0.1, no bug) vs beta (127, bug)

Here's a screencast comparing release (first half of the video) vs. beta 127 (second half of the video). Both using builds from Mozilla (not the ubuntu snap or anything), both using a fresh profile.

Release 126.0.1 is fine -- the tab opening animation is pretty consistent.

Beta 127 shows the issue -- every other tab seems to blink into existence.

Relatedly (I think), the new-tab content seems to reliably flicker pretty much every time we do play the tab-open animation, in beta. Whereas on Release, that flicker occasionally happens but hardly ever (only once in my screencast for release 126, I think, at around t=9s).

Comment 10

[Daniel Holbert [:dholbert]]

In case a profile is useful to see what's going on here... Here's a profile of the bug, in current Nightly with a fresh profile and both prefs from comment 5 set to false (plus a browser restart):
https://share.firefox.dev/3X87S6I

This profile shows three open-new-tab operations. The first and third play the new-tab-animation as expected (you can see the tab gradually growing as you mouse across the screenshot track), but the second one (at timestamp ~t=2.2s) just pops into existence.

Comment 11

[Daniel Holbert [:dholbert]]

If you look in the marker view for the profile, for the parent process or privileged content process, you can see that there are CSS transition markers for max-width and opacity for the first and third tab-open operations, but not for the second one (the one where the tab blinks into existence).

So that's further confirmation that we're really failing to detect that there's anything to be transitioned there, for whatever reason. (No idea if this is the cause here, but sometimes that's an indication that we're being given the "end" CSS without the start styles ever having been flushed, which means the style-system [correctly] doesn't see any reason for a transition to play.)

Comment 12

[Marco Bonardo [:mak]]

no, a backout isn't trivial. Honestly I'm confused how my changes could even have an impact on tab animations, especially with the prefs disabled.
Let me spend some time on this today and hopefully I can figure out more.

Comment 13

[Marco Bonardo [:mak]]

I still don't know why it matters, but the change causing this is that before we were invoking gURLBar.setSelectionRange regardless, while now we first check if the browser had a selection state stored.
Just invoking this.setSelectionRange(0, 0); when there's no stored selection for the browser solves the problem.
There must be code depending on the previous behavior.

Comment 14

[Marco Bonardo [:mak]]

Attachment: Bug 1893568 - Tab open animation is skipped with preloaded content. r=dao

Comment 15

[Marco Bonardo [:mak]]

Another fact is this only happens for about:newTab. Using about:blank or Tabliss as new tab animates fine.

Comment 16

[Marco Bonardo [:mak]]

usingPreloadedContent is alternatively set to false at https://searchfox.org/mozilla-central/rev/b476ffaef761ff85c012e2d93050cf444ff7be34/browser/components/tabbrowser/content/tabbrowser.js#2397 when we don't set selection to 0, 0. So with the bug, when opening tabs I see false, true, false, true... and the animation only happens when it's false.
With the workaround (setting 0, 0) usingPreloadedContent is always true and animation always happens.

In the bogus case we are alternatively skipping maybeCreatePreloadedBrowser as _handleNewTab is not invoked (on_transitionend is not fired) so maybeCreatePreloadedBrowser is not always invoked:
https://searchfox.org/mozilla-central/rev/b476ffaef761ff85c012e2d93050cf444ff7be34/browser/components/tabbrowser/content/tabs.js#2067-2069
This seems more like a consequence of transition not happening, and it's likely the cause for the flicker of new tab page.

Comment 17

[Gregory Pappas [:gregp]]

The problem goes away when I turn off browser.newtab.preload fwiw

Comment 18

[Marco Bonardo [:mak]]

It is likely a timing problem. If I delay t.setAttribute("fadein", "true");, even just by doubling RAF, e.g.:

        requestAnimationFrame(function () {
          requestAnimationFrame(function () {
            // kick the animation off
            t.setAttribute("fadein", "true");
          });
        });

then the animation happens every time regardless of the setSelectionRange call. Was that call adding sufficient delay to hide this problem?
using setTimeout(0) also works.

Comment 19

[Marco Bonardo [:mak]]

Some interesting comments in old https://bugzilla.mozilla.org/show_bug.cgi?id=850163.

Comment 20

[Marco Bonardo [:mak]]

Suspicion is that when newtab is preloaded, we somehow end up setting "fadein" too early (before the fix we had setSelectionRange(0,0) call in the middle), we don't get transitionend, and we don't call _handleNewTab. That makes us not create a preloaded browser, and then the next new tab is not preloaded so fadein is set sufficiently late, transitionend happens and the next one is preloaded. And so on.
I'm still missing some glue in the middle of this theory though, I can't tell why it's too early and why setSelectionRange(0,0) made it good.

Comment 21

[Pascal Chevrel:pascalc]

Tracking for a potential uplift into the planned dot release.

Comment 22

[Donal Meehan [:dmeehan]]

:mak we are in the last week of beta for Fx128, we have little time to fix this.
Any updates or expectations on the fix here?

Comment 23

[Marco Bonardo [:mak]]

I'm about to land the fix, then we can start uplifting.

The impact of the problem is limited because the use-cases where you must open multiple consecutive new tab pages are not many... And in any case important things will work, it's just the preloading that is broken so every other consecutive new tab page may take a bit longer. And of course the animation is not smooth.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44bc525a4bdf
Tab open animation is skipped with preloaded content. r=dao,tabbrowser-reviewers

Comment 25

[Marco Bonardo [:mak]]

Attachment: Bug 1893568 - Tab open animation is skipped with preloaded content. r=dao

Original Revision: https://phabricator.services.mozilla.com/D212679

Comment 26

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Every other time the new tab animation is skipped and the new tab page is not preloaded
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Open consecutive new tabs, check the tab slide in animation
• Risk associated with taking this patch: low
• Explanation of risk level: We may lose a frame of the tab animation
• String changes made/needed: none
• Is Android affected?: no

Comment 27

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/44bc525a4bdf

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c6f02954b1de

Comment 29

[Ciprian Georgiu, Desktop QA]

Hi Marco! I've verified this issue with latest Nightly 129.0a1 and Beta 128.0b8 on Win 11. It seems that the slide animation is still not smooth when openting multiple new tabs with the mentiond builds on fresh profiles.

Can you please take a look at this? You can see the behavior obseerver in a screencast, here.

Comment 30

[Marco Bonardo [:mak]]

There is a timing problem that is not trivial to address here.
We're setting an attribute on the tab, that is supposed to start the tab animation, and there is code that depends on the tab animation happening.
There is a timing issue for which when new tab content is preloaded setting the attribute doesn't kick off the animation. As a consequence we also alternatively not preload new tab.

Before the patch landed here, I could clearly see the tab animation being completely skipped every other time (transitionend not happening), and dumping just after this._createBrowserForTab I could alternatively see usingPreloadedContent: false.
After the patch I see the tab animation every time, and I constantly see transitionend happening and usingPreloadedContent: true being set.

Also in your video I see the tab animation happening constantly, while before it was completely skipped (tab appearing from nowhere) every other time.

The animation may not be 100% smooth, because to address the timing problem itself we are eating up 2 frames of it.
The newtab flicker may also be a problem related to preloading. There is surely more to investigate there.

Based on this, I think the original concern of the animation being skipped and causing newtab to not be preloaded every other time is addressed. For QA is a bit more complicate to assess that, as you are not supposed to dump js objects, thus I'd suggest to stick to ensuring the animation happens, compared to the previous alternate state.
The situation should have improved sufficiently, but I agree there's still something off.

As there are a few more things to investigate and understand, I'll file a follow-up bug where we can ask people with deeper knowledge about Layout and preloaded new tab for an opinion. That should also investigate this new tab flicker.