657038 - [Snippets] Add graphics and tracking tags to default snippets

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Laura Forrest]

Tracking tags will allow us to get a better idea about how often our default content is shown. Default snippets show when the snippet service is down or when no current program snippets have been pushed to a version/channel.

Currently there are two default snippets that are not served out through the snippet service but through the product itself. 

Let's also add graphics while we're under the hood. 

1. It's easy to customize your Firefox exactly the way you want it. Choose from thousands of add-ons.

Link:
https://addons.mozilla.org/?WT.mc_ID=default1

Graphic: addons.png

2. Thanks for choosing Firefox! To get the most out of your browser, learn more about the latest features.

Link:
http://www.mozilla.com/en-US/firefox/features/?WT.mc_ID=default2

Graphic:
Laura to get and attach here. Current feature icon uses "4" which won't last long. Assigning to myself until then.

Comment 1

[Laura Forrest]

Attachment: addons.png

Comment 2

[Laura Forrest]

Features Graphic:
https://bug657039.bugzilla.mozilla.org/attachment.cgi?id=533344

Comment 3

[Les Orchard [:lorchard]]

Fwiw: this is not a snippet service bug.  Default snippets are built into firefox itself,  for display when the service is unavailable. Changes would need to ship in a future browser release,  and would not apply to previous versions

I think marco bonardo did the work for fx4 originally,  not sure who'd do it now.

Comment 4

[Marco Bonardo [:mak]]

I could do this, but it's pretty trivial, I think anyone with basic Web knowledge may do it.

It's matter of changing the snippets here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.xhtml#90
urls here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.js#143
and add some css for icons in: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.css

I you can't find some volunteering for it, just move the bug to Firefox/General and assign it to me, otherwise I could review changes.

Comment 5

[Laura Forrest]

Thanks Marco! I'm not sure who else to ask, so it's fine if you give this a go. I appreciate the help.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

My main concern is that we are going to add 9KB (uncompressed size of the 2 images after pngcrush -brute) to omni.jar. Where possible we may evaluate to re-use some image already bundled (the puzzle piece for add-ons snippet and the Firefox icon for the features snippet may be candidates).  Gavin, what do you think about this small size hit?
Apart this, the patch is mostly a challenge to keep both icon and text vertically centered without regressing style for the remote snippets. I tried the result with different text sizes, window sizes and rtl and it worked fine.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 536498 [details] [diff] [review]
patch v1.0

>diff --git a/browser/base/content/aboutHome.css b/browser/base/content/aboutHome.css

>+#defaultSnippet1,
>+#defaultSnippet2 {

>+body[dir=rtl] #defaultSnippet1,
>+body[dir=rtl] #defaultSnippet2 {

You could use "#defaultSnippets > span" for these, right? Dao is probably a better reviewer for this styling part.

The other parts look fine. I don't think the size of the additional images is worth worrying about.

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #7)
> >+#defaultSnippet1,
> >+#defaultSnippet2 {
> 
> >+body[dir=rtl] #defaultSnippet1,
> >+body[dir=rtl] #defaultSnippet2 {
> 
> You could use "#defaultSnippets > span" for these, right?

I move these elements from #defaultSnippets to #snippets in js code when no snippets are found, so I can't do that

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Can't you use #snippets > span, then? Do they really need to be styled in both containers?

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> Can't you use #snippets > span, then? Do they really need to be styled in
> both containers?

yes, but I can't tell if remote snippets have a span in the same position, and I'd end up styling them with default snippets style.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

ah, I see. could add another class on the default snippets and use that instead of "span". guess it doesn't matter, forget I said anything :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 536498 [details] [diff] [review]
patch v1.0

(In reply to comment #11)
> could add another class on the default snippets and use that
> instead of "span".

Yes, but then I add both id and class to each one; since default snippets are a few (2 actually) that seemed useless noise, compared to just a plain enumeration. Sure for >3 snippets it may make more sense.

Let's see what Dao thinks about that.

Comment 13

[Dão Gottwald [::dao]]

Comment on attachment 536498 [details] [diff] [review]
patch v1.0

>+#defaultSnippet1,
>+#defaultSnippet2 {
>+  display: inline-block;
>+  text-align: start;
>+  background-position: left center;
>+  background-repeat: no-repeat;
>+  /* Make space for the icon. */
>+  -moz-padding-start: 54px;
>+  /* Reset padding to the icon's height, to avoid cutting it while keeping text centered */
>+  margin-top: -14px;
>+  margin-bottom: -14px;
>+  padding-top: 25px;
>+  padding-bottom: 25px;
>+}
>+
>+body[dir=rtl] #defaultSnippet1,
>+body[dir=rtl] #defaultSnippet2 {
>+  background-position: right center;
>+}
>+
>+#defaultSnippet1 {
>+  background-image: url("chrome://browser/content/aboutHome-snippet1.png");
>+}
>+#defaultSnippet2 {
>+  background-image: url("chrome://browser/content/aboutHome-snippet2.png");
>+}

Seems like something like this would be simpler:

#defaultSnippet1,
#defaultSnippet2 {
  display: table-row;
  text-align: start;
}

#defaultSnippet1::before,
#defaultSnippet2::before {
  display: table-cell;
  vertical-align: middle;
  -moz-padding-end: 1em;
}

#defaultSnippet1::before {
  content: url("chrome://browser/content/aboutHome-snippet1.png");
}
#defaultSnippet2::before {
  content: url("chrome://browser/content/aboutHome-snippet2.png");
}

Comment 14

[Marco Bonardo [:mak]]

