The default bug view has changed. See this FAQ.

Add a button for session restore to the tab bar

NEW
Unassigned

Status

()

Firefox
Session Restore
P3
normal
a year ago
3 months ago

People

(Reporter: phlsa, Unassigned, Mentored)

Tracking

(Blocks: 2 bugs, {leave-open})

Trunk
leave-open
Points:
13
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected)

Details

(Whiteboard: [qx:spec][outreachy-12])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8680588 [details]
Restore Button.png

Session restore is a compelling feature for a large audience, but it is very hard to discover at the moment.

We can add a button in the tab bar when the user starts a new session that will restore the previous session. A spec is attached.

Behavior:
- The button only shows up if automatic session restore is off
- The button fades out once the user opens a new tab
- If the window is scaled down so that the button wouldn't fit anymore, it disappears temporarily (re-appears once the window is big enough again)
Cool.
Good idea.

Comment 3

a year ago
As a bonus, can we detect people doing this repeatedly and offer UI (notification bar? Something else?) that allows people to change the startup preference to always restore their session?
Flags: needinfo?(philipp)
(In reply to :Gijs Kruitbosch from comment #3)
> As a bonus, can we detect people doing this repeatedly and offer UI
> (notification bar? Something else?) that allows people to change the startup
> preference to always restore their session?

<3
Let's say, if you restore your third consecutive session, you immediately get a notification bar:
Do you want Firefox to automatically restore your tabs in the future? [Sure!] [No Thanks]
Flags: needinfo?(philipp)
I very much agree that Session Restore is a compelling feature that we should figure out how to expose/promote more, but I'm not sure jamming another button into the toolbar is the right approach. (I wouldn't really object to having a button that's just in the customization palette by default, but that doesn't really help the discoverability issue.)

A few alternative ideas:

* Just flip the default to "Restore my tabs" for new profiles. (But doesn't help existing users, and I'm not sure if we should worry about users being surprised, in a bad way, by having their tabs reopened.)

* Restore tabs by default, but show a notification bar (?) saying "Hey, we restored your stuff, if you don't want this [Click here] to turn it off."

* Don't (actually) restore by default, but show a notification bar the first N runs offering to do so. [This could be interesting for promoting the feature to _existing_ Firefox users in a gentile way.]

The last two have the downside of Yet Another Notification Bar that new users are exposed to.

Comment 6

a year ago
I have been waiting a long time for an easier way to restore last session but I would rather have a button on the newtab page. A button on the tab bar would also work but as I use it only occasionally it would be good to also see what sites were in the last session.
(In reply to Justin Dolske [:Dolske] from comment #5)
> I very much agree that Session Restore is a compelling feature that we
> should figure out how to expose/promote more, but I'm not sure jamming
> another button into the toolbar is the right approach. (I wouldn't really
> object to having a button that's just in the customization palette by
> default, but that doesn't really help the discoverability issue.)

I don't think that will be an issue since the button is pretty transient. By definition, it will only show up when the user has just one tab (or perhaps two when there's a first run page). We can make it even more transient by making it disappear once an additional tab is opened, if that's the issue.

If we notice that that button gets really high usage, flipping the pref to make session restore the default is a possible next step, but it will help us determine the usefulness of session restore to the broader public.
Blocks: 1244854
Priority: -- → P3
What is the harm in enabling session restore by default (for users that still have the default "Show my home page" value)? Users wouldn't lose their tabs. The disadvantages are a possible startup time regression and "eternal session" cookie bug 530594.
Mentor: jaws@mozilla.com
Points: --- → 13
Whiteboard: [qx:spec] → [qx:spec][outreachy-12]

Updated

9 months ago
Assignee: nobody → ewright

Comment 9

9 months ago
Created attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review commit: https://reviewboard.mozilla.org/r/60284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60284/
Attachment #8764330 - Flags: review?(jaws)

Updated

9 months ago
Attachment #8764330 - Flags: ui-review?(shorlander)
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review57470

Thanks for the patch, just some notes here that should help you on your second round. Sorry for the delay in review, please request review from :dao or :gijs on your next version as I will be on PTO next week.

This patch doesn't apply at all locally for me, and the patch doesn't specify the parent revision so I'm not sure what mozilla-central revision you're working off of.

Please remove the Restore Session button from about:home in this patch. We shouldn't have two places in our UI that do identical things.

We should have automated tests with this.

::: browser/base/content/browser.css:154
(Diff revision 1)
> +#restore-tabs-button-wrapper {
> +  padding: 4px;
> +  margin-left: 3px;
> +}
> +
> +#restore-tabs-button > .toolbarbutton-multiline-text{
> +  display: -moz-box;
> +  font-size: .8em;
> +  margin-bottom: 0;
> +}
> +
> +#restore-tabs-button {
> +  border: 1px solid var(--urlbar-separator-color);
> +  border-radius: 2px;
> +  display: none;
> +  transition: opacity 1s ease-out;
> +}
> +
> +#restore-tabs-button.hidden {
> +  opacity: 0;
> +}

These rules should all be in theme CSS, not in /browser/base/content/.

A 1 second transition is too long. As developers we often tend to make transitions too long because we like seeing our work and inherently want others to see our work. Please lower this to .3 seconds.

Please don't use 'hidden' as a className, as it is often used as a property which indirectly sets display:none. After the item has opacity:0; should it become display:none;? As it is now, elements with opacity:0; still consume space. You can see this by making the Firefox window very narrow.

::: browser/base/content/browser.js:7480
(Diff revision 1)
> +      this.showButton();
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -
> +  showButton: function () {

As this code is only called from the init function, I would prefer if it remained inside of init.

::: browser/base/content/browser.js:7490
(Diff revision 1)
> +    function fadeRestoreTabsButton() {
> +      gBrowser.tabContainer.removeEventListener("TabOpen", fadeRestoreTabsButton, false);
> +      restore_tabs_button.addEventListener("transitionend", removeRestoreTabsButton, false);
> +      restore_tabs_button.classList.add("hidden");
> +    }
> +    let restore_tabs_button = document.querySelector("#restore-tabs-button");

Please use document.getElementById here.

::: browser/base/content/browser.js:7491
(Diff revision 1)
> +      gBrowser.tabContainer.removeEventListener("TabOpen", fadeRestoreTabsButton, false);
> +      restore_tabs_button.addEventListener("transitionend", removeRestoreTabsButton, false);
> +      restore_tabs_button.classList.add("hidden");
> +    }
> +    let restore_tabs_button = document.querySelector("#restore-tabs-button");
> +    restore_tabs_button.style.display = "-moz-box";

The button should have hidden="true" in the markup, and here you can call
`restore_tabs_button.hidden = false`

::: browser/base/content/tabbrowser.xml:4909
(Diff revision 1)
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="restorePreviousTabs();"/>

The label for the button should be defined in the markup here, and thus the string would need to be defined in a DTD file.

Also, please double-check that you intend to use .toolbarbutton-multiline-text here. It seems to me that it would be preferable to use .toobarbutton-text since it will probably "break" our tabbar if the text starts to wrap.

::: browser/base/content/tabbrowser.xml:4930
(Diff revision 1)
>            var tab = this.firstChild;
>            tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle");
>            tab.setAttribute("crop", "end");
>            tab.setAttribute("onerror", "this.removeAttribute('image');");
>  
> +          var restore_tabs_button = this.querySelector("#restore-tabs-button");

nit, please use document.getElementById

