Closed Bug 1322738 Opened 3 years ago Closed 3 years ago

Update about:newtab tile shapes / organization to match activity stream

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: Gijs, Assigned: dao)

References

(Blocks 2 open bugs)

Details

(Whiteboard: prefs: browser.newtabpage.compact = true, browser.newtabpage.rows = 2, browser.newtabpage.columns = 6)

Attachments

(4 files, 4 obsolete files)

ni Verdi for spec
Flags: needinfo?(mverdi)
Attached image NewTab Spec@1x.png (obsolete) —
Flags: needinfo?(mverdi)
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
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)
Attached patch WIP 2 (obsolete) — Splinter Review
Attachment #8821803 - Attachment is obsolete: true
Whiteboard: prefs: browser.newtabpage.compact = true, browser.newtabpage.rows = 2, browser.newtabpage.columns = 6
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.
(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?)
(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
(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)
(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.
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #8822556 - Flags: review?(dolske) → review?(adw)
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+
(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.
Attachment #8822556 - Flags: review?(adw)
Attachment #8822556 - Attachment is obsolete: true
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8883a87c6dc
Implement compact about:newtab tiles styling. r=gijs
https://hg.mozilla.org/mozilla-central/rev/c8883a87c6dc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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 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+
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)
Depends on: 1329941
Flags: needinfo?(gwimberly)
Depends on: 1330303
Depends on: 1330611
Blocks: 1331356
Status: RESOLVED → VERIFIED
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.
Blocks: 1354046
Depends on: 1350205
Duplicate of this bug: 1351611
You need to log in before you can comment on or make changes to this bug.