Closed
Bug 729878
Opened 12 years ago
Closed 12 years ago
[New Tab Page] Implement new layout
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 13
People
(Reporter: ttaubert, Assigned: ttaubert)
References
()
Details
Attachments
(3 files, 5 obsolete files)
2.84 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
33.74 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
57.40 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #604238 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #604239 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #604240 -
Flags: review?(dietrich)
Assignee | ||
Updated•12 years ago
|
Attachment #604242 -
Flags: review?(dietrich)
Assignee | ||
Updated•12 years ago
|
Attachment #604243 -
Flags: review?(dietrich)
Assignee | ||
Comment 10•12 years ago
|
||
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)
Comment 11•12 years ago
|
||
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•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #604240 -
Flags: review?(dietrich) → review+
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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-
Assignee | ||
Comment 15•12 years ago
|
||
(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)
Assignee | ||
Comment 16•12 years ago
|
||
(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)
Comment 17•12 years ago
|
||
I see only half of the thumbnails, the others are empty. The ones I see have also a low image quality.
Comment 18•12 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.
Assignee | ||
Comment 19•12 years ago
|
||
You shouldn't mess around with the flexible margins surrounding the grid. You could inject your widgets as grid siblings or position them absolutely.
Assignee | ||
Comment 20•12 years ago
|
||
(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•12 years ago
|
Attachment #604895 -
Flags: review?(dao) → review+
Comment 21•12 years ago
|
||
Comment on attachment 604903 [details] [diff] [review] Part 1 - New layout implementation Looks good!
Attachment #604903 -
Flags: ui-review?(ux-review) → ui-review+
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 25•12 years ago
|
||
(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•12 years ago
|
||
Comment on attachment 604903 [details] [diff] [review] Part 1 - New layout implementation Please attach the updated patch.
Attachment #604903 -
Flags: review?(dao)
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #604903 -
Attachment is obsolete: true
Attachment #605164 -
Flags: review?(dao)
Comment 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
(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.
Assignee | ||
Comment 30•12 years ago
|
||
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•12 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.
Assignee | ||
Comment 32•12 years ago
|
||
The layout has landed and will soon be merged. Please file separate bugs for any suggested improvements.
Assignee | ||
Comment 33•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 35•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•