::: browser/locales/en-US/chrome/browser/tabbrowser.properties:50
(Diff revision 1)
>  
>  # LOCALIZATION NOTE (tabs.allowTabFocusByPromptForSite):
>  # %S is the hostname of the site where dialogs are allowed to switch tabs
>  tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to take you to their tab
> +
> +tabs.restoreTabs=Restore tabs from last time

Are we able to reuse the string from about:home?
Attachment #8764330 - Flags: review?(jaws) → review-

Comment 11

9 months ago
Do you have any advice on where/how to add tests for this? :bwinton is not much help ;p
Flags: needinfo?(gijskruitbosch+bugs)

Comment 12

9 months ago
(In reply to Erica from comment #11)
> Do you have any advice on where/how to add tests for this? :bwinton is not
> much help ;p

You'd want to add a browser-chrome mochitest ( https://developer.mozilla.org/en-US/docs/Browser_chrome_tests ) in a subdir of:

browser/base/content/tests/

The right thing might be to create a tabbrowser directory there and put it in there. You'll then also need to create a 'browser.ini' file in there to list the new dir, and then add the browser.ini file in the browser/base/moz.build file's list:

http://searchfox.org/mozilla-central/source/browser/base/moz.build#17-28

We really need to get away from the dumping ground that the 'general' subdir in there has become (I'm pretty sure there's a fairly large number of bugs in there that we could then move to the newly-created directory in a separate bug).

Alternatively, because the bug is at least vaguely related to sessionstore, you could also add it in browser/components/sessionstore/test/ .

Please name the file browser_tabbar_sessionrestore_button.js or something like that, rather than using the browser_bug1219725.js pattern which we're trying to slowly move away from.

For a hopefully vaguely useful instance that uses some of the helpers you might want/need, see e.g. http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_newtab_userTypedValue.js .

Let me know if you need more help than that!
Flags: needinfo?(gijskruitbosch+bugs)

Comment 13

9 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/1-2/
Attachment #8764330 - Flags: review- → review?(gijskruitbosch+bugs)

Comment 14

9 months ago
https://reviewboard.mozilla.org/r/60284/#review57470

I wrote some tests, they don't cover all the things I would like to test as I struggled a lot in them (any suggestions appreciated). Shorlander has a different opinion on removing the Restore Session button as the one in the tab bar is temporary - have left it in for now till others make the decision.

Comment 15

9 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

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

Comment 16

9 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/3-4/
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Looks good, I noticed a few problems:

- If you only restore one tab the button doesn't disappear
- We probably shouldn't offer to restore a session that would be the same as a default session
- It looks like a button but doesn't pick-up styling form any of our existing buttons
  -- Uses sentence case, buttons should use title case
  -- The square shape of the button looks a little bit odd next to the curved tab, so while I am not super excited about adding another one-off styling, I think we should make it rounded and a little subtle (see mockup)
- Not sure if it's due to this patch, but I am missing the new tab button?
Attachment #8764330 - Flags: ui-review?(shorlander) → ui-review-
Created attachment 8766890 [details]
Restore Button Styling - 01
(In reply to Chris Peterson [:cpeterson] from comment #8)
> What is the harm in enabling session restore by default (for users that
> still have the default "Show my home page" value)? Users wouldn't lose their
> tabs. The disadvantages are a possible startup time regression and "eternal
> session" cookie bug 530594.

I think we should do this. I believe we turned this off by default a long time ago when we still tried to load all tabs at once. Is there a reason we can't do this?
Flags: needinfo?(dolske)

Comment 20

9 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/4-5/

Comment 21

9 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review58560

This is already a pretty nice patch. I've added feedback below to suggest tweaks to various things. However, I think it would be wise to await a decision about whether or not we can enable session restore by default before acting on the feedback. It would be a shame if you spend even more time and folks then decide to just do something else instead. :-(

::: browser/base/content/browser.css:137
(Diff revision 5)
>  }
>  
>  #tabbrowser-tabs:not([overflow="true"]) ~ #alltabs-button,
>  #tabbrowser-tabs:not([overflow="true"]) + #new-tab-button,
>  #tabbrowser-tabs[overflow="true"] > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> -#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs,new-tab-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> +#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs"]):not([currentset*="alltabs-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,

Can you elaborate on what this change accomplishes? I'm a bit confused as to how it relates to what you're doing here.

::: browser/base/content/browser.js:7493
(Diff revision 5)
> +      let restore_tabs_button_wrapper = document.getElementById("restore-tabs-button-wrapper");
> +      let restore_tabs_button = document.getElementById("restore-tabs-button");

Nit: please use camelCase, and see other notes about how to get these elements.

::: browser/base/content/browser.js
(Diff revision 5)
> +      gBrowser.tabContainer.addEventListener("TabOpen", fadeRestoreTabsButton, false);
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -

Nit: leave the blank line alone. :-)

::: browser/base/content/tabbrowser.xml:4934
(Diff revision 5)
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"

We shouldn't have elements with an id inside XBL bindings. You can find elements inside the content from the XBL binding by giving them an anonid attribute and using:

```
document.getAnonymousElementByAttribute(referenceToContainerElement, "anonid", "myanonidvalue");
```

Note that in this case, for the `referenceToContainerElement` you could likely use the `tabsContainer` property of the tabbrowser binding, or perhaps `this` if you're inside the tabbrowser-tabs binding.

You will also see that the binding has some <field>s declared that point to its anonymous content. It seems like it might be useful to create one for this button as well, to make it easier for outside code to modify its classes, as the session restore code is doing.

::: browser/base/content/tabbrowser.xml:4938
(Diff revision 5)
> +
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"

Do we need this class? I don't think it's quite right...

::: browser/base/content/tabbrowser.xml:4939
(Diff revision 5)
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="restorePreviousTabs();"

I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)

::: browser/base/content/tabbrowser.xml:5005
(Diff revision 5)
>          </getter>
>        </property>
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;

Unfortunately this particular line has several problems. Our toolbars are customizable, so the first child of the tabstoolbar isn't necessarily the tabstrip. If you want the tabstrip, you can actually just refer to `this` in this method, as it lives on the XBL binding for the tabstrip.

::: browser/base/content/tabbrowser.xml:5006
(Diff revision 5)
>        </property>
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;

Nit: use camelCase.

Also, naming-wise, I'm a bit confused that we have a method called "checkTabStripLength" and it returns "shouldHide", which isn't a natural answer to the question. Maybe rename the method to something that reflects what question it's trying to answer?

::: browser/base/content/tabbrowser.xml:5007
(Diff revision 5)
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;
> +          let wrapper = document.getElementById("restore-tabs-button-wrapper");

