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.

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.

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

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

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.

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

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

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

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 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?

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; }

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

Comment on attachment 8433224 [details] Screenshot - Windows 8 I'll take comment 12 as a r- from UX then.

(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.

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

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/restoreAll@2x.png"); > + } > + > + #PanelUI-recentlyClosedWindows > .restoreallitem { > + list-style-image: url("chrome://browser/skin/restoreAll@2x.png"); Ditto.

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

(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.

Please make middle click respect loadBookmarksInBackground preference

Pretty sure photon solves this.