Last Comment Bug 722234 - [New Tab Page] provide an option to undo remove a site
: [New Tab Page] provide an option to undo remove a site
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 10 votes (vote)
: Firefox 21
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
: 804892 817936 (view as bug list)
Depends on:
Blocks: 455553
  Show dependency treegraph
 
Reported: 2012-01-29 23:31 PST by Siddhartha Dugar [:sdrocking]
Modified: 2013-02-26 06:59 PST (History)
30 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
21+


Attachments
patch v1 (15.08 KB, patch)
2012-02-21 08:28 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
Screencast showing the undo dialog (990.79 KB, video/mp4)
2012-02-21 10:16 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details
patch v2 (13.13 KB, patch)
2012-03-08 15:02 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v3 (13.10 KB, patch)
2012-03-08 16:54 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
Updated patch (12.12 KB, patch)
2012-04-23 17:27 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Splinter Review
Screencast (559.31 KB, video/quicktime)
2012-04-26 15:16 PDT, Andres Hernandez [:andreshm]
no flags Details
Screenshot (590.04 KB, image/png)
2012-04-26 15:16 PDT, Andres Hernandez [:andreshm]
no flags Details
Mockup: A notification which allows users to undo a single thumbnail removal or to restore to default state (538.96 KB, image/png)
2012-07-19 18:39 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
Patch v5 (16.05 KB, patch)
2012-07-27 08:23 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v6 (16.46 KB, patch)
2012-08-01 09:26 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v7 (16.52 KB, patch)
2012-08-03 10:57 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v8 (16.46 KB, patch)
2012-08-07 12:51 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Splinter Review
Patch v9 (16.83 KB, patch)
2012-08-21 16:58 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v10 (16.86 KB, patch)
2012-08-22 17:10 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v10.1 (16.99 KB, patch)
2012-08-22 17:28 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v10 (16.91 KB, patch)
2012-08-22 18:10 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Mac screenshot (46.05 KB, image/jpeg)
2012-08-22 18:11 PDT, Andres Hernandez [:andreshm]
no flags Details
Patch v11 (17.50 KB, patch)
2012-08-23 15:38 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Mac screenshot (12.68 KB, image/png)
2012-08-23 15:40 PDT, Andres Hernandez [:andreshm]
no flags Details
patch v11b (17.79 KB, patch)
2013-01-15 03:35 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v11c (17.94 KB, patch)
2013-01-15 05:56 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
ttaubert: review+
Details | Diff | Splinter Review
Screenshots for Windows, Mac, Linux (top to bottom) (34.33 KB, image/png)
2013-01-15 07:38 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
shorlander: ui‑review+
Details

Description Siddhartha Dugar [:sdrocking] 2012-01-29 23:31:11 PST
In Google Chrome there is an option to undo the removal of a thumbnail. This function is useful if you accidentally delete a thumbnail. We should have something similar in Firefox.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-18 14:56:38 PST
I really like how Chrome handles this. I concur, we should have something similar.
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-21 08:28:35 PST
Created attachment 599190 [details] [diff] [review]
patch v1
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-21 10:16:08 PST
Created attachment 599244 [details]
Screencast showing the undo dialog

Here's a screencast showing the implementation of the current patch. Looking forward to some feedback and maybe some real mockups from the UX team!
Comment 4 Ursan Marius Bogdan 2012-02-21 23:28:27 PST
Nice screencast Tim :)
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-08 15:02:58 PST
Created attachment 604217 [details] [diff] [review]
patch v2
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-08 16:38:58 PST
Comment on attachment 604217 [details] [diff] [review]
patch v2

