Add one-time infobar that indicates how to restore the previous session on the next session startup if automatic session restore is disabled
Categories
(Firefox :: Session Restore, enhancement, P3)
Tracking
()
People
(Reporter: Gijs, Assigned: sfoster)
References
Details
(Whiteboard: [fidefe-mr11-close-tabs])
Attachments
(2 files)
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
We can have the infobar button do nothing or just open the hamburger panel, and leave the rest of the implementation of the highlighting etc. to bug 1724962.
Comment 2•3 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox::Session Restore' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 3•3 years ago
|
||
Updating the bug title since we had decided to not restrict to restarts within 5 minutes of closing Firefox.
Comment 4•3 years ago
|
||
Note: Firefox 94 will have a spotlight modal on first session start-up. We want to avoid conflict with the infobar and also display the inforbar on the first session post not seeing the close tab modal. We need to show the infobar on the second session of running Firefox 94 for this reason.
Assignee | ||
Comment 5•3 years ago
|
||
Assignee | ||
Comment 6•3 years ago
|
||
I've got a WIP on here. It has some placeholder logic earlier during startup - before the SessionStartup updates the prefs we'll need to know if this is a restart etc. It has the new strings and creates the infobar at what I think is the right moment during startup, and the button opens the application menu.
So, the work remaining is really about determining when to show the infobar. And do I need to manually remove it if this browser navigates away from the start/home page?
Updated•3 years ago
|
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/563ff2a2a021 Add a one-time infobar to explain session restore. r=Gijs,fluent-reviewers
Comment 8•3 years ago
|
||
Assignee | ||
Comment 9•3 years ago
|
||
Ok thanks for the backout. Of all the things I considered, unexpectedly loading the Tahoma font was not one of them!
I'll have to investigate on Monday, as at first blush I have no idea what is going on there or why my patch would trigger this.
I see tahoma used in this rule for window
and maybe the infobar ends up getting its font-family from there? Seems unlikely that this infobar would use a font not already loaded in rendering the rest of our chrome, but its a place to start.
Reporter | ||
Comment 10•3 years ago
•
|
||
(edit: bugzilla was weird about the attachment upload, see next comment)
Reporter | ||
Comment 11•3 years ago
|
||
(In reply to Sam Foster [:sfoster] (he/him) from comment #9)
Ok thanks for the backout. Of all the things I considered, unexpectedly loading the Tahoma font was not one of them!
I'll have to investigate on Monday, as at first blush I have no idea what is going on there or why my patch would trigger this.I see tahoma used in this rule for
window
and maybe the infobar ends up getting its font-family from there? Seems unlikely that this infobar would use a font not already loaded in rendering the rest of our chrome, but its a place to start.
I checked but at least for me, I don't see us using Tahoma as a font for the infobar when inspecting with devtools, so I also don't understand why we'd hit this in xperf. :jfkthame, could you help investigate? I tried reproducing locally but xperf just fails miserably on my machine (see attached log)
That said, I think we should just set a pref in https://searchfox.org/mozilla-central/source/testing/profiles/perf/user.js to disable the infobar; we don't want it to show up in the test or any other perf tests. It does this because xperf first does a sanity run to initialize the profile, and then a "real" run, and so we end up showing it in the "real" run, so it shows up for the first page or two that get loaded in rapid succession (because of the timeout). That should work around the issue and allow us to land. It'll only show up on runs where we open the notification bar and anyway is likely pre-existing / applies to other notification bars, so not a "real" performance regression introduced by this patch in that sense. :sfoster, could you try to update the patch w/ that and trypush and/or reland?
Assignee | ||
Comment 12•3 years ago
|
||
Thanks, Gijs. I added support for an explicit opt-out value of -1 (as setting to '2' to opt-out seemed a bit odd/arbitrary) and set that pref for the testing user. AFAICT this try push ran the tests that previously failed and they don't fail any more: https://treeherder.mozilla.org/jobs?repo=try&revision=1eea02094084a3a7dbd504ff979ef3b3fc49a89c
Comment 13•3 years ago
|
||
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/613de0c29596 Add a one-time infobar to explain session restore. r=Gijs,fluent-reviewers
Comment 14•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Hello! I can confirm that the issue is fixed with firefox 94.0b8 and 95.0a1(2021-10-20) on all OS's.
I will update the flags and mark this issue as VERIFIED->FIXED. If the issue is still valid please feel free to revert the resolution.
Thank you!
Description
•