Thanks, I'll try this, all my attempts with ::before failed due to bad vertical align (either the icon or the text were not middle), but I didn't explicitly try with table-row.

Comment 15

[Marco Bonardo [:mak]]

Attachment: screenshot (forgive colors)

So, it works about the same as I remembered; while the icon is vertically centered, the text is vertically aligned to the start of the icon, not centered.
My hack was exactly to workaround this, but I admit it's a bit verbose and scary.
If we are fine with ignoring that, I can proceed with the suggested styling, otherwise it will need some teaking.

Comment 16

[Laura Forrest]

(In reply to comment #15)
> Created attachment 539216 [details]
> screenshot (forgive colors)
> 
> So, it works about the same as I remembered; while the icon is vertically
> centered, the text is vertically aligned to the start of the icon, not
> centered.
> My hack was exactly to workaround this, but I admit it's a bit verbose and
> scary.
> If we are fine with ignoring that, I can proceed with the suggested styling,
> otherwise it will need some teaking.

That looks fine to me. Cc-ing John Slater here for his stamp of approval. 

One thing to remember here is that since these act as default snippets they should only show when no other content is available, meaning, ideally never. Still, we want them to look good enough in the cases that they do show since this page gets millions of page views/day.

Comment 17

[Marco Bonardo [:mak]]

FWIW, this is another alternative, that mixes the 2 approaches, it saves the rtl rules, but I don't think is that much cleaner.

#defaultSnippet1,
#defaultSnippet2 {
  display: table-cell;
  text-align: start;
  padding: 6px 0;
  -moz-padding-start: 54px;
}

#defaultSnippet1::before,
#defaultSnippet2::before {
    content: "";
    -moz-margin-start: -54px;
    margin-top: -20px;
    position: absolute;
    height: 100%;
    width: 40px;
}

#defaultSnippet1::before {
  background: url("chrome://browser/content/aboutHome-snippet1.png") no-repeat center;
}
#defaultSnippet2::before {
  background: url("chrome://browser/content/aboutHome-snippet2.png") no-repeat center;
}

Comment 18

[John Slater]

If it's possible to align the text so it's vertically centered with the icon that would be even better, but if that's too hard then comment #15 looks good to me.

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #18)
> If it's possible to align the text so it's vertically centered with the icon
> that would be even better, but if that's too hard then comment #15 looks
> good to me.

The problem is that the generated style is a bit cumbersome, see the quoted part of comment 13 or comment 17.  So it mostly depends whether Dao is fine taking one of my two approaches, or has ideas to align text in his suggested style, or just wants to proceed with not-centered text.

Comment 20

[Dão Gottwald [::dao]]

Please take the simpler approach and file a followup bug on tweaking the text alignment, I'll look into it there.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.1 (no-text-valign)

I've been able to further reduce the total size of the pngs to 5153B, that is pretty awesome.
Will file the follow-up to investigate text valign now.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to comment #0)
> Tracking tags will allow us to get a better idea about how often our default
> content is shown. Default snippets show when the snippet service is down or
> when no current program snippets have been pushed to a version/channel.

This made me raise my eyebrow, but it looks like the "tracking" is that clicking the default snippet's link goes to a URL that lets us known it was from the snippet. So I think that's fine.

But if you want more precise data on how often we fallback to the default snippets, the telemetry project (bug 585196) might be a good fit. EG, about:home could keep counters for how many times a default/non-default snippet was shown.

Comment 23

[Justin Dolske [:Dolske]]

(to be clear, that would be a followup, and is more for future data since telemetry isn't yet finished on trunk)

Comment 24

[Laura Forrest]

(In reply to comment #22)
> (In reply to comment #0)
> > Tracking tags will allow us to get a better idea about how often our default
> > content is shown. Default snippets show when the snippet service is down or
> > when no current program snippets have been pushed to a version/channel.
> 
> This made me raise my eyebrow, but it looks like the "tracking" is that
> clicking the default snippet's link goes to a URL that lets us known it was
> from the snippet. So I think that's fine.

Exactly; very innocuous. 

> But if you want more precise data on how often we fallback to the default
> snippets, the telemetry project (bug 585196) might be a good fit. EG,
> about:home could keep counters for how many times a default/non-default
> snippet was shown.

We would like info like that as it applies to both default, and the programmed snippets so thanks for letting me know. I will follow up separately.

Comment 25

[Marco Bonardo [:mak]]

landing on Places, will be merged soon
http://hg.mozilla.org/projects/places/rev/fec8ac14d8e6

Comment 26

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/fec8ac14d8e6

Comment 27

[Vlad [QA]]

How can i verify that the issue is fixed? Can you provide steps to reproduce?
thx

Comment 28

[Laura Forrest]

(In reply to comment #27)
> How can i verify that the issue is fixed? Can you provide steps to reproduce?
> thx

Steps to reproduce:
1. Go to about:home in Firefox 7 or above
2. Check the snippet box and make sure that you see a small graphic for each of the 2 snippets that show. Hit refresh to get to see both pieces of content.
-The two graphics: Add-on graphic and a blue bubble.
3. Check both links used to make sure that the tracking tags from Comment 0 have been implemented (they look like this "...?WT.mc_ID=defaultX")

Comment 29

[Marco Bonardo [:mak]]

Default snippets are shown only if no remote snippets are available. A simple way to force that, may be to create a new profile, set browser.aboutHomeSnippets.updateUrl to an invalid url, then delete chromeappsstore.sqlite from the profile folder (this may break the search box, but it's not interesting for this bug).

Comment 30

[Vlad [QA]]

Setting resolution to Verified Fixed.
Laura, thx for the steps to reproduce.