1388387 - More aggressively switch to the next tab when closing a tab, even with a beforeunload handler

Status: NEW

(Firefox :: Tabbed Browser, enhancement, P3)

Description

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

In bug 1355426, I made it so that we switch to the next tab sooner when removing a tab. This shaved a few milliseconds off of displaying the next tab to the user.

In bug 1355426 comment 12, dao noticed that with the original version of the patch, if the closing tab had a beforeunload event handler, we would switch early, and then switch _back_ to the closing tab to ask the user whether or not to remove the tab, causing a brief flicker. I then modified the patch to work around this by moving the early switch until after we had determined whether or not a beforeunload check was required (and what the result was).

In bug 1355426 comment 33, florian suggests that this flicker was worth it, since it causes us to show the next tab even earlier in the good (and arguably more common) case. He suggested we could mitigate the flicker by reducing the amount of time we're willing to wait for content to response to the beforeunload message.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from bug 1355426 comment #33)
> I was hoping this bug would fix the common case, and cause flicker in the
> uncommon case, in a way that would make the user think the site is getting
> in the way: the perception should be "this site is a pain" rather than
> "Firefox is slow to close tabs."

I have to disagree. beforeunload is an established web feature with quite valid use cases e.g. to prevent data loss. We can't just treat this like an unwanted stepchild. Even if there was no good reason to use beforeunload, the blame-shifting still wouldn't work. Users are smart enough try different browsers and then rightfully blame Firefox for the flicker.

Comment 2

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

So I think there's a nuance of florian's plan from bug 1355426 that's gotten lost here. Perhaps I was the only one that missed it, but I want to make sure it's stated here clearly.

There are two cases to consider - and the second one has two sub-cases:

1) The tab we're closing has no beforeunload handler
2) The tab we're closing has a beforeunload handler
  2a) The beforeunload handler will run very soon because the content process is responsive
  2b) The beforeunload handler will run after the content process frees up (which is unbounded time)

Case (1) is easy - we blur the closing tab right away, and we're done.

florian's plan calls for us to detect and handle case (2) slightly differently - if we notice that there's a beforeunload handler, we wait some short amount of time (50 - 100ms was his suggestion) before blurring the tab.

This means that in case (2a), if content requests that we block the tab closing, it responds soon, and we can cancel the 50ms - 100ms timeout, and there is no flicker.

In the bad case, where content takes a while to get back to us, we blur the tab after 50 - 100ms, and then once content responds, then we switch back.

That's what I believe florian was expressing.

I have another alternative in the event that we're still worried about flicker in that last case - perhaps in the event that the handler responds late with "don't unload" after the 50ms - 100ms timeout, instead of switching back, we highlight the tab instead and keep it open. This is similar to what we do when a background tab opens a modal prompt.

Do either of these two plans sound preferable, dao?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #2)
> So I think there's a nuance of florian's plan from bug 1355426 that's gotten
> lost here. Perhaps I was the only one that missed it, but I want to make
> sure it's stated here clearly.
> 
> There are two cases to consider - and the second one has two sub-cases:
> 
> 1) The tab we're closing has no beforeunload handler
> 2) The tab we're closing has a beforeunload handler
>   2a) The beforeunload handler will run very soon because the content
> process is responsive
>   2b) The beforeunload handler will run after the content process frees up
> (which is unbounded time)
> 
> Case (1) is easy - we blur the closing tab right away, and we're done.
> 
> florian's plan calls for us to detect and handle case (2) slightly
> differently - if we notice that there's a beforeunload handler, we wait some
> short amount of time (50 - 100ms was his suggestion) before blurring the tab.
> 
> This means that in case (2a), if content requests that we block the tab
> closing, it responds soon, and we can cancel the 50ms - 100ms timeout, and
> there is no flicker.
> 
> In the bad case, where content takes a while to get back to us, we blur the
> tab after 50 - 100ms, and then once content responds, then we switch back.
> 
> That's what I believe florian was expressing.

