Closed Bug 729878 Opened 12 years ago Closed 12 years ago

[New Tab Page] Implement new layout

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 13

People

(Reporter: ttaubert, Assigned: ttaubert)

References

()

Details

Attachments

(3 files, 5 obsolete files)

      No description provided.
Blocks: 455553
Depends on: 640443
Blocks: 705001
Blocks: 719675
Blocks: 724089
Blocks: 724539
Blocks: 725393
Blocks: 724504
Blocks: 726628
Depends on: 731726
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.
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.
(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.
Depends on: 733875
Blocks: 722235
Attachment #604238 - Attachment is obsolete: true
Attached patch Part 2 - Theme-specific assets (obsolete) — Splinter Review
Attachment #604239 - Attachment is obsolete: true
Comment on attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation

Included in tomorrow's UX nightly.
Attachment #604242 - Flags: ui-review?(ux-review)
Attachment #604240 - Flags: review?(dietrich)
Attachment #604242 - Flags: review?(dietrich)
Attachment #604243 - Flags: review?(dietrich)
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.
Attachment #604242 - Flags: review?(dao)
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 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/
Attachment #604242 - Flags: review?(dietrich) → review+
Attachment #604240 - Flags: review?(dietrich) → review+
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.
Attachment #604243 - Flags: review?(dietrich) → review-
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?
Attachment #604242 - Flags: review?(dao) → review-
(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.
Attachment #604243 - Attachment is obsolete: true
Attachment #604895 - Flags: review?(dao)
(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.
Attachment #604242 - Attachment is obsolete: true
Attachment #604903 - Flags: ui-review?(ux-review)
Attachment #604903 - Flags: review?(dao)
Attachment #604242 - Flags: ui-review?(ux-review)
I see only half of the thumbnails, the others are empty. The ones I see have also a low image quality.
(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.
You shouldn't mess around with the flexible margins surrounding the grid. You could inject your widgets as grid siblings or position them absolutely.
(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.
Attachment #604895 - Flags: review?(dao) → review+
Comment on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

Looks good!
Attachment #604903 - Flags: ui-review?(ux-review) → ui-review+
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 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.
(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.
(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 on attachment 604903 [details] [diff] [review]
Part 1 - New layout implementation

Please attach the updated patch.
Attachment #604903 - Flags: review?(dao)
Attachment #604903 - Attachment is obsolete: true
Attachment #605164 - Flags: review?(dao)
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.
Attachment #605164 - Flags: review?(dao) → review+
(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.
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.
The layout has landed and will soon be merged. Please file separate bugs for any suggested improvements.
Depends on: 735294
Depends on: 735574
Depends on: 735984
Depends on: 735987
Depends on: 736028
Depends on: 736211
Blocks: 724217
No longer depends on: 736211
Whiteboard: [fixed-in-fx-team]
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.
Status: RESOLVED → VERIFIED
Depends on: 747735
Depends on: 752837
You need to log in before you can comment on or make changes to this bug.