[New Tab Page] Implement new layout

VERIFIED FIXED in Firefox 13

Status

()

Firefox
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

Comment hidden (empty)
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
Created attachment 604238 [details] [diff] [review]
Part 1 - New layout implementation
Created attachment 604239 [details] [diff] [review]
Part 2 - Theme-specific styles and assets
Created attachment 604240 [details] [diff] [review]
Part 3 - Corrected newtab tests
Created attachment 604242 [details] [diff] [review]
Part 1 - New layout implementation
Attachment #604238 - Attachment is obsolete: true
Created attachment 604243 [details] [diff] [review]
Part 2 - Theme-specific assets
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-
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.
Attachment #604243 - Attachment is obsolete: true
Attachment #604895 - Flags: review?(dao)
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.
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.

Comment 18

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

Updated

6 years ago
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)
Created attachment 605164 [details] [diff] [review]
Part 1 - New layout implementation
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.
https://hg.mozilla.org/integration/fx-team/rev/cdd0ecf3851b
https://hg.mozilla.org/integration/fx-team/rev/e2a9031828b0
https://hg.mozilla.org/integration/fx-team/rev/b2a4560c0af0
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13

Comment 31

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/cdd0ecf3851b
https://hg.mozilla.org/mozilla-central/rev/e2a9031828b0
https://hg.mozilla.org/mozilla-central/rev/b2a4560c0af0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 735294
Depends on: 735574
Depends on: 735984
Depends on: 735987
Depends on: 736028
Depends on: 736211

Updated

6 years ago
Duplicate of this bug: 724217
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: 767289

Updated

5 years ago
Depends on: 752837
You need to log in before you can comment on or make changes to this bug.