Closed
Bug 1322738
Opened 7 years ago
Closed 7 years ago
Update about:newtab tile shapes / organization to match activity stream
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
VERIFIED
FIXED
Firefox 53
People
(Reporter: Gijs, Assigned: dao)
References
(Blocks 1 open bug)
Details
(Whiteboard: prefs: browser.newtabpage.compact = true, browser.newtabpage.rows = 2, browser.newtabpage.columns = 6)
Attachments
(4 files, 4 obsolete files)
103.83 KB,
image/png
|
Details | |
110.93 KB,
image/png
|
Details | |
73.93 KB,
image/png
|
Details | |
14.40 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
ni Verdi for spec
Flags: needinfo?(mverdi)
Comment 1•7 years ago
|
||
Flags: needinfo?(mverdi)
Assignee | ||
Comment 2•7 years ago
|
||
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
11px for the titles significantly undercuts OS default text sizes. I think that's going to be an accessibility issue for visually impaired users. My current patch uses 12px but this also reduces the number of characters we can display and also changes the visual balance within the relatively small tiles.
Flags: needinfo?(mverdi)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8821803 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Whiteboard: prefs: browser.newtabpage.compact = true, browser.newtabpage.rows = 2, browser.newtabpage.columns = 6
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
I think we should probably make the tiles a little wider. This would give us a better aspect ratio for screenshots (the content area is hardly ever square) and more space for the title.
Assignee | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #7) > Created attachment 8822058 [details] > screenshot with 10 px wider tiles I guess the heavily-cropped tile contents are because this is still using the current default-tiles images, which assumed a larger tile? Does the work in bug 1322731 yield better looking tiles? (Looks like the upper-left one is an actual rendered screenshot, so that's more-or-less what real sites will look like?)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #8) > (In reply to Dão Gottwald [:dao] from comment #7) > > Created attachment 8822058 [details] > > screenshot with 10 px wider tiles > > I guess the heavily-cropped tile contents are because this is still using > the current default-tiles images, which assumed a larger tile? Not just larger but also wider so they can't be made fit. > Does the work in bug 1322731 yield better looking tiles? Yes. > (Looks like the upper-left one is > an actual rendered screenshot, so that's more-or-less what real sites will > look like?) No, this is https://tiles-cloudfront.cdn.mozilla.net/images/a15c0403863847aef5943a6272cd992335f330f9.59611.png
Comment 10•7 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > 11px for the titles significantly undercuts OS default text sizes. I think > that's going to be an accessibility issue for visually impaired users. My > current patch uses 12px but this also reduces the number of characters we > can display and also changes the visual balance within the relatively small > tiles. Got it. Let's make the tiles larger - 110x110 with 48x48 favicons and 12px text (attaching new spec). > I think we should probably make the tiles a little wider. This would give us > a better aspect ratio for screenshots (the content area is hardly ever > square) and more space for the title. From what I can observe, it seems the thumbnails are not necessarily made in the aspect ratio of the content area used to view the site. So I'm guessing/hoping they will still work with these tiles. Which leads me to a question: With Bug 1322737, when will we use screenshots?
Attachment #8818725 -
Attachment is obsolete: true
Flags: needinfo?(mverdi)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Verdi [:verdi] from comment #10) > > I think we should probably make the tiles a little wider. This would give us > > a better aspect ratio for screenshots (the content area is hardly ever > > square) and more space for the title. > > From what I can observe, it seems the thumbnails are not necessarily made in > the aspect ratio of the content area used to view the site. So I'm > guessing/hoping they will still work with these tiles. That's true, but the more the aspect radio differs from the content area, the more awkward the screenshots will look. > Which leads me to a question: With Bug 1322737, when will we use screenshots? Whenever we can get screenshots from the PageThumbs or BackgroundPageThumbs modules. That's most of the time with a populated history.
Assignee | ||
Comment 12•7 years ago
|
||
I believe neither adw nor Gijs will be working until after new year's, so asking dolske for review.
Attachment #8821822 -
Attachment is obsolete: true
Attachment #8822556 -
Flags: review?(dolske)
Assignee | ||
Updated•7 years ago
|
Attachment #8822556 -
Flags: review?(dolske) → review?(adw)
Reporter | ||
Comment 13•7 years ago
|
||
Comment on attachment 8822556 [details] [diff] [review] patch Review of attachment 8822556 [details] [diff] [review]: ----------------------------------------------------------------- r=me, in case that's helpful. ::: browser/themes/shared/newtab/newTab.inc.css @@ +186,5 @@ > } > > /* TITLES */ > + > +.newtab-sponsored, Why is this rule (and the next one) applying to newtab-sponsored? I couldn't find a corresponding rule previous to this one, and there seem to be plenty of rules relating to it in browser/base/content/newtab/newTab.css that this is now overriding. I don't think it matters too much because I don't think we're doing sponsored tiles anymore, but even so... @@ +234,1 @@ > color: white; color: is always (ie both in compact and non-compact) white, as is border-top-color, can we just keep those in a separate rule that isn't specific to the body having or not having the 'compact' class?
Attachment #8822556 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to :Gijs from comment #13) > Comment on attachment 8822556 [details] [diff] [review] > patch > > Review of attachment 8822556 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, in case that's helpful. Thanks! > ::: browser/themes/shared/newtab/newTab.inc.css > @@ +186,5 @@ > > } > > > > /* TITLES */ > > + > > +.newtab-sponsored, > > Why is this rule (and the next one) applying to newtab-sponsored? I couldn't > find a corresponding rule previous to this one, and there seem to be plenty > of rules relating to it in browser/base/content/newtab/newTab.css that this > is now overriding. I don't think it matters too much because I don't think > we're doing sponsored tiles anymore, but even so... I moved this from browser/base/content/newtab/newTab.css but missed that there was another rule overriding all of this. I'll remove newtab-sponsored from my rule. > @@ +234,1 @@ > > color: white; > > color: is always (ie both in compact and non-compact) white, as is > border-top-color, can we just keep those in a separate rule that isn't > specific to the body having or not having the 'compact' class? I don't think another rule is necessarily an improvement here. This is also temporary, i.e. we'll want to remove either the compact or the non-compact variant in the not too distant future.
Assignee | ||
Updated•7 years ago
|
Attachment #8822556 -
Flags: review?(adw)
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8822556 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8883a87c6dc Implement compact about:newtab tiles styling. r=gijs
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8883a87c6dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee | ||
Comment 18•7 years ago
|
||
Comment on attachment 8823821 [details] [diff] [review] patch v2, review comments addressed Approval Request Comment [Feature/Bug causing the regression]: bug 1322718 comment 1 [User impact if declined]: users would get the old about:newtab layout, not what we want to test in the funnelcake build [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: QA will need to be part of the funnelcake process. This change will need to be part of that. [List of other uplifts needed for the feature/fix]: [Is the change risky?]: low risk [Why is the change risky/not risky?]: The CSS changes / additions are straightforward and for the most part only take effect with browser.newtabpage.compact = true. There's a bug fix in grid.js that might affect the old layout (browser.newtabpage.compact = false), but this should be for the better. [String changes made/needed]: /
Attachment #8823821 -
Flags: approval-mozilla-beta?
Attachment #8823821 -
Flags: approval-mozilla-aurora?
Comment 19•7 years ago
|
||
Comment on attachment 8823821 [details] [diff] [review] patch v2, review comments addressed New layout for newtab, part of funnelcake onboarding build.
Attachment #8823821 -
Flags: approval-mozilla-beta?
Attachment #8823821 -
Flags: approval-mozilla-beta+
Attachment #8823821 -
Flags: approval-mozilla-aurora?
Attachment #8823821 -
Flags: approval-mozilla-aurora+
Comment 20•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6329a43064da
status-firefox52:
--- → fixed
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/24cf293eb969
status-firefox51:
--- → fixed
Comment 22•7 years ago
|
||
Setting the flag for verification, specifications available in Comment 1, with slight changes mentioned for it in Comment 10.
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Comment 23•7 years ago
|
||
I can confirm this issue is fixed, I verified using Firefox 52.0 and 53.0b2 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.11.
You need to log in
before you can comment on or make changes to this bug.
Description
•