996230 - Implement new structure for History panel

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: History.jpg

Implement new History structure in Bug 989108

Still need an icon for restore closed tabs/windows, but we can use the reload icon as placeholder for now.

Comment 1

[Zhenshuo Fang (:fang) - Firefox UX Team]

oh we should also considering decrease the number of history items we show in the subview panel, it's currently 15 items and very long.

Comment 2

[Matthew N. [:MattN]]

Zhenshuo, could you explain what problem these changes are trying to solve and the behaviours of the title and action components?

Comment 3

[Matthew N. [:MattN]]

I also think we should have icons ready before the bug is assigned to engineering.

Comment 4

[Zhenshuo Fang (:fang) - Firefox UX Team]

The current "restore closed tabs" & "restore closed windows" in history subview can be confusing. It's not cleat that they are titles, which means the items below them are recently closed tabs & windows; It's also not clear that they are actions, which means clicking them will restore all recently closed tabs or windows. This design is simply to separate the title & action. Clicking on the action next to the recently closed tab will restore all recently closed tabs.

Comment 5

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: restore icon.png

And for the icon, we can use the "restore" icon in new tab page.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Work in progress. Basically everything working:
* Re-ordered sections
* Reduced number of history items from 15 to 10
* Split "Restored Closed X" items into non-interactive sections headings and a separate button
* Added tooltip text

Still to do:
* Tests

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - Windows 8

Screenshot of this in Windows 8. The Restore Closed Tabs button has the mouse cursor over it.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - OS X

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - Linux

Comment 10

[Zhenshuo Fang (:fang) - Firefox UX Team]

Niice.Thanks for the update Blair!
Michael, what do you think of the restore icon? it's the same one with "restore previous session" on homepage. But right now it seems to draw too much attention.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8433213 [details] [diff] [review]
WiP v1

I need to hunt down some test issues caused by bug 1021606 before I know the tests for this are working right. But this part of the patch hasn't changed - so while I finish shaving the afore mentioned yak, could I get a prelim review on this?

Comment 12

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Sub-menuPanel-spec.png

Sub-menu Panel CSS

<!hover and active states-->

.sub-menu:hover {
  border-radius: 2px;
  background: #EBEBEB;
  border: 1px solid #C1C1C1;
}
.sub-menu-footer:hover {
  background: #EBEBEB;
  border-top-style: 1px solid #C1C1C1;
}
.sub-menu:active {
  border-radius: 2px;
  background: #DADADA;
  border: 1px solid #C1C1C1;
}
sub-menu-reload:hover {
  background: #EBEBEB;
  border-left-style: 1px solid #C1C1C1;
  border-top-style: 1px solid #C1C1C1;
  border-bottom-style: 1px solid #C1C1C1;
  width: 32px;
  height: 24px;
}
.sub-menu-reload:active {
  background: #DADADA;
  border-left-style: 1px solid #C1C1C1;
  border-top-style: 1px solid #C1C1C1;
  border-bottom-style: 1px solid #C1C1C1;
  width: 32px;
  height: 24px;
}

<!Windows typography-->

h1.windows {
  font-family: SegoeUI-Bold;
  font-size: 12px;
  text-transform: uppercase;
  color: #333333;
  letter-spacing: 1px;
}

p.windows {
  font-family: SegoeUI;
  font-size: 14px;
  color: #333333;
  line-height: 24px;
}

<!OSX typography-->

h1.osx {
  font-family: LucidaGrande-Bold;
  font-size: 12px;
  text-transform: uppercase;
  color: #333333;
}

p.osx {
 font-family: LucidaGrande;
 font-size: 13px;
 color: #333333;
 line-height: 24px;
}

<!Linux typography-->

h1.linux {
  font-family: Ubuntu-Bold;
  font-size: 12px;
  text-transform: uppercase;
  color: #333333;
}

p.linux {
  font-family: Ubuntu;
  font-size: 14px;
  color: #333333;
  line-height: 24px;
}

Comment 13

[Michael Maslaney [:mmaslaney] ([e-mail])]

Please use the reload button from the toolbar.png file.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8433224 [details]
Screenshot - Windows 8

I'll take comment 12 as a r- from UX then.

Comment 15

[Tim Nguyen :ntim]

