Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 21

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sdrocking, Assigned: andreshm)

Tracking

Trunk
Firefox 21
Points:
---

Firefox Tracking Flags

(relnote-firefox 21+)

Details

Attachments

(4 attachments, 18 obsolete attachments)

559.31 KB, video/quicktime
Details
538.96 KB, image/png
Details
17.94 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
34.33 KB, image/png
shorlander
: ui-review+
Details
(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 455553
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

6 years ago
Blocks: 716543
No longer blocks: 716543
I really like how Chrome handles this. I concur, we should have something similar.
Keywords: uiwanted
Blocks: 729063
Created attachment 599190 [details] [diff] [review]
patch v1
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
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!
Attachment #599244 - Flags: feedback?(ux-review)

Comment 4

6 years ago
Nice screencast Tim :)
Created attachment 604217 [details] [diff] [review]
patch v2
Attachment #599190 - Attachment is obsolete: true
Attachment #599244 - Attachment is obsolete: true
Attachment #599244 - Flags: feedback?(ux-review)
Comment on attachment 604217 [details] [diff] [review]
patch v2

Included in tomorrow's UX nightly.
Attachment #604217 - Flags: ui-review?(ux-review)
Created attachment 604263 [details] [diff] [review]
patch v3
Attachment #604217 - Attachment is obsolete: true
Attachment #604217 - Flags: ui-review?(ux-review)
Attachment #604263 - Flags: ui-review?(ux-review)
No longer blocks: 729063
(Reporter)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
Tim, this worked well when we had it on UX. Why aren't you taking it to the next level?

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
Tim, this is needed in Nightly and Firefox 13 to make the New Tab page feature complete. Please land the patch ASAP.
(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..
(Assignee)

Updated

5 years ago
Assignee: ttaubert → andres
(Assignee)

Comment 13

5 years ago
Created attachment 617711 [details] [diff] [review]
Updated patch

Re-based patch and test.
Attachment #604263 - Attachment is obsolete: true
Attachment #604263 - Flags: ui-review?(ux-review)
Attachment #617711 - Flags: review?(ttaubert)
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.)
Attachment #617711 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 15

5 years ago
Created attachment 618818 [details]
Screencast
(Assignee)

Comment 16

5 years ago
Created attachment 618819 [details]
Screenshot
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.)
Attachment #618818 - Flags: ui-review?(shorlander)
Summary: New Tab Page does not provide an option to undo removing a site → [New Tab Page] provide an option to undo remove a site

Comment 18

5 years ago
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?
(Reporter)

Comment 19