I think comment 1 still mostly applies in the bad 2a case. The content process may be slow for reasons unrelated to the closing tab, and even if that page itself does stupid expensive stuff I'm not convinced that blame-shifting works. At the end of the day it would still be weird Firefox-specific behavior that other browsers presumably don't have. (I'm guessing most browsers have implemented something similar to our current behavior.)

> I have another alternative in the event that we're still worried about
> flicker in that last case - perhaps in the event that the handler responds
> late with "don't unload" after the 50ms - 100ms timeout, instead of
> switching back, we highlight the tab instead and keep it open. This is
> similar to what we do when a background tab opens a modal prompt.

This sounds more reasonable to me.

Comment 4

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

ni?ing myself to run this by UX to get their take.

Comment 5

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

Hey phlsa,

I've been trying to raise you on Slack, but haven't had luck there, so I'm going to try this needinfo. :)

florian, dao and I are trying to figure out if it's possible to make tab closing feel even faster in the case where a closed tab has set the beforeunload handler (and will show the dialog that asks the user, "Are you sure you want to close this tab?")

The idea is that we switch to the next tab immediately while the tab figures out whether or not it needs to show the dialog. Once it determines that the dialog needs to be shown, it highlights the tab in the background. This means that the tab that was closed by the user doesn't actually close in this case - dao's a bit worried about that. The advantage to doing things this way though is that in the majority case (where there's no need to display the dialog), the tab closing experience will feel faster because the user will have switched to the next tab more quickly.

Do you have any concerns with this approach?

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Hm, the idea is interesting! A few questions though:

Do we have any idea of how frequently this dialog appears (as % of all tab close events)?
Also, how big a gain would we expect from this?

And for the proposed highlighting of the tab, I assume we'd use the neon dot under the favicon, right?

Comment 7

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

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #6)
> And for the proposed highlighting of the tab, I assume we'd use the neon dot
> under the favicon, right?

Yes, I imagine it'd use the same styling rules as background tabs showing alert / confirmation dialogs - so, glow under favicon, and bolded label.

To see what that looks like, open up two tabs, and in the second tab, put this in the devtools console:

setTimeout(() => alert("2 seconds have passed"), 2000)

Press enter, then quickly switch to the other tab.

Does that sound like an optimization worth pursuing here?

Comment 8

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

Sorry, forgot to answer those other question:

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #6)
> Do we have any idea of how frequently this dialog appears (as % of all tab
> close events)?

Only a very, very coarse idea I think. We collect opt-in Telemetry on how often tabs close, and how often they close while we're waiting for a permitUnload result.

WARNING: Incoming vague napkin math. Take these with a grain of salt.

Looking at tabs closing with animations over the past month or so:

https://mzl.la/2f059cQ

We have 241.51 million tab closures recorded across the users sending us Telemetry on the Beta channel.

https://mzl.la/2eZoScZ

We have 40.65 million tab closures recorded without animation.

That's a total of ~281 million tab closures of one kind or another.

https://mzl.la/2eZowDb

So we apparently have 265 million reports of permitUnload being processed when attempting to close a tab.

There are huge error bars here, and I'm not doing much in the way of validation here - I'm just trying to sew some numbers together to give a coarse answer. But 265 / 281 isn't nothing, assuming we can trust these numbers and I haven't screwed up.

That only gives us a vague sense of how many pages might have _tried_ to show the beforeunload dialog. Unfortunately, I don't think we have data on how often the dialog is actually shown.

> Also, how big a gain would we expect from this?

Showing the dialog isn't the expensive part - it's the messaging to the content to ask whether or not we should show one, and that is something we do measure. Looking at https://mzl.la/2eZowDb, it looks like (at least on beta) oftentimes it seems to be cheap, but there's a super long tail. The 95th percentile is at 92.54ms, so I think that means that if we do this fix, in the worst-performing case (for this 95th percentile), we could potentially save almost 100ms. In the best-performing case, we'd probably save very little.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Thanks for the breakdown! OK, that sounds reasonable.

About the proposed solution to keep the tab around (highlighted) in the background:
Would that mean that the tab disappears and then re-appears, or would it just stick around in the background?
Also, is there a timeout where we definitively kill a tab? It would be really odd if a tab came back after multiple seconds when the user already started to focus on something else.

Comment 10

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

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #9)
> Thanks for the breakdown! OK, that sounds reasonable.
> 
> Would that mean that the tab disappears and then re-appears, or would it
> just stick around in the background?

The simplest implementation would cause the tab to stick around in the background.

> Also, is there a timeout where we definitively kill a tab?

We have a timeout of 1s for the content process to respond to the permitUnload request, but once it responds, the page currently has as much time as it needs in order to run its beforeunload handler.