(In reply to Blair McBride [:Unfocused] from comment #14)
> Comment on attachment 8433224 [details]
> Screenshot - Windows 8
> 
> I'll take comment 12 as a r- from UX then.

This seems good for now. I think that if we're changing colors, let's do this in another bug with all colors changed.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

The test here requires bug 1021606. And ironically, that patch for that is currently breaking other tests...

Comment 17

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

Comment on attachment 8440575 [details] [diff] [review]
Patch v1

Review of attachment 8440575 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser.ini
@@ +109,5 @@
>  [browser_992747_toggle_noncustomizable_toolbar.js]
>  [browser_993322_widget_notoolbar.js]
>  [browser_995164_registerArea_during_customize_mode.js]
> +[browser_996230_historyView_restore_all.js]
> +skip-if = e10s # Bug ?????? - test uses promiseTabLoadEvent() which isn't e10s friendly.

Should get a bug on file for fixing this.

::: browser/components/customizableui/test/browser_996230_historyView_restore_all.js
@@ +16,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function promiseWindowClosed() {

In bug 1021606 a `waitForEvent` function was added to head.js. Can you add a similar `waitForObserver` generic event which can then be used for "domwindowopened" and "domwindowclosed"?

@@ +32,5 @@
> +add_task(function* () {
> +  // Ensure only our own closed tab is stored
> +  Services.prefs.setIntPref("browser.sessionstore.max_tabs_undo", 1);
> +
> +	yield promiseTabLoadEvent(gBrowser.tabs[0], TEST_PAGE);

please change these tabs to spaces, here and throughout this file.

@@ +80,5 @@
> +     "Restored tab should have expected URI");
> +
> +  gBrowser.removeTab(tab, {animate: false});
> +  gBrowser.addTab("about:blank", {skipAnimation: true});
> +  gBrowser.removeTab(gBrowser.tabs[0], {animate: false});

we should get a bug on file (probably good-first-bug) to unify these arguments to just 'animate' and get rid of 'skipAnimation'. the only problem with doing so is addon-compat, but we could also continue to support the old argument name during some deprecation period.

::: browser/components/sessionstore/src/RecentlyClosedTabsAndWindowsMenuUtils.jsm
@@ +36,5 @@
>    * @returns A document fragment with UI items for each recently closed tab.
>    */
>    getTabsFragment: function(aWindow, aTagName, aPrefixRestoreAll=false,
> +                            aRestoreAllLabel="menuRestoreAllTabs.label",
> +                            aRestoreAllTooltip=null) {

nit, we shouldn't need to specify a default of 'null' for this. 'undefined' will work just as well.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +788,5 @@
> +
> +/* Not using a <separator> element here, because we want this line to hide only
> +   when hovering over the inner button. If this were a <separator>, we couldn't
> +   query that. */
> +.subviewheaderbutton-btn:not(:hover)::before {

It's ok, on a sidenote I really love this approach because it allows us to use things like background-image:linear-gradient(), etc.

@@ +805,5 @@
> +.subviewheaderbutton-btn > .toolbarbutton-icon {
> +  -moz-margin-end: 0;
> +  width: 14px;
> +  height: 14px;
> +  margin: 4px 2px;

At the top of this, -moz-margin-end is set to 0, but right here that is changed to 2px. It seems the first line can be removed.

@@ +819,5 @@
> +  list-style-image: url("chrome://browser/skin/restoreAll.png");
> +}
> +
> +#PanelUI-recentlyClosedWindows > .restoreallitem {
> +  list-style-image: url("chrome://browser/skin/restoreAll.png");

Please merge this with the `#PanelUI-recentlyCloseTabs > .restoreallitem` rule above.

@@ +828,5 @@
> +    list-style-image: url("chrome://browser/skin/[e-mail]");
> +  }
> +
> +  #PanelUI-recentlyClosedWindows > .restoreallitem {
> +    list-style-image: url("chrome://browser/skin/[e-mail]");

Ditto.

Comment 18

[Marco Mucci [:MarcoM]]

Hi Blair, should we remove Bug 996230 from this iteration until Bug 1021606 is resolved?

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Mucci [:MarcoM] from comment #18)
> Hi Blair, should we remove Bug 996230 from this iteration until Bug 1021606
> is resolved?

Hmm, yes, thanks.

Comment 20

[Vova Olar]

Please make middle click respect loadBookmarksInBackground preference

Comment 21

[Tim Nguyen :ntim]

Pretty sure photon solves this.