The grid reset button should moved from the main UI to the preferences dialog.

How is this planned at the UI level? My suggestion is to show a list of sites removed by the user, in the Tabs panel within Options. A user can then remove one or more sites from this list to get it back on the new tab page. This would also allow selective restore.

I'd rather implement bug 722234 and not this one but let's get some UX feedback.

Comment on attachment 604224 [details] [diff] [review] Part 1 - Add restore button to the preference dialog Included in tomorrow's UX nightly.

I've previously hit the button more times than I would have wished while it was displayed on the grid, so I just want to say I really like this proposal. Really nice work, Tim.

I think this option makes sense but I will let Boriss make the final call. I did notice some oddness with the added section: - OSX: The font-size is too small - Windows: There is a large space between the other tab options and the new block, the block is also a lot larger than it needs to be - Linux: Same issues as on Windows

Makes sense to me as well. If we can tweak the UI oddness, that should help clean things up nicely.

Comment on attachment 604224 [details] [diff] [review] Part 1 - Add restore button to the preference dialog Review of attachment 604224 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(In reply to Stephen Horlander from comment #8) > - OSX: The font-size is too small Fixed. > - Windows: There is a large space between the other tab options and the new > block, the block is also a lot larger than it needs to be Fixed. > - Linux: Same issues as on Windows Fixed.

Comment on attachment 607928 [details] [diff] [review] Part 2 - Make sure there are no pinned sites after restoring Review of attachment 607928 [details] [diff] [review]: ----------------------------------------------------------------- looks ok, r=me. though, i'm beginning to think all the newtab code should be converted to emit events only, instead having to do iterate(update) in every little nook and cranny.

Comment on attachment 607926 [details] [diff] [review] Part 1 - Add restore button to the preference dialog > <!-- XXX flex below is a hack because wrapping checkboxes don't reflow > properly; see bug 349098 --> >- <vbox id="tabPrefsBox" align="start" flex="1"> >+ <vbox id="tabPrefsBox" align="start"> r-! Is that comment not valid anymore? >+<!ENTITY restoreNewTabPage.label "Restore all sites from the new tab page"> I'm not sure that's good wording. I'm not even myself sure what "restore" means in this context...

(In reply to Dão Gottwald [:dao] from comment #15) > > <!-- XXX flex below is a hack because wrapping checkboxes don't reflow > > properly; see bug 349098 --> > >- <vbox id="tabPrefsBox" align="start" flex="1"> > >+ <vbox id="tabPrefsBox" align="start"> > > r-! Is that comment not valid anymore? I didn't notice that comment at all, sorry. > >+<!ENTITY restoreNewTabPage.label "Restore all sites from the new tab page"> > > I'm not sure that's good wording. I'm not even myself sure what "restore" > means in this context... I tried hard finding something meaningful. Maybe a native speaker could step in here?

How about: "Restore all website thumbnails in new tab" We also will want to update the support page here with this new option: http://support.mozilla.org/en-US/kb/Options%20window%20-%20Tabs%20panel?as=u +cheng

Again, what exactly does "restore" refer to? What's "all" in "all website thumbnails"? Having witnessed the development process, I guess it refers to manually removed items, although I can imagine that it will push out pinned items as well since you can only have nine items at a time, or that it won't actually bring back all deleted items, since their frecency could be lower by now.

(In reply to Dão Gottwald [:dao] from comment #18) > Having witnessed the development process, I guess it refers to > manually removed items, although I can imagine that it will push out pinned > items as well since you can only have nine items at a time, or that it won't > actually bring back all deleted items, since their frecency could be lower > by now. That's exactly what happens (or might happen). How about: "Undo all customizations applied to the new tab page"

That sounds better to me. To be in line with that, the button should probably say "Reset" rather than "Restore".

(In reply to Dão Gottwald [:dao] from comment #15) > > <!-- XXX flex below is a hack because wrapping checkboxes don't reflow > > properly; see bug 349098 --> > >- <vbox id="tabPrefsBox" align="start" flex="1"> > >+ <vbox id="tabPrefsBox" align="start"> If we want the new groupbox to be positioned right under the vbox and the vbox to behave like it's not flex=1, how can we do that?

This keeps the flex and new groupbox to be positioned right under the vbox.

Unrotted

Unrotted

Comment on attachment 616490 [details] [diff] [review] Part 3 - Correct newtab tests v2 Review of attachment 616490 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/newtab/browser_newtab_reset.js @@ +8,1 @@ > // create a new tab page and check its modified state after blocking a site Nit: the comment needs to be corrected. There's no modified state check anymore. ::: browser/base/content/test/newtab/browser_newtab_tabsync.js @@ +21,4 @@ > // unpin a cell > yield unpinCell(1); > checkGrid("0,1,2,3,4,5,6,7,8"); > + checkGrid("0,1,2,3,4,5,6,7,8", getGrid().sites); The point here is to check if the two active tabs share the same state. You're always checking the second tab's sites. The easiest way is to save the return value of "getGrid()" to a variable before opening the 2nd tab. And then you can check the grid state by calling: checkGrid("0,1,2,3,4,5,6,7,8", oldGrid.sites);

(In reply to Tim Taubert [:ttaubert] from comment #25) > Comment on attachment 616490 [details] [diff] [review] > Part 3 - Correct newtab tests v2 > > Review of attachment 616490 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/newtab/browser_newtab_reset.js > @@ +8,1 @@ > > // create a new tab page and check its modified state after blocking a site > > Nit: the comment needs to be corrected. There's no modified state check > anymore. Updated. > > ::: browser/base/content/test/newtab/browser_newtab_tabsync.js > @@ +21,4 @@ > > // unpin a cell > > yield unpinCell(1); > > checkGrid("0,1,2,3,4,5,6,7,8"); > > + checkGrid("0,1,2,3,4,5,6,7,8", getGrid().sites); > > The point here is to check if the two active tabs share the same state. > You're always checking the second tab's sites. > > The easiest way is to save the return value of "getGrid()" to a variable > before opening the 2nd tab. And then you can check the grid state by calling: > > checkGrid("0,1,2,3,4,5,6,7,8", oldGrid.sites); Updated.

Tim, bug 722234 is blocked on input from the UX people. For the time being, this is the only way of restoring thumbnails. Please land this quickly.

Comment on attachment 622269 [details] [diff] [review] Part 3 - Correct newtab tests v3 Review of attachment 622269 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: browser/base/content/test/newtab/browser_newtab_reset.js @@ +4,5 @@ > /* > * These tests make sure that resetting the 'New Tage Page' works as expected. > */ > function runTests() { > + // create a new tab page and check after blocking a site and restoring the grid state. Nit: create a new tab page, block a site and restore the grid state

r=ttaubert

r=ttaubert

Still waiting for windows build. Will post a screenshot when it's available.

(In reply to Raymond Lee [:raymondlee] from comment #33) > Created attachment 628801 [details] > mac screenshot > > Still waiting for windows build. Will post a screenshot when it's available. Try url https://tbpl.mozilla.org/?tree=Try&rev=5d35edfa07be

The patch didn't apply cleanly so updated it.

Tim: could you review the Part 1 patch for this bug please?

Comment on attachment 628793 [details] [diff] [review] Part 1 - Add restore button to the preference dialog v4 Sorry that it took me so long to review this. Thank you for working on this bug but we'll not need this anymore now that we're going to implement bug 722234. It will offer an option restore all pages that have been removed from the newtab grid.