Last Comment Bug 729878 - [New Tab Page] Implement new layout
: [New Tab Page] Implement new layout
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 13
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
http://people.mozilla.com/~shorlander...
Depends on: 735294 640443 731726 733875 735574 735984 735987 736028 747735 752837 767289
Blocks: 455553 705001 719675 722235 724089 724217 724504 724539 725393 726628
  Show dependency treegraph
 
Reported: 2012-02-23 03:40 PST by Tim Taubert [:ttaubert]
Modified: 2012-08-06 23:41 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - New layout implementation (39.98 KB, patch)
2012-03-08 15:49 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Theme-specific styles and assets (82.07 KB, patch)
2012-03-08 15:49 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - Corrected newtab tests (2.84 KB, patch)
2012-03-08 15:50 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 1 - New layout implementation (56.00 KB, patch)
2012-03-08 15:57 PST, Tim Taubert [:ttaubert]
dietrich: review+
dao+bmo: review-
Details | Diff | Splinter Review
Part 2 - Theme-specific assets (66.04 KB, patch)
2012-03-08 15:57 PST, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
Part 2 - Theme-specific assets (33.74 KB, patch)
2012-03-12 05:12 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
Part 1 - New layout implementation (57.08 KB, patch)
2012-03-12 06:09 PDT, Tim Taubert [:ttaubert]
shorlander: ui‑review+
Details | Diff | Splinter Review
Part 1 - New layout implementation (57.40 KB, patch)
2012-03-12 15:30 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-02-23 03:40:39 PST

    
Comment 1 Guillaume C. [:ge3k0s] 2012-03-04 06:27:21 PST
I'm gonna be a bit out of topic, but I think the new tab page should be handled differently in private browsing mode, either disabled or rethought. Currently it shows the site without thumbnails and it's quite unappealing.
Comment 2 Tim Taubert [:ttaubert] 2012-03-05 00:57:07 PST
Please don't just comment on bugs that sound related but rather search for your specific issue or file a new bug. You want bug 731157.
Comment 3 Guillaume C. [:ge3k0s] 2012-03-05 03:43:15 PST
(In reply to Tim Taubert [:ttaubert] from comment #2)
> Please don't just comment on bugs that sound related but rather search for
> your specific issue or file a new bug. You want bug 731157.
Thanks for the answer. I must have missed this one.
Comment 4 Tim Taubert [:ttaubert] 2012-03-08 15:49:16 PST
Created attachment 604238 [details] [diff] [review]
Part 1 - New layout implementation
Comment 5 Tim Taubert [:ttaubert] 2012-03-08 15:49:55 PST
Created attachment 604239 [details] [diff] [review]
Part 2 - Theme-specific styles and assets
Comment 6 Tim Taubert [:ttaubert] 2012-03-08 15:50:15 PST
Created attachment 604240 [details] [diff] [review]
Part 3 - Corrected newtab tests
Comment 7 Tim Taubert [:ttaubert] 2012-03-08 15:57:09 PST
Created attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation
Comment 8 Tim Taubert [:ttaubert] 2012-03-08 15:57:40 PST
Created attachment 604243 [details] [diff] [review]
Part 2 - Theme-specific assets
Comment 9 Tim Taubert [:ttaubert] 2012-03-08 16:37:34 PST
Comment on attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation

Included in tomorrow's UX nightly.
Comment 10 Tim Taubert [:ttaubert] 2012-03-08 17:27:52 PST
Comment on attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation

Asking Dão for review as well to have him weigh in with especially his CSS knowledge.
Comment 11 Siddhartha Dugar [:sdrocking] 2012-03-08 23:15:53 PST
Nice work!

The page title container is taller compared to the mockups. Also it needs a top border.
Site favicons should be shown somewhere, possibly within the title container.
Comment 12 Dietrich Ayala (:dietrich) 2012-03-09 08:31:00 PST
Comment on attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation

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

r=me

::: browser/base/content/newtab/dropTargetShim.js
@@ +40,5 @@
> +    switch (aEvent.type) {
> +      case "dragstart":
> +        this._start(aEvent);
> +        break;
> +      case "drag":

is there a point in having this, since you don't listen for it? could remove the drag stuff, really.

::: browser/base/content/newtab/transformations.js
@@ +260,5 @@
> +
> +  /**
> +   * Checks whether a site is currently frozen.
> +   * @param aSite The site to check.
> +   * @return Whether the given site if frozen.

s/if/is/
Comment 13 Dão Gottwald [:dao] 2012-03-12 04:40:07 PDT
Comment on attachment 604243 [details] [diff] [review]
Part 2 - Theme-specific assets

>diff --git a/browser/themes/gnomestripe/newtab/noise.png b/browser/themes/gnomestripe/newtab/noise.png
>new file mode 100644

This file seems unreasonably large. There's a similar image here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/devtools/background-noise-toolbar.png
It's a bit lighter, so not quite the same, but in principle you should be able to achieve about the same size here.
Comment 14 Dão Gottwald [:dao] 2012-03-12 05:04:00 PDT
Comment on attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation

>   _createSiteFragment: function Grid_createSiteFragment() {
>     let site = document.createElementNS(HTML_NAMESPACE, "a");
>-    site.classList.add("site");
>+    site.classList.add("newtab-site");
>     site.setAttribute("draggable", "true");
> 
>     // Create the site's inner HTML code.
>     site.innerHTML =
>-      '<img class="site-img" width="' + THUMB_WIDTH +'" ' +
>-      ' height="' + THUMB_HEIGHT + '" alt=""/>' +
>-      '<span class="site-title"/>' +
>-      '<span class="site-strip">' +
>-      '  <input class="button strip-button strip-button-pin" type="button"' +
>-      '   tabindex="-1" title="' + newTabString("pin") + '"/>' +
>-      '  <input class="button strip-button strip-button-block" type="button"' +
>-      '   tabindex="-1" title="' + newTabString("block") + '"/>' +
>-      '</span>';
>+      '<span class="newtab-thumbnail"/>' +
>+      '<input type="button" title="' + newTabString("pin") + '"' +
>+      '       class="newtab-control newtab-control-pin"/>' +
>+      '<input type="button" title="' + newTabString("block") + '"' +
>+      '       class="newtab-control newtab-control-block"/>' +
>+      '<span class="newtab-title"/>';

Structurally (though not visually) the buttons should be siblings rather than children of the link since clicking them won't activate the link. Currently the status panel shows the link target for these buttons.

>+#newtab-margin-left,
>+#newtab-margin-right {
>+  min-width: 40px;
>+  max-width: 300px;
>+  -moz-box-flex: 1;
>+}

>+  <div id="newtab-scrollbox" title=" ">
>+
>+    <div id="newtab-vertical-margin">
>+      <div id="newtab-margin-top"/>
>+
>+      <div id="newtab-horizontal-margin">
>+        <div id="newtab-margin-left"/>

>+        <div id="newtab-margin-right"/>
>       </div>

For RTL, newtab-margin-left is right and newtab-margin-right is left. You don't actually need two distinct ids, you can use one class instead.

>--- a/browser/locales/en-US/chrome/browser/newTab.properties
>+++ b/browser/locales/en-US/chrome/browser/newTab.properties
>@@ -1,3 +1,5 @@
> newtab.pin=Pin this site at its current position
> newtab.unpin=Unpin this site
> newtab.block=Remove this site
>+newtab.show=Show the New Tab Page
>+newtab.hide=Hide the New Tab Page

Tooltips shouldn't use title capitalization. "New Tab Page" probably doesn't count as a proper name either...

Hovering between the cells erroneously shows the "Hide the New Tab Page" tooltip.

Why do you still show the gray background with the page hidden?
Comment 15 Tim Taubert [:ttaubert] 2012-03-12 05:12:49 PDT
Created attachment 604895 [details] [diff] [review]
Part 2 - Theme-specific assets

(In reply to Dão Gottwald [:dao] from comment #13)
> This file seems unreasonably large. There's a similar image here:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/
> devtools/background-noise-toolbar.png
> It's a bit lighter, so not quite the same, but in principle you should be
> able to achieve about the same size here.

You're right that's pretty large. I cropped the image to 48x48 and it went from 11kb down to 2.1kb. Cropping it even smaller resulted in a too noisy background.
Comment 16 Tim Taubert [:ttaubert] 2012-03-12 06:09:35 PDT
Created attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

(In reply to Dão Gottwald [:dao] from comment #14)
> Structurally (though not visually) the buttons should be siblings rather
> than children of the link since clicking them won't activate the link.
> Currently the status panel shows the link target for these buttons.

Yes, that's a very good point, done.

> For RTL, newtab-margin-left is right and newtab-margin-right is left. You
> don't actually need two distinct ids, you can use one class instead.

Right, makes sense.

> >+newtab.show=Show the New Tab Page
> >+newtab.hide=Hide the New Tab Page
> 
> Tooltips shouldn't use title capitalization. "New Tab Page" probably doesn't
> count as a proper name either...

Yeah, I got a little bit too used to calling it that internally.

> Hovering between the cells erroneously shows the "Hide the New Tab Page"
> tooltip.

Fixed, I accidentally set the title attribute on the grid instead of the toggle button.

> Why do you still show the gray background with the page hidden?

Wasn't intentional, removed.
Comment 17 Guillaume C. [:ge3k0s] 2012-03-12 06:22:39 PDT
I see only half of the thumbnails, the others are empty. The ones I see have also a low image quality.
Comment 18 Tiger.711 2012-03-12 06:43:44 PDT
(In reply to Tim Taubert [:ttaubert] from comment #16)
> > For RTL, newtab-margin-left is right and newtab-margin-right is left. You
> > don't actually need two distinct ids, you can use one class instead.
> 
> Right, makes sense.
Let it be a different ID's.
Then we can write jetpaks for a new tab, which will be on the left or right side of the Dial's. For example, I thinking about the clock and the simple weather forecast, which will be on left side.
Comment 19 Tim Taubert [:ttaubert] 2012-03-12 07:00:05 PDT
You shouldn't mess around with the flexible margins surrounding the grid. You could inject your widgets as grid siblings or position them absolutely.
Comment 20 Tim Taubert [:ttaubert] 2012-03-12 07:02:45 PDT
(In reply to Guillaume C. [:GE3K0S] from comment #17)
> I see only half of the thumbnails, the others are empty. The ones I see have
> also a low image quality.

There are lots of bugs wrt to the thumbnails and their quality. This bug is about the new layout, not especially about the thumbnails contained in the page.
Comment 21 Stephen Horlander [:shorlander] 2012-03-12 13:33:07 PDT
Comment on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

Looks good!
Comment 22 Dão Gottwald [:dao] 2012-03-12 13:40:38 PDT
Comment on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

>+<xul:window id="newtab-window" xmlns="http://www.w3.org/1999/xhtml"
>             xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>             disablefastfind="true" title="&newtab.pageTitle;">

xul:disablefastfind and xul:title

>+  <div id="newtab-scrollbox" title=" ">

remove title=" "

The gray background is still there with the page disabled.
Comment 23 Dão Gottwald [:dao] 2012-03-12 13:46:28 PDT
Comment on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

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

>+  font-family: Lucida Grande, Segoe UI, Helvetica, Calibra;

This list doesn't make sense on Linux.
Comment 24 Dão Gottwald [:dao] 2012-03-12 13:47:53 PDT
(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 604903 [details] [diff] [review]
> Part 1 - New layout implementation
> 
> >--- a/browser/themes/gnomestripe/newtab/newTab.css
> >+++ b/browser/themes/gnomestripe/newtab/newTab.css
> 
> >+  font-family: Lucida Grande, Segoe UI, Helvetica, Calibra;
> 
> This list doesn't make sense on Linux.

I think the sanest thing is to completely remove this font-family declaration.
Comment 25 Tim Taubert [:ttaubert] 2012-03-12 14:55:30 PDT
(In reply to Dão Gottwald [:dao] from comment #22)
> xul:disablefastfind and xul:title
> 
> >+  <div id="newtab-scrollbox" title=" ">
> 
> remove title=" "

Yeah, now I get why there was a tooltip everywhere. Thanks.

> The gray background is still there with the page disabled.

Fixed.

(In reply to Dão Gottwald [:dao] from comment #24)
> > >+  font-family: Lucida Grande, Segoe UI, Helvetica, Calibra;
> > 
> > This list doesn't make sense on Linux.
> 
> I think the sanest thing is to completely remove this font-family
> declaration.

Agreed, removed.
Comment 26 Dão Gottwald [:dao] 2012-03-12 15:24:56 PDT
Comment on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

Please attach the updated patch.
Comment 27 Tim Taubert [:ttaubert] 2012-03-12 15:30:09 PDT
Created attachment 605164 [details] [diff] [review]
Part 1 - New layout implementation
Comment 28 Dão Gottwald [:dao] 2012-03-12 15:36:11 PDT
Comment on attachment 605164 [details] [diff] [review]
Part 1 - New layout implementation

>--- a/browser/base/content/newtab/newTab.css
>+++ b/browser/base/content/newtab/newTab.css
>@@ -1,173 +1,188 @@
> :root {
>   -moz-appearance: none;
>+  -moz-user-focus: normal;
>+  background-color: transparent;
> }

Please move "-moz-appearance: none" and "background-color: transparent" to the theme files.
Comment 29 Tim Taubert [:ttaubert] 2012-03-12 19:13:01 PDT
(In reply to Dão Gottwald [:dao] from comment #28)
> > :root {
> >   -moz-appearance: none;
> >+  -moz-user-focus: normal;
> >+  background-color: transparent;
> > }
> 
> Please move "-moz-appearance: none" and "background-color: transparent" to
> the theme files.

Done.
Comment 31 anthonydam 2012-03-13 03:11:25 PDT
Few suggestions :
-Add a favicon in front of website name (like in the bookmark, it's here for a reason).
-Remove the gray dots in the background. White backgound + gray empty box is much more coherent with Windows.
Comment 32 Tim Taubert [:ttaubert] 2012-03-13 03:15:34 PDT
The layout has landed and will soon be merged. Please file separate bugs for any suggested improvements.
Comment 34 patrickjdempsey 2012-03-16 01:09:54 PDT
*** Bug 724217 has been marked as a duplicate of this bug. ***
Comment 35 Virgil Dicu [:virgil] [QA] 2012-04-10 06:44:03 PDT
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120409 Firefox/13.0a2
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120409 Firefox/14.0a1

new tab layout implemented and working on all platforms. Marking this as verified.

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