722234 - [New Tab Page] provide an option to undo remove a site

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Siddhartha Dugar [:sdrocking]]

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] (inactive)]

I really like how Chrome handles this. I concur, we should have something similar.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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]

Nice screencast Tim :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 604217 [details] [diff] [review]
patch v2

Included in tomorrow's UX nightly.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 8

[Siddhartha Dugar [:sdrocking]]

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

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

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

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]

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

Attachment: Updated patch

Re-based patch and test.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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

Attachment: Screencast

Comment 16

[Andres Hernandez [:andreshm]]

Attachment: Screenshot

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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

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

(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] (inactive)]

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

Attachment: 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]]

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] (inactive)]

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

(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] (inactive)]

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

Attachment: Patch v5

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

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

(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] (inactive)]

(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)]

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

Attachment: 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]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06

Comment 33

[Andres Hernandez [:andreshm]]

Try finished with only one intermittent orange. 

https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

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

Attachment: Patch v7

Updated patch with suggest changes. 
Local tests are running fine.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

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

Attachment: Patch v8

Applied changes.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

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

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

(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] (inactive)]

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

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

Attachment: Patch v9

Updated to use the suggested images.

Tim please confirm it looks good on linux.

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

(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] (inactive)]

You should also set the background-image to no-repeat, I think.

Comment 47

[Andres Hernandez [:andreshm]]

Attachment: 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] (inactive)]

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

Attachment: Patch v10.1

Try with this one.

Comment 50

[Andres Hernandez [:andreshm]]

Attachment: Patch v10

Minor fixes

Comment 51

[Andres Hernandez [:andreshm]]

Attachment: Mac screenshot

Comment 52

[Dão Gottwald [:dao]]

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] (inactive)]

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

Attachment: Patch v11

Applied changes.

Comment 55

[Andres Hernandez [:andreshm]]

Attachment: Mac screenshot

Comment 58

[henryfhchan]

can someone land this?

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v11b

Rebased the patch.

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v11c

Added a couple of style fixes for paddings, margins and the close buttons.

Comment 61

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshots for Windows, Mac, Linux (top to bottom)

Final screenshots from all three platforms.

Comment 62

[Tim Taubert [:ttaubert] (inactive)]

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

Comment on attachment 702315 [details]
Screenshots for Windows, Mac, Linux (top to bottom)

Looks good, thank you!

Comment 64

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=9ed67430f288

Comment 65

[Dão Gottwald [:dao]]

Why do the Undo and Restore All actions come with question marks?

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

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

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]

Would it be better to make them all buttons like the cross, and do without the question marks?

Comment 69

[Siddhartha Dugar [:sdrocking]]

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

I think we should just land this with dots rather than question marks.

Comment 71

[Tim Taubert [:ttaubert] (inactive)]

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

When I said dots I meant single dots, just to separate the actions.

Comment 73

[Dão Gottwald [:dao]]

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] (inactive)]

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

(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] (inactive)]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/890154e8ba3e

Comment 78

[Siddhartha Dugar [:sdrocking]]

This might me useful and low risk to have on Aurora.

Comment 79

[u452416]

Cool. It's working on Nightly 21

Comment 80

[Cornel Ionce [:noni] [Hubs QA]]

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