Closed
Bug 657038
Opened 13 years ago
Closed 12 years ago
[Snippets] Add graphics and tracking tags to default snippets
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 7
People
(Reporter: lforrest, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Whiteboard: [about-home][fixed-in-places])
Attachments
(3 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•13 years ago
|
||
Reporter | ||
Comment 2•13 years ago
|
||
Features Graphic: https://bug657039.bugzilla.mozilla.org/attachment.cgi?id=533344
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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.
Reporter | ||
Comment 5•13 years ago
|
||
Thanks Marco! I'm not sure who else to ask, so it's fine if you give this a go. I appreciate the help.
Assignee: lforrest → nobody
Component: Snippets → General
Product: Mozilla Services → Firefox
QA Contact: snippets → general
Target Milestone: --- → Firefox 5
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #536498 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [about-home]
Target Milestone: Firefox 5 → ---
Comment 7•13 years ago
|
||
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.
Attachment #536498 -
Flags: review?(gavin.sharp) → feedback+
Assignee | ||
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
Can't you use #snippets > span, then? Do they really need to be styled in both containers?
Assignee | ||
Comment 10•13 years ago
|
||
(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•13 years ago
|
||
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 :)
Assignee | ||
Comment 12•13 years ago
|
||
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.
Attachment #536498 -
Flags: review?(dao)
Comment 13•12 years ago
|
||
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"); }
Attachment #536498 -
Flags: review?(dao) → review-
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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.
Reporter | ||
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
Please take the simpler approach and file a followup bug on tweaking the text alignment, I'll look into it there.
Assignee | ||
Comment 21•12 years ago
|
||
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.
Attachment #536498 -
Attachment is obsolete: true
Attachment #539487 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #539487 -
Flags: review?(dao) → review+
Comment 22•12 years ago
|
||
(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•12 years ago
|
||
(to be clear, that would be a followup, and is more for future data since telemetry isn't yet finished on trunk)
Reporter | ||
Comment 24•12 years ago
|
||
(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.
Assignee | ||
Comment 25•12 years ago
|
||
landing on Places, will be merged soon http://hg.mozilla.org/projects/places/rev/fec8ac14d8e6
Whiteboard: [about-home] → [about-home][fixed-in-places]
Assignee | ||
Comment 26•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fec8ac14d8e6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Comment 27•12 years ago
|
||
How can i verify that the issue is fixed? Can you provide steps to reproduce? thx
Reporter | ||
Comment 28•12 years ago
|
||
(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")
Assignee | ||
Comment 29•12 years ago
|
||
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•12 years ago
|
||
Setting resolution to Verified Fixed. Laura, thx for the steps to reproduce.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•