Included in tomorrow's UX nightly.
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-08 16:54:05 PST
Created attachment 604263 [details] [diff] [review]
patch v3
Comment 8 Siddhartha Dugar [:sdrocking] 2012-03-14 01:44:22 PDT
This needs to be in Firefox 13. Currently, there is now way to undo removal of a site or resetting the page using the new layout.
Comment 9 Siddhartha Dugar [:sdrocking] 2012-03-27 01:04:39 PDT
Tim, this worked well when we had it on UX. Why aren't you taking it to the next level?
Comment 10 Chris Lee [:clee] 2012-03-27 10:54:28 PDT
Tim, how close are we to landing this patch?  Depending on timing we can decide if we should/can make an aurora approval request or just target Fx14.
Comment 11 Siddhartha Dugar [:sdrocking] 2012-04-11 12:33:49 PDT
Tim, this is needed in Nightly and Firefox 13 to make the New Tab page feature complete. Please land the patch ASAP.
Comment 12 Jim Jeffery not reading bug-mail 1/2/11 2012-04-11 13:09:08 PDT
(In reply to Siddhartha Dugar [:sdrocking] from comment #11)
> Tim, this is needed in Nightly and Firefox 13 to make the New Tab page
> feature complete. Please land the patch ASAP.

This is still pending reviews - so its not going anywhere yet, and I believe Tim is working on Mobile/Fennec stuff at the moment..
Comment 13 Andres Hernandez [:andreshm] 2012-04-23 17:27:39 PDT
Created attachment 617711 [details] [diff] [review]
Updated patch

Re-based patch and test.
Comment 14 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-26 12:38:51 PDT
Comment on attachment 617711 [details] [diff] [review]
Updated patch

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

This looks good to me but as I wrote most of that patch I'm only giving f+ and we'll need review from another peer as well. Before we do that, can you please take a screenshot and make a screencast showing this feature? We then can ask for UX review and they'll probably suggest some better styling.

(I didn't really look at the CSS because that's probably going to change.)
Comment 15 Andres Hernandez [:andreshm] 2012-04-26 15:16:11 PDT
Created attachment 618818 [details]
Screencast
Comment 16 Andres Hernandez [:andreshm] 2012-04-26 15:16:41 PDT
Created attachment 618819 [details]
Screenshot
Comment 17 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-27 07:15:23 PDT
Comment on attachment 618818 [details]
Screencast

Asking for UX input whether we should include this feature and if so we'll probably need some better layout for this. We can of course push it to the UX branch if that helps.

(There's a screenshot and a screencast attached to this bug.)
Comment 18 Asa Dotzler [:asa] 2012-04-27 07:36:18 PDT
That feels a bit invasive for an undo action. I wonder if placing it at the bottom and potentially right aligned, or some other visual treatment, would help. I'm also not convinced we need visual UI for that. Would ctrl+z be sufficient?
Comment 19 Siddhartha Dugar [:sdrocking] 2012-04-27 10:35:07 PDT
(In reply to Asa Dotzler [:asa] from comment #18)
> I'm also not convinced we need visual UI for that. Would ctrl+z be sufficient?

Using the mouse for removal and keyboard for undoing would be weird. There has to be a intuitive way for undoing.

IMO the alignment in the screenshot is good enough. Otherwise we can have a "trash" icon in the bottom right corner which would animate when a site is removed and would do an undo upon clicking. This would also allow multiple undo's.
Comment 20 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-05-22 06:57:02 PDT
I'd really like to get input from the UX people. It would be definitely nice to have this feature, we just need some guidance on how to implement this.
Comment 21 Jennifer Morrow [:Boriss] (UX) 2012-07-19 18:39:38 PDT
Created attachment 644116 [details]
Mockup: A notification which allows users to undo a single thumbnail removal or to restore to default state

(In reply to Tim Taubert [:ttaubert] from comment #20)
> I'd really like to get input from the UX people. It would be definitely nice
> to have this feature, we just need some guidance on how to implement this.

It would definitely be awesome to get this in.  I'm attaching a mockup of what a button which undoes a single removed site but also allows for restoring to default setting.  Unfortunately/coincidentally, it's roughly what Chrome does, but they have a point - it's a nifty way to nip both cases (restore one and restore all) in the bud without adding additional UI.
Comment 22 Andres Hernandez [:andreshm] 2012-07-24 12:09:18 PDT
I have some questions regarding new changes:

The 'restore all' link will restore all previous blocked links in the same session. I mean, the list is clean when Fx closes? Or we use the timeout to remove each item from the restoring list? 

Should I use the same new tab controls image for the X button? 
For the link color -moz-nativehyperlinktext is ok? Should have a hover state?
Comment 23 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-25 05:58:24 PDT
(In reply to Andres Hernandez [:andreshm] from comment #22)
> The 'restore all' link will restore all previous blocked links in the same
> session. I mean, the list is clean when Fx closes? Or we use the timeout to
> remove each item from the restoring list? 

I'm not sure what you mean with 'the timeout'?  When clicking 'restore all' we're basically clearing the blocked sites list and removing all pinned sites. Patch v1 did exactly this, you can probably take a look at how it worked.

> Should I use the same new tab controls image for the X button?
> For the link color -moz-nativehyperlinktext is ok? Should have a hover state?

Just go ahead and try with the close image. No idea about the color, we probably want to underline on hover. I think we can start without this information as that will be very small fixes and we should ask for ux-review anyway before landing this.
Comment 24 Siddhartha Dugar [:sdrocking] 2012-07-25 21:46:26 PDT
(In reply to Tim Taubert [:ttaubert] from comment #23)
> (In reply to Andres Hernandez [:andreshm] from comment #22)
> > The 'restore all' link will restore all previous blocked links in the same
> > session. I mean, the list is clean when Fx closes? Or we use the timeout to
> > remove each item from the restoring list? 
> 
> I'm not sure what you mean with 'the timeout'?  When clicking 'restore all'
> we're basically clearing the blocked sites list and removing all pinned
> sites.

Please do not remove pinned sites. We already have other ways to unpin, which are easily accessible. Also the string "Restore all" implies restoring just the deleted sites.
Comment 25 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-26 02:28:55 PDT
(In reply to Siddhartha Dugar [:sdrocking] from comment #24)
> Please do not remove pinned sites. We already have other ways to unpin,
> which are easily accessible. Also the string "Restore all" implies restoring
> just the deleted sites.

Thinking about this... you're probably right. That's actually how I'd like to feature to work, too. Let's not remove pinned sites unless UX says otherwise.
Comment 26 Andres Hernandez [:andreshm] 2012-07-27 08:23:09 PDT
Created attachment 646590 [details] [diff] [review]
Patch v5
Comment 27 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-31 09:05:09 PDT
Comment on attachment 646590 [details] [diff] [review]
Patch v5

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

We can't let the undo container be a sibling of the newtab margins and position it absolutely. If make the window small enough the undo dialog overlays the thumbnails. We need to put it into #newtab-margin-top and utilize the flexbox model. Also, the close button looks not like the one in the mockup - it's a bit too heavy.

::: browser/base/content/newtab/undo.js
@@ +22,5 @@
> +  /**
> +   * Initializes the undo dialog.
> +   */
> +  init: function UndoDialog_init() {
> +    this._undoBox = document.getElementById("newtab-undo-box");

We don't need to register event listeners for all elements. We can just register a single "click" listener for #newtab-undo-box and then check the event target like you already did.

@@ +26,5 @@
> +    this._undoBox = document.getElementById("newtab-undo-box");
> +    this._undoButton = document.getElementById("newtab-undo-button");
> +    this._undoButton.addEventListener("click", this, false);
> +    this._undoAllButton = document.getElementById("newtab-undo-all-button");
> +    this._undoAllButton.addEventListener("click", this, false);

We don't need to save those two buttons if we wait for the event to bubble up.

@@ +59,5 @@
> +      return;
> +
> +    clearTimeout(this._undoData.timeout);
> +    this._undoData = null;
> +    this._undoBox.setAttribute("hidden", true);

this._undoBox.setAttribute("hidden", "true");

::: browser/base/content/test/newtab/browser_newtab_undo.js
@@ +5,5 @@
> + * These tests make sure that the undo dialog works as expected.
> + */
> +function runTests() {
> +  // remove unpinned sites and undo it
> +  setLinks("0,1,2,3,4,5,6,7,8");

yield setLinks("0,1,2,3,4,5,6,7,8");

::: browser/base/content/test/newtab/head.js
@@ +207,5 @@
>    browser.addEventListener("load", function onLoad() {
>      browser.removeEventListener("load", onLoad, true);
>  
>      if (NewTabUtils.allPages.enabled) {
> +      waitForFocus(function () {

Do we really need this here? Do we want to do this for every tab we're opening or just once at the beginning of a test? (I see, I included this in my first patches for this bug but I can't exactly remember why.)

::: browser/locales/en-US/chrome/browser/newTab.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- These strings are used in the about:newtab page -->
>  <!ENTITY newtab.pageTitle "New Tab">
> +<!ENTITY newtab.undo.removed.label "Thumbnail removed.">

Better: newtab.undo.removedLabel

@@ +4,5 @@
>  
>  <!-- These strings are used in the about:newtab page -->
>  <!ENTITY newtab.pageTitle "New Tab">
> +<!ENTITY newtab.undo.removed.label "Thumbnail removed.">
> +<!ENTITY newtab.undo.button.label "Undo?">

newtab.undo.undoButton

@@ +5,5 @@
>  <!-- These strings are used in the about:newtab page -->
>  <!ENTITY newtab.pageTitle "New Tab">
> +<!ENTITY newtab.undo.removed.label "Thumbnail removed.">
> +<!ENTITY newtab.undo.button.label "Undo?">
> +<!ENTITY newtab.undo.all.button.label "Restore All?">

newtab.undo.restoreButton

@@ +6,5 @@
>  <!ENTITY newtab.pageTitle "New Tab">
> +<!ENTITY newtab.undo.removed.label "Thumbnail removed.">
> +<!ENTITY newtab.undo.button.label "Undo?">
> +<!ENTITY newtab.undo.all.button.label "Restore All?">
> +<!ENTITY newtab.undo.close.tooltip "Hide">

newtab.undo.closeTooltip

::: browser/modules/NewTabUtils.jsm
@@ +596,5 @@
>      Storage.clear();
>      Links.resetCache();
> +    if (!aKeepPinnedTabs) {
> +      PinnedLinks.resetCache();
> +    }

This just resets the cache of the pinned links. We need to modify the Storage.clear() call as that removes the pinnedLinks from the storage. Resetting the pinnedLinksCache doesn't really hurt.
Comment 28 Andres Hernandez [:andreshm] 2012-07-31 09:39:36 PDT
(In reply to Tim Taubert [:ttaubert] from comment #27)
> Comment on attachment 646590 [details] [diff] [review]

> Also, the close button looks not like the one in
> the mockup - it's a bit too heavy.

I was temporally using the new tab controls image. But, I don't know how to proceed with this, should I request the image to someone?
Comment 29 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-31 09:44:30 PDT
(In reply to Andres Hernandez [:andreshm] from comment #28)
> > Also, the close button looks not like the one in
> > the mockup - it's a bit too heavy.
> 
> I was temporally using the new tab controls image. But, I don't know how to
> proceed with this, should I request the image to someone?

Yes... let's ask Boriss. In the meantime you can just continue to use it until we have some UX feedback so that the patch is ready.

Boriss, should we use some existing image here or should we wait for you to provide a new one?
Comment 30 Jennifer Morrow [:Boriss] (UX) 2012-07-31 10:42:40 PDT
(In reply to Tim Taubert [:ttaubert] from comment #29)
> (In reply to Andres Hernandez [:andreshm] from comment #28)
> > > Also, the close button looks not like the one in
> > > the mockup - it's a bit too heavy.
> > 
> > I was temporally using the new tab controls image. But, I don't know how to
> > proceed with this, should I request the image to someone?
> 
> Yes... let's ask Boriss. In the meantime you can just continue to use it
> until we have some UX feedback so that the patch is ready.
> 
> Boriss, should we use some existing image here or should we wait for you to
> provide a new one?

Something more like the tab close button (Shorlander's suggestion) could do well here.  I'll attach an image
Comment 31 Andres Hernandez [:andreshm] 2012-08-01 09:26:36 PDT
Created attachment 647984 [details] [diff] [review]
Patch v6

Applied changes, but we still need to update it with the proper close image. 

> ::: browser/base/content/test/newtab/head.js
> @@ +207,5 @@
> >    browser.addEventListener("load", function onLoad() {
> >      browser.removeEventListener("load", onLoad, true);
> >  
> >      if (NewTabUtils.allPages.enabled) {
> > +      waitForFocus(function () {
> 
> Do we really need this here? Do we want to do this for every tab we're
> opening or just once at the beginning of a test? (I see, I included this in
> my first patches for this bug but I can't exactly remember why.)

I don't think we need it, it works fine without it.
Comment 32 Andres Hernandez [:andreshm] 2012-08-01 09:27:09 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06
Comment 33 Andres Hernandez [:andreshm] 2012-08-01 19:53:57 PDT
Try finished with only one intermittent orange. 

https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06
Comment 34 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-02 03:12:28 PDT
Comment on attachment 647984 [details] [diff] [review]
Patch v6

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

::: browser/base/content/newtab/newTab.css
@@ +19,5 @@
>  }
>  
> +/* UNDO */
> +#newtab-undo-container {
> +  display: inline-block;

We don't seem to need that, do we?

@@ +21,5 @@
> +/* UNDO */
> +#newtab-undo-container {
> +  display: inline-block;
> +  -moz-transition: opacity 100ms ease-out;
> +}

Nit: please add a newline here.

::: browser/base/content/newtab/newTab.xul
@@ +25,5 @@
> +          <span id="newtab-undo-label">&newtab.undo.removedLabel;</span>
> +          <a id="newtab-undo-button" class="newtab-text-link">
> +            &newtab.undo.undoButton;</a>
> +          <a id="newtab-undo-restore-button" class="newtab-text-link">
> +            &newtab.undo.restoreButton;</a>

I think those two should be buttons as well. They're a specific action and don't take you anywhere.

::: browser/base/content/newtab/page.js
@@ +41,5 @@
>      // Initialize the whole page if we haven't done that, yet.
>      if (enabled)
>        this._init();
> +    else
> +      gUndoDialog.hide();

Nit: Please add braces for both branches here.

::: browser/base/content/test/newtab/browser_newtab_undo.js
@@ +24,5 @@
> +  checkGrid("0,1,2,4,6,7,8");
> +
> +  yield undo();
> +  checkGrid("5p,0,1,2,4,6,7,8");
> +  

Nit: the awesome review tool seems to indicate here's an empty line constisting of spaces, only :)

::: browser/modules/NewTabUtils.jsm
@@ +79,2 @@
>     */
> +  clear: function Storage_clear(aKeepPinnedTabs) {

I think Storage.clear() should take a list of keys to keep. and then do:

> function Storage_clear(aKeysToRetain = []) {
>   for (let i = 0; i < this.domStorage.length; i++) {
>     let key = this.domStorage.key(i);
>     if (!(key in aKeysToRetain)) {
>       this.domStorage.removeItem(key);
>     }
>   }
> }

This way the Storage object itself stays independent without knowing too much about pinned tabs or the like.

::: browser/themes/gnomestripe/newtab/newTab.css
@@ +41,5 @@
> +  height: 24px;
> +  border: none;
> +  background: transparent url(chrome://browser/skin/newtab/controls.png);
> +  background-position: -144px 0;
> +}

Nit: please add a newline here.

@@ +44,5 @@
> +  background-position: -144px 0;
> +}
> +#newtab-undo-close-button:hover {
> +  background-position: -168px 0;
> +}

and here.

::: browser/themes/pinstripe/newtab/newTab.css
@@ +41,5 @@
> +  height: 24px;
> +  border: none;
> +  background: transparent url(chrome://browser/skin/newtab/controls.png);
> +  background-position: -144px 0;
> +}

Nit: please add a newline here.

@@ +44,5 @@
> +  background-position: -144px 0;
> +}
> +#newtab-undo-close-button:hover {
> +  background-position: -168px 0;
> +}

and here.

::: browser/themes/winstripe/newtab/newTab.css
@@ +41,5 @@
> +  height: 24px;
> +  border: none;
> +  background: transparent url(chrome://browser/skin/newtab/controls.png);
> +  background-position: -144px 0;
> +}

Nit: please add a newline here.

@@ +44,5 @@
> +  background-position: -144px 0;
> +}
> +#newtab-undo-close-button:hover {
> +  background-position: -168px 0;
> +}

and here.
Comment 35 Andres Hernandez [:andreshm] 2012-08-03 10:57:30 PDT
Created attachment 648772 [details] [diff] [review]
Patch v7

Updated patch with suggest changes. 
Local tests are running fine.
Comment 36 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-07 07:12:31 PDT
Comment on attachment 648772 [details] [diff] [review]
Patch v7

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

::: browser/base/content/newtab/newTab.css
@@ +19,5 @@
>  }
>  
> +/* UNDO */
> +#newtab-undo-container {
> +  -moz-transition: opacity 100ms ease-out;

transitions are unprefixed now.

@@ +22,5 @@
> +#newtab-undo-container {
> +  -moz-transition: opacity 100ms ease-out;
> +}
> +
> +#newtab-undo-container[hidden] {

"hidden" is a default attribute that sets 'display: none' and will thus prevent the container's transition. We should just use a different attribute name here, like "undo-disabled".

::: browser/base/content/newtab/newTab.xul
@@ +26,5 @@
> +          <input id="newtab-undo-button" type="button"
> +                 value="&newtab.undo.undoButton;" class="newtab-undo-button" />
> +          <input id="newtab-undo-restore-button" type="button"
> +                 value="&newtab.undo.restoreButton;"
> +                 class="newtab-undo-button" />

Those both buttons need a tabindex=-1 attribute and appropriate handling in undo.js as well.

::: browser/base/content/newtab/undo.js
@@ +80,5 @@
> +
> +  /**
> +   * Undo the last blocked site.
> +   */
> +  _undo : function UndoDialog_undo() {

Nit: please remove the space before ":"

@@ +88,5 @@
> +    let {index, wasPinned, blockedLink} = this._undoData;
> +    gBlockedLinks.unblock(blockedLink);
> +
> +    if (wasPinned)
> +      gPinnedLinks.pin(blockedLink, index);

Nit: please add braces here.

@@ +98,5 @@
> +  /**
> +   * Undo all blocked sites.
> +   */
> +  _undoAll: function UndoDialog_undoAll() {
> +    NewTabUtils.restore(["pinnedLinks"]);

I just noticed that when using the "restore all" button, there is no animation but pages are just updated. This is because NewTabUtils.restore() just calls |AllPages.update()|. What we actually want is to maybe pass a callback to NTU.restore() and then call |gUpdater.updateGrid()| which will use animations to update the grid and then calls |AllPages.update()| for us.

Also, we could maybe introduce a second method a la |NewTabUtils.undoAll()| that calls Storage.remove("pinnedLinks") and a given callback when finished. That means we'd have two quite similar functions but they're used for different purposes and the frontend doesn't need to know about storage internals. Also we don't need to touch Storage.clear() and NTU.restore() then.

::: browser/modules/NewTabUtils.jsm
@@ +79,5 @@
>     */
> +  clear: function Storage_clear(aKeysToRetain = []) {
> +    for (let i = this.domStorage.length - 1; i >= 0; i--) {
> +      let key = this.domStorage.key(i);
> +      if (-1 == aKeysToRetain.indexOf(key)) {

Nit: no yoda conditions, please --> if (aKeysToRetain.indexOf(key) == -1)

::: browser/themes/gnomestripe/newtab/newTab.css
@@ +45,5 @@
> +.newtab-undo-button:hover {
> +  text-decoration: underline;
> +}
> +
> +#newtab-undo-close-button {

We should add 'padding: 1px 2px 3px' here so that the outline of the button looks nice when focusing it with they keyboard. The values will probably change if the new icon size differs. Please apply this to all themes.
Comment 37 Andres Hernandez [:andreshm] 2012-08-07 12:51:43 PDT
Created attachment 649762 [details] [diff] [review]
Patch v8

Applied changes.
Comment 38 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-08 06:48:10 PDT
Comment on attachment 649762 [details] [diff] [review]
Patch v8

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

Thank you! r=me with the small fixes below. Now we just need to wait for a neat close icon.

::: browser/modules/NewTabUtils.jsm
@@ +610,5 @@
> +  undoAll: function NewTabUtils_undoAll(aCallback) {
> +    let pinnedLinks = PinnedLinks.links;
> +
> +    Storage.clear();
> +    Storage.set("pinnedLinks", pinnedLinks);

Please add a Storage.remove() method. Calling |Storage.remove("blockedLinks") seems a little more elegant to me.

@@ +612,5 @@
> +
> +    Storage.clear();
> +    Storage.set("pinnedLinks", pinnedLinks);
> +    Links.resetCache();
> +    PinnedLinks.resetCache();

We're not changing the pinnedLinks list. No need to reset the cache.
Comment 39 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-08 06:51:08 PDT
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #30)
> Something more like the tab close button (Shorlander's suggestion) could do
> well here.  I'll attach an image

Hey Boriss, the feature itself is ready. We now only need a better close icon before we can land this.
Comment 40 Siddhartha Dugar [:sdrocking] 2012-08-20 22:30:28 PDT
Please set this to use an existing icon (like the tab close button) and land it. We can replace the icon with a better one later. I do not want this to miss another train.
Comment 41 Asa Dotzler [:asa] 2012-08-20 23:06:32 PDT
(In reply to Siddhartha Dugar [:sdrocking] from comment #40)
> Please set this to use an existing icon (like the tab close button) and land
> it. We can replace the icon with a better one later. I do not want this to
> miss another train.

Siddhartha, please don't comment in bugs unless you're adding information that will help us fix the bug. Thanks. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 42 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-21 12:22:20 PDT
Andres, can you please try to use the tab-close icon? I talked to shorlander and he said it might work!
Comment 43 Jennifer Morrow [:Boriss] (UX) 2012-08-21 16:33:47 PDT
(In reply to Tim Taubert [:ttaubert] from comment #42)
> Andres, can you please try to use the tab-close icon? I talked to shorlander
> and he said it might work!

Actually, let's try this first.  Perhaps we can just tweak them if they don't work.  

In mxr:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/icons/close.png
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/icons/close.png
Comment 44 Andres Hernandez [:andreshm] 2012-08-21 16:58:41 PDT
Created attachment 654020 [details] [diff] [review]
Patch v9

Updated to use the suggested images.

Tim please confirm it looks good on linux.
Comment 45 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-22 16:37:54 PDT
(In reply to Andres Hernandez [:andreshm] from comment #44)
> Tim please confirm it looks good on linux.

This doesn't look quite right to me :)

http://i.imgur.com/Qgywd.png

The gtk close image itself is only 16x16.
Comment 46 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-22 16:39:30 PDT
You should also set the background-image to no-repeat, I think.
Comment 47 Andres Hernandez [:andreshm] 2012-08-22 17:10:44 PDT
Created attachment 654438 [details] [diff] [review]
Patch v10

Sorry, my fault, I just fixed the icon size. 

(In reply to Tim Taubert [:ttaubert] from comment #46)
> You should also set the background-image to no-repeat, I think.

I think is not necessary, actually with this the hover and active icons don't work.
Comment 48 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-22 17:14:21 PDT
(In reply to Andres Hernandez [:andreshm] from comment #47)
> Sorry, my fault, I just fixed the icon size. 

That is better but we'll probably need a min-height for the #undo-container. Looks a little flat: http://i.imgur.com/1NvmH.png
Comment 49 Andres Hernandez [:andreshm] 2012-08-22 17:28:02 PDT
Created attachment 654451 [details] [diff] [review]
Patch v10.1

Try with this one.
Comment 50 Andres Hernandez [:andreshm] 2012-08-22 18:10:00 PDT
Created attachment 654470 [details] [diff] [review]
Patch v10

Minor fixes
Comment 51 Andres Hernandez [:andreshm] 2012-08-22 18:11:52 PDT
Created attachment 654473 [details]
Mac screenshot
Comment 52 Dão Gottwald [:dao] 2012-08-22 23:51:18 PDT
Comment on attachment 654470 [details] [diff] [review]
Patch v10

>+  width: 16px;
>+  height: 16px;
>+  border: none;
>+  background: transparent url("moz-icon://stock/gtk-close?size=menu");

I don't think you rely count on this icon having a size of 16*16 pixels.
Comment 53 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-23 12:43:50 PDT
Comment on attachment 654470 [details] [diff] [review]
Patch v10

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

Some hints I discovered while trying it out:

* all buttons should have |-moz-appearance: none| so that they look like we want them to
* you should give the <xul:button>s a |min-width: 0| or else they'll be wider than necessary

::: browser/base/content/newtab/newTab.xul
@@ +21,5 @@
>  
>      <div id="newtab-vertical-margin">
> +      <div id="newtab-margin-top">
> +        <div id="newtab-undo-container" undo-disabled="true">
> +          <span id="newtab-undo-label">&newtab.undo.removedLabel;</span>

Let's turn this into a <xul:label value="&newtab...">

@@ +26,5 @@
> +          <input id="newtab-undo-button" type="button" tabindex="-1"
> +                 value="&newtab.undo.undoButton;" class="newtab-undo-button" />
> +          <input id="newtab-undo-restore-button" type="button" tabindex="-1"
> +                 value="&newtab.undo.restoreButton;"
> +                 class="newtab-undo-button" />

These should now be <xul:button>s

@@ +28,5 @@
> +          <input id="newtab-undo-restore-button" type="button" tabindex="-1"
> +                 value="&newtab.undo.restoreButton;"
> +                 class="newtab-undo-button" />
> +          <input id="newtab-undo-close-button" type="button" tabindex="-1"
> +                 title="&newtab.undo.closeTooltip;" />

If you make this a <xul:toolbarbutton> with a list-style-image it will adjust its size and the container's size to the image dimensions.

::: browser/themes/gnomestripe/newtab/newTab.css
@@ +22,5 @@
> +  border-color: rgba(8,22,37,.12) rgba(8,22,37,.14) rgba(8,22,37,.16);
> +  background-color: #f7f7f7;
> +  font-size: 12px;
> +  color: #525e69;
> +  height: 30px;

We don't really need this here if we increase the top and bottom padding to 5px.

@@ +50,5 @@
> +#newtab-undo-close-button {
> +  width: 16px;
> +  height: 16px;
> +  border: none;
> +  background: transparent url("moz-icon://stock/gtk-close?size=menu");

You can now just use:

list-style-image: url("moz-icon://stock/gtk-close?size=menu");
Comment 54 Andres Hernandez [:andreshm] 2012-08-23 15:38:02 PDT
Created attachment 654823 [details] [diff] [review]
Patch v11

Applied changes.
Comment 55 Andres Hernandez [:andreshm] 2012-08-23 15:40:29 PDT
Created attachment 654825 [details]
Mac screenshot
Comment 56 Dão Gottwald [:dao] 2012-10-23 20:27:39 PDT
*** Bug 804892 has been marked as a duplicate of this bug. ***
Comment 57 Ekanan Ketunuti 2012-12-03 22:59:45 PST
*** Bug 817936 has been marked as a duplicate of this bug. ***
Comment 58 henryfhchan 2013-01-09 09:07:00 PST
can someone land this?
Comment 59 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-15 03:35:26 PST
Created attachment 702210 [details] [diff] [review]
patch v11b

Rebased the patch.
Comment 60 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-15 05:56:22 PST
Created attachment 702261 [details] [diff] [review]
patch v11c

Added a couple of style fixes for paddings, margins and the close buttons.
Comment 61 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-15 07:38:46 PST
Created attachment 702315 [details]
Screenshots for Windows, Mac, Linux (top to bottom)

Final screenshots from all three platforms.
Comment 62 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-15 07:43:44 PST
Comment on attachment 702261 [details] [diff] [review]
patch v11c

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

r=me for Andres' version including a couple of padding/margin - so non-functional - style fixes from me.
Comment 63 Stephen Horlander [:shorlander] 2013-01-15 08:47:44 PST
Comment on attachment 702315 [details]
Screenshots for Windows, Mac, Linux (top to bottom)

Looks good, thank you!
Comment 64 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-15 09:22:04 PST
https://tbpl.mozilla.org/?tree=Try&rev=9ed67430f288
Comment 65 Dão Gottwald [:dao] 2013-01-15 13:59:34 PST
Why do the Undo and Restore All actions come with question marks?
Comment 66 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-16 04:04:48 PST
I like the question marks because they subtly change the dialog a little. For me, it expresses that the dialog is very optional and just there in case you might want to revert a change. You're totally fine to ignore it or just not pick an answer and it will fade out after a couple of seconds.

OTOH, I understand that this is not totally common in our code base and we might want some UX input here. I could also land this feature as is and file a follow-up for it.
Comment 67 Dão Gottwald [:dao] 2013-01-16 05:14:24 PST
Apart from being completely uncommon (AFAIK we don't do this anywhere else), it seems to me that the question marks could (somewhat like ellipses) make the user think that these aren't direct actions.
Comment 68 henryfhchan 2013-01-16 06:54:29 PST
Would it be better to make them all buttons like the cross, and do without the question marks?
Comment 69 Siddhartha Dugar [:sdrocking] 2013-01-16 20:46:31 PST
(In reply to Dão Gottwald [:dao] from comment #67)
> Apart from being completely uncommon (AFAIK we don't do this anywhere else),
> it seems to me that the question marks could (somewhat like ellipses) make
> the user think that these aren't direct actions.

Totally agree with this. Anyways, can't the strings be handled in a follow-up?
Comment 70 Dão Gottwald [:dao] 2013-01-17 06:13:26 PST
I think we should just land this with dots rather than question marks.
Comment 71 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-17 06:59:19 PST
Maybe just without any dots/ellipsis or question mark? I agree with your argument that it might indicate an indirect action otherwise.
Comment 72 Dão Gottwald [:dao] 2013-01-17 08:08:57 PST
When I said dots I meant single dots, just to separate the actions.
Comment 73 Dão Gottwald [:dao] 2013-01-17 08:12:33 PST
Comment on attachment 702261 [details] [diff] [review]
patch v11c

>--- a/browser/themes/gnomestripe/newtab/newTab.css
>+++ b/browser/themes/gnomestripe/newtab/newTab.css

>+#newtab-undo-container {
>+  padding: 4px 3px;
>+  border: 1px solid;
>+  border-color: rgba(8,22,37,.12) rgba(8,22,37,.14) rgba(8,22,37,.16);
>+  background-color: #f7f7f7;
>+  font-size: 12px;
>+  color: #525e69;
>+}

Not sure why you're hardcoding the font size here.

>+#newtab-undo-label {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+}
>+
>+.newtab-undo-button {
>+  -moz-appearance: none;
>+  color: -moz-nativehyperlinktext;

System text color on hard-coded background color is potentially illegible.
Comment 74 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-21 04:39:52 PST
(In reply to Dão Gottwald [:dao] from comment #72)
> When I said dots I meant single dots, just to separate the actions.

Sounds like a good compromise to me.

(In reply to Dão Gottwald [:dao] from comment #73)
> >+#newtab-undo-container {
> >+  padding: 4px 3px;
> >+  border: 1px solid;
> >+  border-color: rgba(8,22,37,.12) rgba(8,22,37,.14) rgba(8,22,37,.16);
> >+  background-color: #f7f7f7;
> >+  font-size: 12px;
> >+  color: #525e69;
> >+}
> 
> Not sure why you're hardcoding the font size here.

12px is the page's font size. I'm going to put 'font-size: 75%' under the ':root' selector and remove all other occurrences.

> >+#newtab-undo-label {
> >+  margin-top: 0;
> >+  margin-bottom: 0;
> >+}
> >+
> >+.newtab-undo-button {
> >+  -moz-appearance: none;
> >+  color: -moz-nativehyperlinktext;
> 
> System text color on hard-coded background color is potentially illegible.

What would you suggest to do? The background colors of the page are fixed. Should we just specify hard-coded colors?
Comment 75 Dão Gottwald [:dao] 2013-01-21 14:46:38 PST
(In reply to Tim Taubert [:ttaubert] from comment #74)
> > System text color on hard-coded background color is potentially illegible.
> 
> What would you suggest to do? The background colors of the page are fixed.
> Should we just specify hard-coded colors?

You need to either use a system background color for #newtab-undo-container or hard-code the link text color.
Comment 76 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-01-23 08:53:23 PST
I went with hard-coded link colors for now. All other colors on the new tab page are hard-coded as well and if we want to adapt to the OS theme we'd need to overhaul the whole new tab page color scheme which we might decide to do in a follow-up.

https://hg.mozilla.org/integration/fx-team/rev/890154e8ba3e
Comment 77 Panos Astithas [:past] 2013-01-24 00:17:54 PST
https://hg.mozilla.org/mozilla-central/rev/890154e8ba3e
Comment 78 Siddhartha Dugar [:sdrocking] 2013-01-24 17:12:20 PST
This might me useful and low risk to have on Aurora.
Comment 79 sjc.erick 2013-01-29 03:54:24 PST
Cool. It's working on Nightly 21
Comment 80 Cornel Ionce [QA] (:cornel_ionce) 2013-01-30 05:39:37 PST
Working on Nightly. Verified on:

Ubuntu
User Agent: Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130129 Firefox/21.0
Build ID: 20130129030851

Mac OS X
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130130 Firefox/21.0
Build ID: 20130130030907

Windows
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130129 Firefox/21.0
Build ID: 20130129030851

Note You need to log in before you can comment on or make changes to this bug.