As noted elsewhere, you want to be using getAnonymousElementByAttribute. Sorry for the double feedback compared to what Jared said earlier. :-(

::: browser/base/content/tabbrowser.xml:5008
(Diff revision 5)
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;
> +          let wrapper = document.getElementById("restore-tabs-button-wrapper");
> +          if (wrapper.classList.contains('session-exists') && this.boxObject.width > 500 ) {

The 500 here (and the 600 below) is a bit of a code smell. What does the magic number mean, and why is it here? :-)

Nit: feels like you could have an outer 'if' on `wrapper.classList.contains('session-exists')` and an inner if...else for the other contents here...

Nit: when storing state, we tend to use an attribute rather than a class. So you could set the "session-exists" attribute to "true" instead.

::: browser/base/content/tabbrowser.xml:5011
(Diff revision 5)
> +            //if the search bar is in the toolbar, show the restore tabs button
> +            //even though it encroaches on the tab

I'm a little confused by this comment. You're mentioning the search bar, but you're not directly referring to it, as far as I can tell? Can you explain what's going on here?

::: browser/base/content/tabbrowser.xml:5013
(Diff revision 5)
> +          if (wrapper.classList.contains('session-exists') && this.boxObject.width > 500 ) {
> +            should_hide = false;
> +
> +            //if the search bar is in the toolbar, show the restore tabs button
> +            //even though it encroaches on the tab
> +          } else if (wrapper.classList.contains("session-exists") && this.parentNode.boxObject.width > 600) {

The 600 here is a bit of a code smell. What does the magic number mean, and why is it here? :-)

::: browser/base/content/tabbrowser.xml:5018
(Diff revision 5)
> +          } else if (wrapper.classList.contains("session-exists") && this.parentNode.boxObject.width > 600) {
> +            while (node && node.nodeType === 1) {
> +              if (node.flex > 1) {
> +                should_hide = false;
> +              }
> +              node = node.nextElementSibling || node.nextSibling;

Nit: because you're also checking node.nodeType, there are two ways this can pan out:

1) we get a nextElementSibling and we go through the loop once more
2) we don't get a nextElementSibling. We assign node.nextSibling instead, but even if there were a non-node sibling, the nodeType check in the loop condition will fail and we break out of the loop anyway.

IOW, I don't think you need the `|| node.nextSibling` bit. I'm also not sure there are actual non-element nodes in the tabstrip at any point, tbh...

::: browser/base/content/tabbrowser.xml:5458
(Diff revision 5)
>                if (width != this.mTabstripWidth) {
>                  this.adjustTabstrip();
>                  this._fillTrailingGap();
>                  this._handleTabSelect();
>                  this.mTabstripWidth = width;
> +                document.getElementById('restore-tabs-button-wrapper').hidden = this.checkTabStripLength();

All the callers to this method do this. Feels like the method should just be called something like "updateSessionRestoreVisibility()" and set .hidden itself, instead of burdening the consumers to all do exactly the same thing.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

We don't normally license test files anymore, or if we do, we use public domain.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:11
(Diff revision 5)
> +  let win = yield BrowserTestUtils.openNewBrowserWindow();
> +  let restore_tabs_button_wrapper = win.document.getElementById('restore-tabs-button-wrapper');
> +  let new_tab_button = win.document.getAnonymousElementByAttribute(win.gBrowser.tabContainer, 'class', 'tabs-newtab-button');
> +  is(new_tab_button.style.visibility !== 'collapse', true, 'new tab button is not visibility: collapse');
> +
> +  is(restore_tabs_button_wrapper.hidden, false, 'restore_tabs_button_wrapper shows initially');

I'm confused by this... do we show the button even when there's no session to restore?

Note also that you're implicitly relying on the window being > 500 or 600 pixels or whatever it is... we should probably not do that because if the machine on which the test runs doesn't have windows that large, this won't be the case.

::: browser/modules/BrowserUITelemetry.jsm:68
(Diff revision 5)
>      ],
>      "TabsToolbar": [
>        "tabbrowser-tabs",
>        "new-tab-button",
>        "alltabs-button",
> +      "restore-tabs-button",

I'm a bit confused. The button isn't in the placements at all (it isn't a customizable button, and it's not in the #TabsToolbar but inside #tabbrowser-tabs). Why did you need to add this?

::: browser/themes/shared/toolbarbuttons.inc.css:103
(Diff revision 5)
>  
>  #print-button[cui-areatype="toolbar"] {
>    -moz-image-region: rect(0, 414px, 18px, 396px);
>  }
>  
> +#restore-tabs-button > .toolbarbutton-icon {

Can you put all the CSS you added here at the end of tabs.inc.css instead? toolbarbuttons.inc.css is only used to pick icons for toolbarbuttons, and this code does other things. :-)

::: browser/themes/shared/toolbarbuttons.inc.css:115
(Diff revision 5)
> +  transition: opacity 300ms ease-out;
> +}
> +
> +#restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;

AFAICT this will be evaluated against font: message-box and so the font-size will be different depending on OS settings. Are we sure this will be readable and/or not too big? This might be a question for shorlander...

::: browser/themes/shared/toolbarbuttons.inc.css:121
(Diff revision 5)
> +  margin-bottom: 0;
> +  padding: 0 5px;
> +}
> +
> +#restore-tabs-button {
> +  border: 1px solid var(--urlbar-separator-color);

We shouldn't use urlbar-separator-color outside of the urlbar. I'm not sure what else to use - tabs separators use `currentColor`, but that also relies on a very low opacity, which would hide the text here. Can you see if there's something else we can rely on, or alternatively, pick 2 rgba colors, one for the 'normal' state and one for the inverted (ie dark high contrast or lightweight themes) ?
Attachment #8764330 - Flags: review?(gijskruitbosch+bugs)

Comment 22

9 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> The 600 here is a bit of a code smell. What does the magic number mean, and why is it here? :-)