5 years ago
(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.
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.
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.
Attachment #618818 - Flags: ui-review?(shorlander)
(Assignee)

Comment 22

5 years ago
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?
(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.
(Reporter)

Comment 24

5 years ago
(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.
(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.
(Assignee)

Comment 26

5 years ago
Created attachment 646590 [details] [diff] [review]
Patch v5
Attachment #617711 - Attachment is obsolete: true
Attachment #646590 - Flags: review?(ttaubert)
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.
Attachment #646590 - Flags: review?(ttaubert)
(Assignee)

Comment 28

5 years ago
(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?
(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?
(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
(Assignee)

Comment 31

5 years ago
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.
Attachment #646590 - Attachment is obsolete: true
Attachment #647984 - Flags: review?(ttaubert)
(Assignee)

Comment 32

5 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06
(Assignee)

Comment 33

5 years ago
Try finished with only one intermittent orange. 

https://tbpl.mozilla.org/?tree=Try&rev=172d91c5be06
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.
Attachment #647984 - Flags: review?(ttaubert)
(Assignee)

Comment 35

5 years ago
Created attachment 648772 [details] [diff] [review]
Patch v7

Updated patch with suggest changes. 
Local tests are running fine.
Attachment #647984 - Attachment is obsolete: true
Attachment #648772 - Flags: review?(ttaubert)
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.
Attachment #648772 - Flags: review?(ttaubert)
(Assignee)

Comment 37

5 years ago
Created attachment 649762 [details] [diff] [review]
Patch v8

Applied changes.
Attachment #648772 - Attachment is obsolete: true
Attachment #649762 - Flags: review?(ttaubert)
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.
Attachment #649762 - Flags: review?(ttaubert) → review+
(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.
(Reporter)

Comment 40

5 years ago
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

5 years ago
(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
Andres, can you please try to use the tab-close icon? I talked to shorlander and he said it might work!
(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
(Assignee)

Comment 44

5 years ago
Created attachment 654020 [details] [diff] [review]
Patch v9

Updated to use the suggested images.

Tim please confirm it looks good on linux.
Attachment #649762 - Attachment is obsolete: true
Attachment #654020 - Flags: review?(ttaubert)
(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.
You should also set the background-image to no-repeat, I think.
(Assignee)

Comment 47

5 years ago
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.
Attachment #654020 - Attachment is obsolete: true
Attachment #654020 - Flags: review?(ttaubert)
Attachment #654438 - Flags: review?(ttaubert)
(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
(Assignee)

Comment 49

5 years ago
Created attachment 654451 [details] [diff] [review]
Patch v10.1

Try with this one.
Attachment #654438 - Attachment is obsolete: true
Attachment #654438 - Flags: review?(ttaubert)
Attachment #654451 - Flags: review?(ttaubert)
(Assignee)

Comment 50

5 years ago
Created attachment 654470 [details] [diff] [review]
Patch v10

Minor fixes
Attachment #654451 - Attachment is obsolete: true
Attachment #654451 - Flags: review?(ttaubert)
(Assignee)

Comment 51

5 years ago
Created attachment 654473 [details]
Mac screenshot
Attachment #618819 - Attachment is obsolete: true
Attachment #654473 - Flags: ui-review?(ux-review)
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.
Keywords: uiwanted
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");
(Assignee)

Comment 54

5 years ago
Created attachment 654823 [details] [diff] [review]
Patch v11

Applied changes.
Attachment #654470 - Attachment is obsolete: true
Attachment #654823 - Flags: review?(ttaubert)
(Assignee)

Comment 55

5 years ago
Created attachment 654825 [details]
Mac screenshot
Attachment #654473 - Attachment is obsolete: true
Attachment #654473 - Flags: ui-review?(ux-review)
Attachment #654825 - Flags: ui-review?(ux-review)

Updated

5 years ago
Duplicate of this bug: 804892

Updated

5 years ago
Duplicate of this bug: 817936

Comment 58

5 years ago
can someone land this?
Created attachment 702210 [details] [diff] [review]
patch v11b

Rebased the patch.
Attachment #654823 - Attachment is obsolete: true
Attachment #654823 - Flags: review?(ttaubert)
Created attachment 702261 [details] [diff] [review]
patch v11c

Added a couple of style fixes for paddings, margins and the close buttons.
Attachment #702210 - Attachment is obsolete: true
Created attachment 702315 [details]
Screenshots for Windows, Mac, Linux (top to bottom)

Final screenshots from all three platforms.
Attachment #654825 - Attachment is obsolete: true
Attachment #654825 - Flags: ui-review?(ux-review)
Attachment #702315 - Flags: ui-review?(ux-review)
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.
Attachment #702261 - Flags: review+
Comment on attachment 702315 [details]
Screenshots for Windows, Mac, Linux (top to bottom)

Looks good, thank you!
Attachment #702315 - Flags: ui-review?(ux-review) → ui-review+
https://tbpl.mozilla.org/?tree=Try&rev=9ed67430f288
Why do the Undo and Restore All actions come with question marks?
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.
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

5 years ago
Would it be better to make them all buttons like the cross, and do without the question marks?
(Reporter)

Comment 69

5 years ago
(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?
I think we should just land this with dots rather than question marks.
Maybe just without any dots/ellipsis or question mark? I agree with your argument that it might indicate an indirect action otherwise.
When I said dots I meant single dots, just to separate the actions.
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.
(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?
(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.
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
https://hg.mozilla.org/mozilla-central/rev/890154e8ba3e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
(Reporter)

Comment 78

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

Comment 79

5 years ago
Cool. It's working on Nightly 21
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
Status: RESOLVED → VERIFIED

Updated

5 years ago
relnote-firefox: --- → 21+
You need to log in before you can comment on or make changes to this bug.