716543 - [New Tab Page] Remove the reset/reload button and move it to the preferences dialog

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 1

[Siddhartha Dugar [:sdrocking]]

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.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - Add restore button to the preference dialog

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - Add NewTabUtils.restore() functionality

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - Correct newtab tests

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 604224 [details] [diff] [review]
Part 1 - Add restore button to the preference dialog

Included in tomorrow's UX nightly.

Comment 7

[Virgil Dicu [:virgil] [QA]]

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.

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: Restore Section Layout Issues

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

Comment 9

[Chris Lee [:clee]]

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

Comment 10

[Jennifer Morrow [:Boriss] (UX)]

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!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - Add restore button to the preference dialog

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

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - Make sure there are no pinned sites after restoring

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - Correct newtab tests

Comment 14

[Dietrich Ayala (:dietrich)]

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 15

[Dão Gottwald [:dao]]

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 17

[Chris Lee [:clee]]

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

Comment 18

[Dão Gottwald [:dao]]

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.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 20

[Dão Gottwald [:dao]]

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

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Add restore button to the preference dialog v2

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

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - Make sure there are no pinned sites after restoring v2

Unrotted

Comment 24

[Raymond Lee [:raymondlee]]

Attachment: Part 3 - Correct newtab tests v2

Unrotted

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 26

[Raymond Lee [:raymondlee]]

Attachment: Part 3 - Correct newtab tests v3

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

Comment 27

[Siddhartha Dugar [:sdrocking]]

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 28

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Add restore button to the preference dialog v4

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - Make sure there are no pinned sites after restoring v4

r=ttaubert

Comment 31

[Raymond Lee [:raymondlee]]

Attachment: Part 3 - Correct newtab tests v4

r=ttaubert

Comment 32

[Raymond Lee [:raymondlee]]

Attachment: ubuntu screenshot

Comment 33

[Raymond Lee [:raymondlee]]

Attachment: mac screenshot

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

Comment 34

[Raymond Lee [:raymondlee]]

(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

Comment 35

[Raymond Lee [:raymondlee]]

Attachment: windows screenshot

Comment 36

[Raymond Lee [:raymondlee]]

Attachment: Part 3 - Correct newtab tests v4

The patch didn't apply cleanly so updated it.

Comment 37

[Raymond Lee [:raymondlee]]

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

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

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.