Jennifer Morrow [:Boriss] (UX) Reporter
	Description • 11 years ago

This icon will identify the tiles which are sponsored by Mozilla partners, differentiating them from Top Site tiles.

Acceptance criteria: 

1. On mouseover with standard tooltip delay, the icon displays the Sponsored Tile panel
2. On cessation of mouseover, the panel fades quickly and disappears
2. On click, the panel displays and maintains visibility even if the user ceases mouseover.  Panel is only dismissed from this state if user clicks else (in New Tab page, on the icon itself, or in Firefox chrome)

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 1 • 11 years ago

The attached mockup below shows how the user interacts with the Sponsored Tile icon in New Tab.  For v1, the icons can be identical across platforms.

	Ed Lee :Mardak
	Comment 5 • 11 years ago

clarkbw, is there a requirement on the color of the indicator? If not we'll go with the attached icons.

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 6 • 11 years ago

(In reply to Edward Lee :Mardak from comment #5)
> clarkbw, is there a requirement on the color of the indicator? If not we'll
> go with the attached icons.

There's a requirement the there an indicator exists but no requirements on colour that I'm aware of.

	Ed Lee :Mardak
	Comment 7 • 11 years ago

Boriss, do we want to add the sponsored icon to the existing sprite sheet that contais 24x24 icons (for pin, remove, show tiles)

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/newtab/controls.png

I see that these icons are 14x14, so either we put extra transparent-space in the existing controls.png file or make a new 14x(3*14) sprite sheet for the 3 new images.

Also, there's a 2x resolution controls file:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/newtab/controls@2x.png

Do we need an equivalent 28x28?

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 8 • 11 years ago

(In reply to Edward Lee :Mardak from comment #7)
> Boriss, do we want to add the sponsored icon to the existing sprite sheet
> that contais 24x24 icons (for pin, remove, show tiles)

If that's the bets way to do it, then we should do that.  I can do it manually if there's no easier way (I'll ping Shorlander for an opinion).  This is an engineering rather than UX question, so I defer to you, Mardak.

> Also, there's a 2x resolution controls file:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/newtab/
> controls@2x.png
> 
> Do we need an equivalent 28x28?

I'm assuming we do.  I'll go ahead and make one and attach it here.

	Ed Lee :Mardak
	Comment 9 • 11 years ago

adw, would you recommend we combine the three 14x14 sponsored icon states/sprites into its own sprite sheet as opposed to fitting them into the 24x24 icons of controls.png? Perhaps we'll call it sponsored.png!

And we should just copy this into each of windows, osx, and linux newtab directories? As well as making the equivalent @2x sheet per bug 781327.

	Matthew N. [:MattN]
	Comment 10 • 11 years ago

(In reply to Ed Lee :Mardak from comment #9)
> And we should just copy this into each of windows, osx, and linux newtab
> directories? As well as making the equivalent @2x sheet per bug 781327.

No, we have a browser/themes/shared/ directory now that is perfect for cases where the resources are identical across platforms. Put them in browser/themes/shared/newtab/ and reference then in all three platforms' jar.mn using relative paths in the brackets (see others doing the same there).

The @2x version should probably go alongside the existing controls@2x.png until we support higher resolutions on non-OSX since we don't know what sizes of images we'll need for those other platforms.

	Matthew N. [:MattN]
	Comment 13 • 11 years ago

(In reply to Ed Lee :Mardak from comment #9)
> adw, would you recommend we combine the three 14x14 sponsored icon
> states/sprites into its own sprite sheet as opposed to fitting them into the
> 24x24 icons of controls.png? Perhaps we'll call it sponsored.png!

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #12)
> Created attachment 8381140 [details]
> controls.png

The existing controls.png differs by platform so either these three should go in a new spritesheet in shared/ or you should update all three controls.png.

	Matthew N. [:MattN]
	Comment 15 • 11 years ago

(In reply to Matthew N. [:MattN] from comment #13)
> or you should update all three controls.png.

To clarify, Windows and Linux are currently the same but OS X is different:
https://mxr.mozilla.org/mozilla-central/find?string=newtab%2Fcontrols.png&tree=mozilla-central&hint=

	Stephen Horlander [:shorlander]
	Comment 17 • 11 years ago

(In reply to Matthew N. [:MattN] from comment #15)
> (In reply to Matthew N. [:MattN] from comment #13)
> > or you should update all three controls.png.
> 
> To clarify, Windows and Linux are currently the same but OS X is different:
> https://mxr.mozilla.org/mozilla-central/find?string=newtab%2Fcontrols.
> png&tree=mozilla-central&hint=

Wonder how that happened. They should all be the same.

	Drew Willcoxon :adw
	Comment 18 • 11 years ago

(In reply to Ed Lee :Mardak from comment #9)
> adw, would you recommend we combine the three 14x14 sponsored icon
> states/sprites into its own sprite sheet as opposed to fitting them into the
> 24x24 icons of controls.png? Perhaps we'll call it sponsored.png!

Sounds fine to me, and I think Matt addressed this, too.

	Madhava Enros [:madhava]
	Comment 19 • 11 years ago

It looks like this bug is actually just about creating icon/indicator, vs. another bug which is about having the hover, etc. Can the acceptance criteria here be just about having the indicator?

	Ed Lee :Mardak
	Comment 20 • 11 years ago

(In reply to Madhava Enros [:madhava] from comment #19)
> It looks like this bug is actually just about creating icon/indicator, vs.
> another bug which is about having the hover, etc.
That was the intent of separately filing bug 974745.

> Can the acceptance criteria here be just about having the indicator?
Are you asking or clarifying that this bug is focused on just showing the icon appropriately? If so, yes.

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 21 • 11 years ago

(In reply to Ed Lee :Mardak from comment #20)
> (In reply to Madhava Enros [:madhava] from comment #19)
> > It looks like this bug is actually just about creating icon/indicator, vs.
> > another bug which is about having the hover, etc.
> That was the intent of separately filing bug 974745.
> 
> > Can the acceptance criteria here be just about having the indicator?
> Are you asking or clarifying that this bug is focused on just showing the
> icon appropriately? If so, yes.

So to close this bug, I'm assuming we just need to position this icon in the titlebar for some (sponsored) tiles.  Now that the icon exists, this should be assigned to a developer.  Ed, I'll assign it to you for now, and if you want to handle all positioning issues on bug 974745 instead, we can close this, put the icons there, and patch there as well.

	Ed Lee :Mardak
	Comment 22 • 11 years ago

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #21)
> if you want to handle all positioning issues on bug 974745 instead,
> we can close this, put the icons there, and patch there as well.
What positioning issues are you referring to? I suppose we can just position the icon towards the bottom right and if there's any problems, we can have a bug to tweak pixel positions.

	Ed Lee :Mardak
	Comment 23 • 11 years ago

Does this look okay? (Note title cropping)

For notes:
- newtab-title's padding-right increase 8px -> 24px
- new <input> positioned bottom/right 4px, background-position -248px, height/width 16px, opacity 1 if sponsored

	Olivier Yiptong [:oyiptong]
	Comment 26 • 11 years ago

Attached are a couple of screenshots describing behavior we'd like feedback on.

Please disregard the icons used, icon states and panel contents.

In situation 1, the button is hovered over and a panel is shown.
In situation 2, the mouse cursor leaves the button to enter the panel.

Should leaving the button make the panel disappear?

	Ed Lee :Mardak
	Comment 27 • 11 years ago

I believe oyiptong is asking if the panel should stay visible or get hidden if the mouse cursor moves towards the panel without first clicking the button.

	Ed Lee :Mardak
	Comment 28 • 11 years ago

Boriss, is it okay to only show the panel on click? There's going to be some odd behavior of clicking the arrow to make the panel persist because the panel will already be open.

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 29 • 11 years ago

(In reply to Ed Lee :Mardak from comment #23)
> Created attachment 8381697 [details]
> v1 screenshot
> 
> Does this look okay? (Note title cropping)

Yes, that does look ok!  It'd be great if future versions could do a more subtle gradient fade out, but for now: 1. That's really hard and 2. We shorten Titles with ellipses, so this is consistent.

(In reply to Ed Lee :Mardak from comment #28)
> Boriss, is it okay to only show the panel on click? There's going to be some
> odd behavior of clicking the arrow to make the panel persist because the
> panel will already be open.

Ha, I went back and forth on this so many times.  Yes, that's fine, let's show it on mouseover.  It does need a tooltip, in that case.  I propose "Show information on sponsored tiles" to be consistent with verb tooltips.

	Ed Lee :Mardak
	Comment 30 • 11 years ago

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #29)
> Yes, that's fine, let's show it on mouseover.  It does need a tooltip
To be clear, you mean show the panel on click and show a regular title/tooltip on wait.

	Ed Lee :Mardak
	Comment 31 • 11 years ago

Boriss, do you know who to talk to about ltr/rtl considerations? The icon theoretically could point down which is where the panel normally appears anyway. Right now the icon is pointing right assuming a ltr layout.

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 32 • 11 years ago

(In reply to Ed Lee :Mardak from comment #31)
> Boriss, do you know who to talk to about ltr/rtl considerations? The icon
> theoretically could point down which is where the panel normally appears
> anyway. Right now the icon is pointing right assuming a ltr layout.

I believe that Ehsan Akhgari would be a good person to talk to about LTR.  We could possibly display the icon at the end of the line of text of the website title, such that websites with smaller-than-maximum titles would show the icon not at the corner of the image.  Hopefully the attached clarifies.

	Ed Lee :Mardak
	Comment 33 • 11 years ago

My concern was more that the icon is facing the wrong way for RTL. It would be pointing back into the text.

	Ed Lee :Mardak
	Comment 34 • 11 years ago

Also fixes bug 976638.

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 35 • 11 years ago

(In reply to Ed Lee :Mardak from comment #34)
> Created attachment 8386887 [details] [diff] [review]
> v1
> 
> Also fixes bug 976638.

Ed, would you be willing to make & post a build with this for a UX review?

	Ed Lee :Mardak
	Comment 36 • 11 years ago

I started a build: https://tbpl.mozilla.org/?tree=Try&rev=5f15bb1045a6
The files should appear here when it finishes:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/edward.lee@engineering.uiuc.edu-5f15bb1045a6/

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 37 • 11 years ago

(In reply to Ed Lee :Mardak from comment #36)
> I started a build: https://tbpl.mozilla.org/?tree=Try&rev=5f15bb1045a6

Thanks Mardak, this is getting awesome!  I did want to point out a text alignment issue:

1. The “Learn more” link should be aligned left, with the rest of the text
2. The leading distance between “Learn More” and the left of the window, bottom of window, and text above it should be equal.

No close button is fine, since we decided to activate this on click rather than on hover!  You mentioned on IRC that the icon wasn’t rendering yet and another was coming, so no issue there.

	Ed Lee :Mardak
	Comment 38 • 11 years ago

from Boriss: the gutters are great.  the display is fantastic.  the only missing piece is mouseover.  right now, mousing over that icon highlights the thumbnail rather than the icon itself, making it look like the target is the site rather than the sponsored icon.  i know this is because the whole title bar right now highlights the thumbnail, which is all well and good.. until there's a separate target there, e.g. the sponsored icon.  so the on change i think we need is to *not* highlight the thumbnail when the user is over the sponsored icon.  whether the thumbnail is highlighted when the user is over just the non-sponsored-icon bit of the title or not, frankly whatever's easiest for you is fine there

Basically we should try to treat the icon not as a child of the cell to prevent it from triggering mouseover effects.

	Ed Lee :Mardak
	Comment 39 • 11 years ago

(In reply to Ed Lee :Mardak from comment #38)
> mousing over that icon highlights the thumbnail rather than the icon itself
To be clear, both the icon and thumbnail are being highlighted. It's just that the difference between unhover and hovered for the sponsored icon is very minor.

adw, I've fixed this by setting .newtab-cell[ignorehover] after maksik tried a bunch of css selectors and ordering changes to get it to work for thumbnail opacity. But those tricks would have failed for the cell's box shadow, etc.

Do you want this as a separate bug? Here's the initial patch:
https://github.com/Mardak/tiles-dev/commit/6037c8901d8986c6edff13566bc39343192c50e2

	Drew Willcoxon :adw
	Comment 40 • 11 years ago

Comment on attachment 8386887 [details] [diff] [review]
v1

Review of attachment 8386887 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but it sounds like there are some changes coming?  And I can't tell where providerData.type comes from.

::: browser/base/content/newtab/sites.js
@@ +129,5 @@
>      let title = this.title || url;
>      let tooltip = (title == url ? title : title + "\n" + url);
>  
> +    // set site "sponsored" attribute
> +    if (this.providerData.type == "sponsored") {

The idea behind providerData is that it's opaque to everybody but the provider, so reaching in and grabbing a property on it isn't what I had in mind.  (After commenting in bug 975228, I think providerData might not even be necessary anymore.)  But anyway, the front end does need to tell whether the link is sponsored, so I think it's fine to simply have link.sponsored.  Links from the directory provider would set the property to true, and links from the Places provider just wouldn't define it at all.

	Drew Willcoxon :adw
	Comment 41 • 11 years ago

(In reply to Ed Lee :Mardak from comment #39)
> adw, I've fixed this by setting .newtab-cell[ignorehover] after maksik tried
> a bunch of css selectors and ordering changes to get it to work for
> thumbnail opacity. But those tricks would have failed for the cell's box
> shadow, etc.
> 
> Do you want this as a separate bug?

Could we add that to this bug's patch?  I think that would be easiest for me to follow, anyway.

	Drew Willcoxon :adw
	Comment 42 • 11 years ago

(In reply to Ed Lee :Mardak from comment #38)
> whether the thumbnail is highlighted when the user is over just the
> non-sponsored-icon bit of the title or not, frankly whatever's easiest
> for you is fine there

If that's what Boriss says, then maybe it would be cleanest to move the <span class="newtab-title"/> and the sponsored button out of the newtab-site div.  Put them below it so they don't trigger its hover effect.

	Ed Lee :Mardak
	Comment 43 • 11 years ago

(In reply to Drew Willcoxon :adw from comment #42)
> maybe it would be cleanest to move the <span class="newtab-title"/> and the sponsored button out of the newtab-site div.
Any suggestions? Seems like there'll need to be some surgery as the siteFragment gets cloned. Should we create the title/button directly into the cell or continue the current site creation and on init, move nodes into the cell parent?

	Ed Lee :Mardak
	Comment 44 • 11 years ago

(In reply to Drew Willcoxon :adw from comment #40)
> > +    if (this.providerData.type == "sponsored") {
> The idea behind providerData is that it's opaque to everybody but the
> provider, so reaching in and grabbing a property on it isn't what I had in
> mind.  (After commenting in bug 975228, I think providerData might not even
> be necessary anymore.)
I think we might want to go with your original suggestion of having the Link object contain the image, so PlacesProvider sets it with the PageThumbs url while DirectoryTiles has the desired data uri. Similarly we want the shared rank value which would be frecency from Places while something Directory would probably have fake values of 1000, 1001, 1002, etc. And lastly, there could be a type, so Places would set "places" while Directory would set sponsored/organic/etc.

I'm thinking the Links object: {
  title: string,
  url: string,
  imageUri: string,
  rank: number,
  type: string
}

It seems that placeId for bug 911307 isn't really used other than for tie-breakers, and I think comparing titles might be fine.

	Drew Willcoxon :adw
	Comment 45 • 11 years ago

(In reply to Ed Lee :Mardak from comment #43)
> Any suggestions?

Right now there's one DOM container for a site, the .newtab-site div.  Couldn't there be two containers, one within the other?  The inner container would not contain the title or sponsored button but would contain everything else and get the hover styling, and the outer container would do everything else that the .newtab-site div currently does, like be draggable.  I haven't thought a lot about it, though.

(In reply to Ed Lee :Mardak from comment #44)
> I'm thinking the Links object: {
>   title: string,
>   url: string,
>   imageUri: string,
>   rank: number,
>   type: string
> }

That sounds OK to me, but please capitalize it imageURI instead of imageUri, and I think rank should just be called frecency if that's what it is, and:

> It seems that placeId for bug 911307 isn't really used other than for
> tie-breakers, and I think comparing titles might be fine.

The point of using placeID is to match the tie-breaking behavior of the initial Places query that PlacesProvider uses.  If you used some other tie breaker, then you'll potentially see a different ordering of sites in the grid after you restart the app.  Although Marco says in bug 911307 that I might not be able to include the ID in the Places notifications the front end relies on, so it's uncertain whether we can use the ID at all.

	maxim zhilyaev Assignee
	Comment 46 • 11 years ago

Instead of using sponsored attribute, set type="value_from_type_field", in which case css rules have to check for [type=sponsored]

	u279076
	Comment 48 • 11 years ago

I don't think this needs special QA testing and can likely be covered via automation. Please let me know if test coverage will not be included with the fix.

	Jennifer Morrow [:Boriss] (UX) Reporter
	Comment 49 • 11 years ago

(In reply to Ed Lee :Mardak from comment #47)
> Created attachment 8391051 [details] [diff] [review]
> v2

Could I possibly grab a build or screenshot for ui review?

	Ed Lee :Mardak
	Comment 51 • 11 years ago

adw, FYI emtwo noticed and fixed an issue with hidpi/@2x where we forgot to..

-    background-size: 248px;
+    background-size: 296px;

for the new file sizes. For a moment she thought she was going crazy with only her mac showing broken icons. ;)

	Ed Lee :Mardak
	Comment 52 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/5227bbca70f0

	Phil Ringnalda (:philor)
	Comment 53 • 11 years ago

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b, see bug 975228 comment 30.

	Ed Lee :Mardak
	Comment 54 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/5e07a425befc

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 55 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5e07a425befc