Whoops, forgot to remove this duplicate.
(In reply to Stephen Horlander [:shorlander] from comment #19)
> (In reply to Chris Peterson [:cpeterson] from comment #8)
> > What is the harm in enabling session restore by default (for users that
> > still have the default "Show my home page" value)? Users wouldn't lose their
> > tabs. The disadvantages are a possible startup time regression and "eternal
> > session" cookie bug 530594.
> 
> I think we should do this. I believe we turned this off by default a long
> time ago when we still tried to load all tabs at once. Is there a reason we
> can't do this?

I don't think "Restore my tabs" was ever the default. There were some performance issues like you mention long ago (which perhaps was a limiting factor to enabling it by default), but that's fixed now that we only load restored tabs on-demand.

I'm also in favor of basically just enabling the pref (or some variation, see comment 5). But Philipp seemed to really want the button. :)

If we've basically got a finished patch, seeing how it's used is fine. Although it would be helpful to gather usage telemetry, and a plan for how we'd use that to get to a decision on enabling it for everyone (as comment 7 suggests). I'm not really clear what that would look like.
Flags: needinfo?(dolske)
(In reply to Erica from comment #20)
> Comment on attachment 8764330 [details]
> Bug 1219725 - Add a button for session restore to the tab bar. ,
> ui-r=shorlander
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/60284/diff/4-5/

Sorry, missed this. Does this patch address Comment 17?
Flags: needinfo?(ewright)

Comment 25

9 months ago
(In reply to Stephen Horlander [:shorlander] from comment #24)
> (In reply to Erica from comment #20)
> > Comment on attachment 8764330 [details]
> > Bug 1219725 - Add a button for session restore to the tab bar. ,
> > ui-r=shorlander
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/60284/diff/4-5/
> 
> Sorry, missed this. Does this patch address Comment 17?

Yes it does. 
 `If you only restore one tab the button doesn't disappear` - this is fixed (I forgot to test with the other restore button).

` We probably shouldn't offer to restore a session that would be the same as a default session` - I agree, but that is also the current behavior of the other restore session button.

`Uses sentence case, buttons should use title case` - now is picking up the same text from the other button 

`Not sure if it's due to this patch, but I am missing the new tab button?` - should be fixed - was behaving differently on different profiles (let me know if it's not)
Flags: needinfo?(ewright)
(In reply to Erica from comment #25)
> `Uses sentence case, buttons should use title case` - now is picking up the
> same text from the other button 

We should use the "Restore Tabs From Last Time" wording. It's an improvement over "Restore Previous Session" because "Session" is pretty jargon-y and ill-defined. The important thing to know is that it will give you the stuff (Tabs) that you had before.

Comment 27

9 months ago
(In reply to Stephen Horlander [:shorlander] from comment #26)
> (In reply to Erica from comment #25)
> > `Uses sentence case, buttons should use title case` - now is picking up the
> > same text from the other button 
> 
> We should use the "Restore Tabs From Last Time" wording. It's an improvement
> over "Restore Previous Session" because "Session" is pretty jargon-y and
> ill-defined. The important thing to know is that it will give you the stuff
> (Tabs) that you had before.

Will do. Waiting for some sort of consensus on whether or not it's worth continuing. (but if it is I'll change it)
You should proceed here with the work, don't wait for consensus. When you're patch is ready to land we can experiment with ways that we want to tweak it.
Yeah, we would want this regardless of what we decide on for the default behavior.

Comment 30

8 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> Unfortunately this particular line has several problems. Our toolbars are customizable, so the first child of the tabstoolbar isn't necessarily the tabstrip. If you want the tabstrip, you can actually just refer to `this` in this method, as it lives on the XBL binding for the tabstrip.

I am not specifically looking for the tab strip. I'm looking for all of the objects within the #TabsToolBar in order to find if any of the children have the flex attr > 1 (specifically the search bar.

> I'm a little confused by this comment. You're mentioning the search bar, but you're not directly referring to it, as far as I can tell? Can you explain what's going on here?

Edited the comment to be more clear. We are only showing the restoreTabsButton if there is room for it to not encroach on the existing tab's space. If any custom object with flex > 1 is in the toolbar, specifically the search bar, it will push the restoreTabsButton into the existing tab's space - in this case, this is allowed.

> I'm confused by this... do we show the button even when there's no session to restore?
> 
> Note also that you're implicitly relying on the window being > 500 or 600 pixels or whatever it is... we should probably not do that because if the machine on which the test runs doesn't have windows that large, this won't be the case.

agreed, since the first test doesn't set the windows size there is no telling what it may be, so it may or may not show up. For the other tests setting the window size is rather important because I need to be able to tell if there is room for the button to exist or not. Any advice on how to get around this?

> I'm a bit confused. The button isn't in the placements at all (it isn't a customizable button, and it's not in the #TabsToolbar but inside #tabbrowser-tabs). Why did you need to add this?

It was my impression this is where I would add it to track clicks for metrics. Is this wrong?

Comment 31

8 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> Can you elaborate on what this change accomplishes? I'm a bit confused as to how it relates to what you're doing here.

when a foreign thing gets added to the toolbar the `+` (new tab) button hides. A replacement new tab button will show up on the far right corner. I want the two buttons to co-exist. So changing the allowable `currentset` accomplishes this. I could see it possibly having a negative effect as it allows other people to add other things there too, which I believe is what is trying to be prevented.

Comment 32

8 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> I am not specifically looking for the tab strip. I'm looking for all of the objects within the #TabsToolBar in order to find if any of the children have the flex attr > 1 (specifically the search bar.

So I don't think this is the right way to accomplish this goal, and I also don't think this happens only with the search bar - or conversely, that it would happen with any element with flex > 1. It ends up having similar problems to the ones with the hardcoded 500/600 pixel required width.

What does the "encroaching" of the button actually end up meaning? In XUL you don't normally get overlapping of two siblings without a lot of work.

> agreed, since the first test doesn't set the windows size there is no telling what it may be, so it may or may not show up. For the other tests setting the window size is rather important because I need to be able to tell if there is room for the button to exist or not. Any advice on how to get around this?

You're generally assuming that an absolute window size is sufficient to determine whether or not there is room for the button. I think that is incorrect (because of OS font sizes, because of localization concerns, because tab widths are configurable via prefs, because of custom themes, etc.), and needs to be addressed in the code (and, after that, in the tests).

> It was my impression this is where I would add it to track clicks for metrics. Is this wrong?

Yeah, this list indicates which customizable widgets are in a particular area. What you're adding isn't a customizable widget, so it shouldn't be here.

Comment 33

8 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> So I don't think this is the right way to accomplish this goal, and I also don't think this happens only with the search bar - or conversely, that it would happen with any element with flex > 1. It ends up having similar problems to the ones with the hardcoded 500/600 pixel required width.
> 
> What does the "encroaching" of the button actually end up meaning? In XUL you don't normally get overlapping of two siblings without a lot of work.

The tab has a full-width state, and when the tabbar is filled up it has a smaller state that it switches to, I'm trying to avoid causing this smaller state to reduce a visual jump when the restoreButton fades away.
Can I suggest that if you are worried about a visual jump when the restoreButton fades away, maybe as part of the fade you can first transition the width of the restoreButton to 0 so that the tabs will animate their expansion?

Comment 35

8 months ago
https://reviewboard.mozilla.org/r/60284/#review58560

> I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)

if I want to call it from `tabbrowser.xml` then I need to include the line `let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);` before calling `restoreLastSession()` I suppose I could add a click listener to the element instead from `browser.js`

Comment 36

8 months ago
(In reply to Erica from comment #35)
> https://reviewboard.mozilla.org/r/60284/#review58560
> 
> > I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)
> 
> if I want to call it from `tabbrowser.xml` then I need to include the line
> `let ss =
> Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);`
> before calling `restoreLastSession()` I suppose I could add a click listener
> to the element instead from `browser.js`

Other code in tabbrowser.xml already relies on the SessionStore global existing in the window, so I don't think that's necessary. You can just call it directly via SessionStore.restoreLastSession() - the code in the binding has access to globals in the window where it's running.

Comment 37

8 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/5-6/
Attachment #8764330 - Flags: review?(jaws)

Comment 38

8 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/6-7/

Comment 39

8 months ago
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/7-8/

Comment 40

8 months ago
https://reviewboard.mozilla.org/r/60284/#review65094

::: browser/locales/en-US/chrome/browser/browser.dtd:261
(Diff revision 8)
>  
>  <!ENTITY historyButton.label            "History">
>  <!ENTITY historySidebarCmd.commandKey   "h">
>  
>  <!ENTITY toolsMenu.label              "Tools">
> -<!ENTITY toolsMenu.accesskey          "T"> 
> +<!ENTITY toolsMenu.accesskey          "T">

note: my editor stripped the trailing whitespace in this file, figured I'd leave it.
Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review65164

I only reviewed the diff from v5-v8.

The button has the wrong styling on Windows 10. It is missing the border that is shown on the spec, http://screencast.com/t/6vkYTRaRre and it shows a blue background when hovered, http://screencast.com/t/P7AxkJiyS

::: browser/locales/en-US/chrome/browser/browser.dtd:6
(Diff revision 8)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- LOCALIZATION NOTE : FILE This file contains the browser main menu items -->
> -<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys --> 
> +<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys -->

Although it's not bad to remove the trailing whitespace, it should be done in a standalone bug.

Combining it with this work will make it confusing if someone finds something broken and it points to your patch, or if someone wonders why a patch about session restore just modified the localization line for the tools menu.

::: browser/themes/shared/tabs.inc.css:584
(Diff revision 8)
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;
> +  position:fixed;
> +  padding: 4px;
> +  margin-left: 3px;

margin-inline-start instead of margin-left

::: browser/themes/shared/tabs.inc.css:585
(Diff revision 8)
> +  transition: opacity 300ms ease-out;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;
> +  margin-bottom: 0;
> +  padding: 0 5px;
> +}
> +
> +.restore-tabs-button {
> +  border: 1px solid var(--restore-tabs-button-color);
> +  border-radius: 2px;
> +}
> +
> +.restore-tabs-button-wrapper.restore-fade-out {
> +  opacity: 0;

I think we should remove the fade-out. It doesn't accomplish much and might make the user interface feel slow. Opening a tab is already not super smooth, and introducing this extra animation will increase the "jank"-factor.

Let's just move forward with immediately hiding the button in all cases.
Attachment #8764330 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
I'm picking up where Erica left off! I added a declaration for a background-color on Linux because I found that I couldn't see the button against the dark background.

Comment 44

7 months ago
mozreview-review
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review70894

I've not really checked the styling here, because there's enough tweaking to do with the behaviour. We can look at the styling nearer the end (or perhaps Jared has comments on that).

::: browser/base/content/browser.css:142
(Diff revision 1)
> +/*#TabsToolbar[currentset*="tabbrowser-tabs,alltabs-button,new-tab-button"] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> +#TabsToolbar[currentset*="tabbrowser-tabs,new-tab-button,alltabs-button"] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button {
> +  visibility: visible;
> +}*/

Did you mean to have this commented out? Or should this hunk just be removed?

::: browser/base/content/browser.js:7571
(Diff revision 1)
>  
>  var RestoreLastSessionObserver = {
>    init: function () {
>      if (SessionStore.canRestoreLastSession &&
>          !PrivateBrowsingUtils.isWindowPrivate(window)) {
> +      let restoreTabsButtonWrapper = document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button-wrapper");

I'd add a getter for this, because right now we're repeating it 3 times:

```
get restoreTabsButtonWrapper() {
  delete this.restoreTabsButtonWrapper;
  return this.restoreTabsButtonWrapper =
    document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid",
                                            "restore-tabs-button-wrapper");
}
```


In fact, reading further down the patch, it probably makes more sense to add a `<field>` for this on the relevant bit of the tabcontainer binding. Then you can just fetch it once and do:

`let {restoreTabsButtonWrapper} = gBrowser.tabContainer;`

to fetch it in here, and you can simplify the code in the tabbrowser that also currently uses the anonid fetching.

::: browser/base/content/browser.js:7591
(Diff revision 1)
> +    restoreTabsButtonWrapper.removeEventListener("transitionend", this.removeRestoreTabsButton, false);
> +    restoreTabsButtonWrapper.hidden = true;
> +  },
> +  fadeRestoreTabsButton: function () {
> +    let restoreTabsButtonWrapper = document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button-wrapper");
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.fadeRestoreTabsButton, false);

This unfortunately won't work, because you're adding the listener with a bound reference rather than the original function.

The easiest way to fix this is to just use a handleEvent method on this object to deal with the two event listeners. Then you can just pass `this` as the listener argument to add/removeEventListener. Inside handleEvent, `this` will be correct (so you won't need to `bind` anything). You'll want code that checks what event we're getting and calls the `fadeRestoreTabsButton` or `removeRestoreTabsButton` as appropriate.  See e.g. https://dxr.mozilla.org/mozilla-central/rev/f97a056ae6235de7855fd8aaa04fb1c8d183bd06/browser/components/customizableui/content/panelUI.js#181 as an example.

Though... actually, AFAICT you've kept the transitionend listener but there's no longer any transition or "fade out"? You should probably get rid of that level of indirection completely, and as soon as the new tab is opened just call `restoreTabsButtonWrapper.hidden = true` (or even just `restoreTabsButtonWrapper.remove()` !). That will let you simplify this code a lot, and remove the fadeout/transition references.

::: browser/base/content/browser.js:7600
(Diff revision 1)
>    observe: function () {
>      // The last session can only be restored once so there's
>      // no way we need to re-enable our menu item.
>      Services.obs.removeObserver(this, "sessionstore-last-session-cleared");
>      goSetCommandEnabled("Browser:RestoreLastSession", false);
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.fadeRestoreTabsButton, false);

You also do this inside `fadeRestoreTabsButton`, so doing it here is probably not necessary?

::: browser/base/content/tabbrowser.xml:4955
(Diff revision 1)
> +        <xul:hbox class="restore-tabs-button-wrapper"
> +                  anonid="restore-tabs-button-wrapper">
> +          <xul:toolbarbutton anonid="restore-tabs-button"
> +                             class="restore-tabs-button"
> +                             onclick="SessionStore.restoreLastSession();"
> +                             label="&restoreLastTabs.label;"/>

The last code I saw that needed to do something like this set the attribute at runtime (from the binding's constructor) based on an attribute (like "restorelasttabslabel") set on the element that we bind here. Then we don't need to include an extra DTD and the browser.xul markup can include the value that we then set in the binding.

::: browser/base/content/tabbrowser.xml:5010
(Diff revision 1)
> +      <field name="buttonWidth">
> +        document.getAnonymousElementByAttribute(this, "anonid", "restore-tabs-button-wrapper").clientWidth;
> +      </field>

This will only be evaluated once. Is that actually what you want? Shouldn't this be a property with a getter?

Also, we should use a more specific name than just "buttonWidth". Maybe "restoreTabsButtonWidth" ?

::: browser/base/content/tabbrowser.xml:5027
(Diff revision 1)
> +      <method name="updateSessionRestoreVisibility">
> +        <body><![CDATA[
> +          let shouldHide = true;
> +          let buttonPadding = 10;
> +
> +          //measure the button width initially on the DOM - it changes due to localization

Super-picky nit: general convention is leaving a space after "//" and using sentence case + punctuation, so please add a space, s/measure/Measure/, and add a period/dot at the end of the sentence. Same comment on some of the other comments.

::: browser/base/content/tabbrowser.xml:5033
(Diff revision 1)
> +          let buttonWidth = this.buttonWidth;
> +          let wrapper = document.getAnonymousElementByAttribute(this, "anonid", "restore-tabs-button-wrapper");
> +
> +          // if there is a flex > 1 element in the TabToolbar then there will not be a visual
> +          // jump when the button fades, so we can show it so long as there is some room.
> +          if (!wrapper.classList.contains("restore-fade-out") && wrapper.getAttribute('session-exists') === "true") {

We don't normally use strict-equals, and the session-exists attribute just won't be there if there isn't a session, so this can be simplified to:

if (!wrapper.classList.contains("restore-fade-out") && wrapper.hasAttribute("session-exists"))

(Also: please use double quotes rather than single quotes.)

::: browser/base/content/tabbrowser.xml:5034
(Diff revision 1)
> +            let toolbarNode = document.getElementById("TabsToolbar").firstChild;
> +            while (toolbarNode && toolbarNode.nodeType === 1) {
> +              if (toolbarNode.flex > 1) {
> +                if ((wrapper.hidden === true && toolbarNode.boxObject.width >= (buttonWidth * 2.5)) || (wrapper.hidden !== true && toolbarNode.boxObject.width >= buttonWidth * 1.5)) {
> +                  shouldHide = false;
> +                }
> +              }
> +              toolbarNode = toolbarNode.nextElementSibling;
> +            }

If we keep this, rather than the manual loop and having to worry about element types, I would do something like this:

```
let flexNodes = document.getElementById("TabsToolbar").querySelector("[flex]");
for (let flexNode of flexNodes) {
  if (flexNode.flex > 1 &&
      /* Long condition here */) {
  }
}
```

There are a couple of other notes here to your existing code:

First, where do the magic `buttonWidth * 1.5` and `buttonWidth * 2.5` values come from? Did you just test with the search box and determine that these values were about right? Or something else? The values (1.5/2.5) are likely to be different for different things with flex>1, depending on their minimum intrinsic width etc.  There is no really sane way of determining those minimum widths from JS.

**IMO we should not care about the 'search bar in the tabstrip' case. If the user has customized their browser like this, they likely don't need telling about the possibilities of session restore. If other people think this is important, we can delay dealing with this to a followup bug - right now, iterating over this is taking a lot of time when it's comparatively not very important in the "grand scheme" of the feature itself (ie very few people will hit this codepath anyway).**

Second, comparing to `true` and `false` literals in almost any language is a "code smell". It is effectively almost always unnecessary. It is less code, and easier to read to do

```
if (something)
```
than
```
if (something === true)
```
and likewise
```
if (!something)
```
rather than
```
if (something === false)
```

While the meanings of strict-comparison to booleans and just the condition are subtly different in JS, the difference rarely matters if your variables are sane (that is, something should either be a boolean or a string or a number or an object/null, not a mixture where the type of thing it is matters over the value of the thing and you have to be "on your guard" all the time). In this particular case, we can be pretty sure that element.hidden is a "real" boolean, so the strict check is unnecessary.

::: browser/base/content/tabbrowser.xml:5051
(Diff revision 1)
> +            let newTabButton = document.getAnonymousElementByAttribute(this, "anonid", "tabs-newtab-button");
> +            let childNodesTotalWidth = firstTab.clientWidth + newTabButton.clientWidth + wrapper.clientWidth;
> +
> +            //if there is enough extra room in the flex-element "tabbrowser-tabs"
> +            //then there is room for the button.
> +            if ((wrapper.hidden && document.getElementById("tabbrowser-tabs").clientWidth - childNodesTotalWidth >= buttonWidth + buttonPadding) || (!wrapper.hidden === true && document.getElementById("tabbrowser-tabs").clientWidth - childNodesTotalWidth >= buttonPadding)) {

This would love to be split over several lines so it's easier to read. In particular, I suspect it'll help if you add a temp for the width of the tabstrip. Or actually, can we just use `this.(tabContainer.)mTabstripWidth` instead? It's also a little odd that the binding here relies on the document ID of the element which we're binding (ie tabbrowser-tabs). You should be able to use `this` or `this.tabContainer` or something like this, if `mTabstripWidth` doesn't work out.

Otherwise, it looks like the code currently does approximately this:

if (no wrapper and tabstrip width - (tab + newtab + wrapper width) >= wrapper width + padding)
OR
if (have wrapper and tabstrip width - (tab + newtab + wrapper width) >= button padding)

I'm struggling to follow this. I'm guessing wrapper width (as part of `childNodesTotalWidth`) is 0 when the button is hidden. So then we check whether effectively, we have room for the width + padding of the button in the "spare" space we have.

When we've already shown the button, we check if the total width of the wrapper (which includes the button and its padding) and the new tab button and the single tab is greater than or equal to... the padding on the button? Why do we need to do that? Is this just to check that we're not making the single tab smaller? In that case, I expect the comparison should simply be to check against > 0  (ie there should be at least some space remaining). Or does that not work for some reason?

Finally, I think this code is harder to read because both comparisons include the width of the child nodes, but in one case that width includes the width of the wrapper and in one case it does not, plus we have two ways of measuring the width of the wrapper. It would help understandability of the code if the "meaning" of `childNodesTotalWidth` didn't change, and if we used just one way of measuring the width of the wrapper. Is that possible or is there another subtlety I'm missing?


Also note the creeping in of another:

`!foo === true`

(which is even semantically exactly the same as just `!foo` because the `!` is coercing the value to a boolean anyway, and so the comparison to `true` is redundant.)

::: browser/themes/linux/browser.css:1574
(Diff revision 1)
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: -moz-Dialog;
> +}

What's the foreground color here? We should make sure it is `-moz-dialogtext`, or we should make the background color you're declaring here match whatever the foreground color is. Have you checked if we need special styling on any other OS? It might need `color: inherit` somewhere (and in fact, maybe that would help on Linux?), but I've not checked.

::: browser/themes/shared/tabs.inc.css:11
(Diff revision 1)
>  
>  :root {
>    --tab-toolbar-navbar-overlap: 1px;
>    --navbar-tab-toolbar-highlight-overlap: 1px;
>    --tab-min-height: 31px;
> +  --restore-tabs-button-color: hsla(0,0%,16%,.2);

This should probably be called restore-tabs-button-border-color , as it only controls the border and not the foreground color.

::: browser/themes/shared/tabs.inc.css:14
(Diff revision 1)
>    --navbar-tab-toolbar-highlight-overlap: 1px;
>    --tab-min-height: 31px;
> +  --restore-tabs-button-color: hsla(0,0%,16%,.2);
>  }
> +
> +:root[lwthemetextcolor="dark"] {

Nit: you can use the :-moz-lwtheme-brighttext / :-moz-lwtheme-darktext pseudoselectors instead.
Attachment #8782591 - Flags: review?(gijskruitbosch+bugs) → review-
Attachment #8764330 - Attachment is obsolete: true

Comment 45

7 months ago
mozreview-review
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review71134

Clearing review, I'll wait until a new patch is attached with Gijs' feedback addressed.

::: browser/base/content/tabbrowser.xml:4950
(Diff revision 1)
> +        <xul:hbox class="restore-tabs-button-wrapper"
> +                  anonid="restore-tabs-button-wrapper">
> +          <xul:toolbarbutton anonid="restore-tabs-button"

Can you test to see what happens when Customize Mode is entered and this button is visible?

We should hide the button in customize mode.
Attachment #8782591 - Flags: review?(jaws)

Updated

7 months ago
Assignee: ewright → kbroida
I'd like to test how release users perceive the session restore button implementation and whether or not it meets their needs before shipping in the core product.  I'll define what we want to test and post it here for input/discussion.
Comment hidden (mozreview-request)
Thanks for the feedback, Gijs! Coming in as the second person on this patch, I've realized I was a little lost as well in terms of where the button width numbers were coming from. I believe a lot of the complexity had to do with making sure there was enough space for the new tab button to show and also to allow for the fade away animation. I've added a simpler one (as Jared suggested) that transitions the button's max-width to zero to hopefully avoid jank. I also simplified some of the tabs math to reflect the way I've understood this patch's goals. Let me know if I've missed something/it's not what we're going for. 

One issue I haven't figure out yet is how to make sure the restore tabs button appears when running tests. Currently some of my tests are failing and I believe it's because the restore tabs button isn't appearing reliably (when I do the same tests manually I get the results I expect). To get the button to appear, I mimicked how I've been testing it manually by opening a window, closing it, then re-opening it again. Let me know if there's a better way to go about this.

Comment 49

7 months ago
mozreview-review-reply
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review71134

> Can you test to see what happens when Customize Mode is entered and this button is visible?
> 
> We should hide the button in customize mode.

When in Customize Mode, I found that the restore tabs button was already hidden even without specifying it should be hidden in updateSessionRestoreVisibility().
(In reply to Katie Broida [:ktbee] from comment #48)
> One issue I haven't figure out yet is how to make sure the restore tabs
> button appears when running tests. Currently some of my tests are failing
> and I believe it's because the restore tabs button isn't appearing reliably
> (when I do the same tests manually I get the results I expect). To get the
> button to appear, I mimicked how I've been testing it manually by opening a
> window, closing it, then re-opening it again. Let me know if there's a
> better way to go about this.

Just a hunch, but you might need to wait for session store to be saved. Walking through the steps manually would give enough time for state to get saved but through automation it might be moving too fast.

Comment 51

7 months ago
mozreview-review
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review74350

Closer! (and simpler!)

But still a few issues. I didn't look at the test yet - I hope Jared's feedback will help you get that going. If not, please do feel free to ping me or Jared on IRC or via email or on this bug.

::: browser/base/content/browser.js:7652
(Diff revision 2)
>      if (SessionStore.canRestoreLastSession &&
>          !PrivateBrowsingUtils.isWindowPrivate(window)) {
> +      let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +      restoreTabsButtonWrapper.setAttribute("session-exists", "true");
> +      // After restoreTabsButtonWrapper's width has been measured in tabbrowser.xml,
> +      // we need remove the styling in tabs.inc.css that hides it.

Nit: "override" rather than "remove".

::: browser/base/content/browser.js:7661
(Diff revision 2)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -
> +  removeRestoreButton: function () {
> +    let {restoreTabsButtonWrapper} = gBrowser.tabContainer;

Can you add a comment here saying the method isn't bound to |this| because it's invoked from an event handler? Or use the handleEvent() stuff I mentioned in the last review?

::: browser/base/content/browser.js:7671
(Diff revision 2)
> +  },
>    observe: function () {
>      // The last session can only be restored once so there's
>      // no way we need to re-enable our menu item.
>      Services.obs.removeObserver(this, "sessionstore-last-session-cleared");
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.removeRestoreButton);

I think I suggested doing the removing should happen inside the removeRestoreButton code, because now the listener will not be removed if a tab is opened. Because `this` will be wrong inside the listener right now, I guess that's a little tricky if you're not refactoring to use the handleEvent things... but we do need to make sure the listener gets removed. :-)

::: browser/base/content/tabbrowser.xml:5013
(Diff revision 2)
> +          let restoreTabsButton = document.getAnonymousElementByAttribute(
> +            gBrowser.tabContainer, "anonid", "restore-tabs-button");

I think this can just be restoreTabsButtonWrapper.firstChild. :-)

::: browser/base/content/tabbrowser.xml:5025
(Diff revision 2)
> +          // Get the widths of the elements in the tabs area
> +          let restoreTabsButtonWidth = restoreTabsButtonWrapper.clientWidth;
> +          let tabs = document.getElementById("tabbrowser-tabs");
> +          let tabsChildNodes = Array.from(tabs.children);
> +          let tabsUsedSpace = 0;
> +          tabsChildNodes.forEach(node => tabsUsedSpace += node.clientWidth);

There are issues here where the tab resizes to become smaller, and if you resize the window slowly, you can see flickering with the tabstrip intermittently going to overflow mode.

I'm not sure this is sufficient to fix all of these problems, but a way to get this code to provide a more consistent result is to reuse the `_tabDefaultMaxWidth` value from this binding, which you can then just multiply with the number of tabs we have, which avoids the loop an dreading all the tabs' client width.

You'd have to then check manually if you can reproduce the flickering still...

::: browser/themes/linux/browser.css:1605
(Diff revision 2)
> +  background-color: -moz-Dialog;
> +  color: -moz-dialogText;

These don't seem like the colors we typically use for a button. I think we probably use ThreeDFace and ButtonText, if anything. Does that work or does it look really ugly or something?

Alternatively, maybe we can make it use the same background + foreground colors as the tabs, without making it connect to the navbar? That feels like it would work relatively well.

::: browser/themes/shared/tabs.inc.css:547
(Diff revision 2)
> +  margin-inline-start: 3px;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;

The text looks very small to me on Windows, with antialiasing artifacts as well. Why do we need to give this item a custom text size? We don't normally do that, and it feels like the font and text size we use for a tab would be fine to use here.

::: browser/themes/shared/tabs.inc.css:554
(Diff revision 2)
> +  /* The button will likely never reach this width, but it is specified here for the transition */
> +  max-width: 400px;

While I admire your optimism, I worry that on locales with long text (German, Russian, Ukrainian) we will end up breaking this limit, if not with the default font size then definitely with some other windows font sizes. Using `em` for the units would alleviate some of that problem, I suspect.

::: browser/themes/shared/tabs.inc.css:570
(Diff revision 2)
> +  border-color: hsl(0, 0%, 70%);
> +}
> +
> +.restore-tabs-button[animate=true] {
> +  max-width: 0;
> +  transition: max-width 300ms;

I noticed the juggling of the attribute and that looks a bit odd. Can't we just set transition: on the button at all times and do away with the extra attribute? Does that not work?
Attachment #8782591 - Flags: review?(gijskruitbosch+bugs)
Before landing, can we have a feature flag (or equivalent) so that this can be tested before letting it ride the trains?  I'm drafting a test plan here: https://docs.google.com/document/d/1KLVhbeovQAc-B-M95jx5UdTXRzte3WvGsK6Le0UqrF0/edit#heading=h.23kbtdsiyvwa
Gijs pushed the patch on this bug to tryserver so we could get some builds available for user testing. The trypush is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf809af9dd7

Click on the green (B) next to a platform then click on Job Details to find a link to all of the uploaded files for that specific build.

Peter and Philipp, flagging you so you both see this. Let Gijs or I know if you need anything else.
Flags: needinfo?(philipp)
Flags: needinfo?(pdolanjski)
Thanks Jared.  Philipp, I'll let you give it a try to see if it is sufficient for usertesting.com.
Flags: needinfo?(pdolanjski)
Yep, the feature works as expected! I do have a few comments on styling, but that's not relevant for testing.

There's on issue though: could we sign the build?
We've seen in previous tests that these prompts (https://cl.ly/0c0T222a2A1H) are often big barriers for users and might make them drop out of tests.
Flags: needinfo?(philipp)
Comment hidden (mozreview-request)
(In reply to :Gijs Kruitbosch from comment #51)
> Comment on attachment 8782591 [details]
> Bug 1219725 - Add a button for session restore to the tab bar.

> I didn't look at the test yet - I hope Jared's
> feedback will help you get that going. If not, please do feel free to ping
> me or Jared on IRC or via email or on this bug.

Thanks for taking a look at this Gijs! I haven't finished writing tests yet, but I thought it would be good to get some feedback on your other points in the meantime. 

> There are issues here where the tab resizes to become smaller, and if you
> resize the window slowly, you can see flickering with the tabstrip
> intermittently going to overflow mode.
> 
> I'm not sure this is sufficient to fix all of these problems, but a way to
> get this code to provide a more consistent result is to reuse the
> `_tabDefaultMaxWidth` value from this binding, which you can then just
> multiply with the number of tabs we have, which avoids the loop an dreading
> all the tabs' client width.
> 
> You'd have to then check manually if you can reproduce the flickering
> still...

I saw the flickering effect you mentioned, and found that it was because the restoreTabsButton's width was toggling between 0 and its width when it wasn't hidden. Because of this changing width, the if statement comparing `(tabs.clientWidth - tabsUsedSpace) > restoreTabsButtonWidth` sometimes evaluated to `true` when it shouldn't have. The solution I found for this was to just save the button's initial width in a field in tabbrowser.xml so that it wouldn't change as the browser resizes. 

> ::: browser/themes/linux/browser.css:1605
> (Diff revision 2)
> > +  background-color: -moz-Dialog;
> > +  color: -moz-dialogText;
> 
> These don't seem like the colors we typically use for a button. I think we
> probably use ThreeDFace and ButtonText, if anything. Does that work or does
> it look really ugly or something?

There was no aesthetic reason I chose those colors, I had just thought those were the defaults. These color suggestions work also. 

> ::: browser/themes/shared/tabs.inc.css:570
> (Diff revision 2)
> > +  border-color: hsl(0, 0%, 70%);
> > +}
> > +
> > +.restore-tabs-button[animate=true] {
> > +  max-width: 0;
> > +  transition: max-width 300ms;
> 
> I noticed the juggling of the attribute and that looks a bit odd. Can't we
> just set transition: on the button at all times and do away with the extra
> attribute? Does that not work?

I'm not sure what you mean by setting transition on the button at all times. Doesn't it need a change in state to trigger the transition? Toggling the animate attribute was my attempt at triggering the transition using JavaScript. 

More soon on the tests ...

Comment 58

6 months ago
mozreview-review
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review83762

Closer! We should work on the tests and the finer points of the transition, but I think we're almost there.

We should probably put this behind a feature pref. To do this, you would add a pref("browser.tabs.restorebutton", true); line to browser/app/profile/firefox.js, and you'd check the browser.tabs.restorebutton pref in the updateSessionRestoreButton() method (and not show the button if the pref is set to false). Then we can turn this on/off for experiments and on beta/release/whatever if we decide to ship it or not ship it or just turn session restore on for everybody or something.

::: browser/base/content/browser.js:7780
(Diff revision 3)
> +      gBrowser.tabContainer.addEventListener("TabOpen", this, false);
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> +  handleEvent: function(aEvent) {

Nit: empty lines between the methods, please.

::: browser/base/content/browser.js:7784
(Diff revision 3)
>    },
> +  handleEvent: function(aEvent) {
> +    switch (aEvent.type) {
> +      case "TabOpen":
> +        this.removeRestoreButton();
>  

Nit: and you can remove this empty line.

::: browser/base/content/browser.js:7792
(Diff revision 3)
> +  removeRestoreButton: function () {
> +    let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +    let restoreTabsButton = restoreTabsButtonWrapper.firstChild;
> +    restoreTabsButton.setAttribute("animate", true);
> +    window.setTimeout(function(){restoreTabsButtonWrapper.hidden = true;
> +                                 restoreTabsButton.removeAttribute("animate");}, 290);

It probably isn't necessary to remove the attribute (or the style property when that's set).

Nit: spaces around the { brace and before the } brace, please. This could be shortened to:

window.setTimeout(() => restoreTabsButtonWrapper.hidden = true, 290);

Speaking of 290... what's that magical value? :-)
It looks like the animation is 300ms, so shouldn't this just be 300ms?

Oh... I think our feedback has been confusing here, sorry. If we're keeping a transition, we should continue to use a transitionend listener to remove the item when the transition finishes.

::: browser/base/content/tabbrowser.xml:5235
(Diff revision 3)
> +          let tabsUsedSpace = 0;
> +          tabsChildNodes.forEach(node => tabsUsedSpace += node.clientWidth);

Nit: let tabsUsedSpace = tabsChildNodes.reduce((sum, node) => sum + node.clientWidth, 0);

Note that I still think using `_tabDefaultMaxWidth` and multiplying with the number of tabs would be a good idea.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:2
(Diff revision 3)
> +// Ensure new tab button is visible after resizing window
> +add_task(function* (){

None of the tests here check that the tab restore button is actually visible, or that clicking it works. Can we simulate the case where it appears? This might require mocking some session restore stuff. Maybe Mike de Boer can help - you could needinfo him and ask?

This first task seems to not do very much - it resizes, and then it checks that the resize works, but crucially it doesn't check anything after that.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:11
(Diff revision 3)
> +  win = yield BrowserTestUtils.openNewBrowserWindow();
> +
> +  let tabBrowser = win.document.getElementById('tabbrowser-tabs');
> +  let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +  let newTabButton = win.document.getAnonymousElementByAttribute(win.gBrowser.tabContainer, 'class', 'tabs-newtab-button');
> +  is(newTabButton.style.visibility != 'collapse', true, 'new tab button is not visibility: collapse');

Nit: isnot(newTabButton.style.visibility, "collapse", "new tab button is not visibility: collapse");

::: browser/base/moz.build:25
(Diff revision 3)
>      'content/test/newtab/browser.ini',
>      'content/test/plugins/browser.ini',
>      'content/test/popupNotifications/browser.ini',
>      'content/test/referrer/browser.ini',
>      'content/test/social/browser.ini',
> +    'content/test/tabbrowser/browser.ini',

Since we started, the test dir now has a tabs/ subdir. Can we reuse that instead for the test?

::: browser/themes/linux/browser.css:1364
(Diff revision 3)
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: ThreeDFace;
> +  color: ButtonText;
> +}

We'll need similar styling on OS X and Windows, I think.

::: browser/themes/shared/tabs.inc.css:548
(Diff revision 3)
> +  display: none;
> +}
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;
> +  position:fixed;

Can you add a comment about why we're using position: fixed in the CSS here?

::: browser/themes/shared/tabs.inc.css:576
(Diff revision 3)
> +.restore-tabs-button:-moz-lwtheme-brighttext {
> +  border-color: hsl(0, 0%, 70%);
> +}
> +
> +.restore-tabs-button[animate=true] {
> +  max-width: 0;

So my point was that you can set the transition: CSS property in the .restore-tabs-button selector, and instead of setting this "animate" attribute, you could use element.style.maxWidth = 0 instead. Then we don't need this rule.
Attachment #8782591 - Flags: review?(gijskruitbosch+bugs)
Hey Katie, are you still working on this bug?  I've been talking with some people about running some tests with it, and wondered if I could help get the patch finished and landed…  Thanks!
Flags: needinfo?(kbroida)

Comment 60

5 months ago
(In reply to Blake Winton (:bwinton) (:☕️) from comment #59)
> Hey Katie, are you still working on this bug?  I've been talking with some
> people about running some tests with it, and wondered if I could help get
> the patch finished and landed…  Thanks!

She is, she's looking at automated tests. She's been in touch with Jared, Mike de Boer and me via e-mail.

From the comments from Philipp et al, I thought that the patch as-is could already be tested? I would suggest that further conversation about doing a test run of this feature might be best over email / IRC rather than in the bug where the feature is being implemented.

Comment 61

5 months ago
Per discussion with Jared, it feels like it might make sense to pre-land the strings so this can make the 52 train even if the tests take a little longer to sort out.

Katie, could you create a patch file with just the tabbrowser.properties change? Then Jared or I can rubberstamp that and we can land it in 52. When the patch is done, we can ask for uplift to aurora if we don't make the merge day cutoff (next Monday).
Sure thing, Gijs. I'll make those changes now and continue working on the tests after submitting the patch you need for 52.
Flags: needinfo?(kbroida)
Comment hidden (mozreview-request)

Updated

5 months ago
Attachment #8782591 - Flags: review?(philipp)

Comment 64

5 months ago
Philipp can review the label, I think. :-)

Updated

5 months ago
Attachment #8782591 - Flags: review?(jaws)
Attachment #8782591 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Looks good!
Attachment #8782591 - Flags: review?(philipp) → review+

Comment 66

5 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a3f82cfc81
This patch adds a label for the forthcoming Restore Tabs Button. r=phlsa,gijs

Comment 67

5 months ago
Pushed to inbound to avoid issues with reusing the reviewboard thingy for autoland.
Keywords: leave-open

Comment 68

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6a3f82cfc81
Hi everyone, I wanted to let you know that I'll be starting a new job soon and won't be able to keep up with this bug. I don't want to hold the process up, so I'll go ahead an un-assign myself now :) Thank you for all your guidance on it!
Assignee: kbroida → nobody
Blocks: 1330638
You need to log in before you can comment on or make changes to this bug.