Closed Bug 974736 Opened 6 years ago Closed 6 years ago

Add icon to title bar of Sponsored Tiles

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: jboriss, Assigned: mzhilyaev)

References

()

Details

(Whiteboard: [tiles] p=3 s=it-31c-30a-29b.2 [qa-])

Attachments

(13 files, 4 obsolete files)

75.65 KB, image/png
Details
1.32 KB, image/png
Details
1.35 KB, image/png
Details
1.49 KB, image/png
Details
7.16 KB, image/png
Details
21.51 KB, image/png
Details
6.26 KB, image/png
Details
46.36 KB, image/png
jboriss
: ui-review+
Details
72.55 KB, image/png
Details
68.34 KB, image/png
Details
145.04 KB, image/gif
Details
91.82 KB, image/png
Details
92.53 KB, patch
Details | Diff | Splinter Review
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)
Blocks: 974728
Blocks: 974745
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.
clarkbw, is there a requirement on the color of the indicator? If not we'll go with the attached icons.
(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.
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?
Flags: needinfo?(jboriss)
(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.
Flags: needinfo?(jboriss) → needinfo?(edilee)
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.
Flags: needinfo?(edilee) → needinfo?(adw)
(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.
Attached image controls@2x.png (obsolete) —
Attached image controls.png (osx)
Flags: needinfo?(adw)
(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.
Attached image controls@2x.png
Attachment #8381139 - Attachment is obsolete: true
(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=
Attachment #8381140 - Attachment description: controls.png → controls.png (osx)
(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.
Depends on: 976638
(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.
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?
(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.
Summary: Add icon to title bar of Sponsored Tiles that shows a panel on mouseover (with delay) → Add icon to title bar of Sponsored Tiles
(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.
Assignee: nobody → edilee
(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.
Attached image v1 screenshot
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
Attachment #8381697 - Flags: ui-review?(jboriss)
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?
Attachment #8382512 - Flags: feedback+ → ui-review?(jboriss)
Attachment #8382513 - Flags: ui-review?(jboriss)
Attachment #8382513 - Flags: ui-review+
Attachment #8382513 - Flags: feedback+
Attachment #8382512 - Flags: ui-review?(jboriss)
Attachment #8382513 - Flags: ui-review?(jboriss)
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.
Flags: needinfo?(jboriss)
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.
(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.
Flags: needinfo?(jboriss)
Attachment #8381697 - Flags: ui-review?(jboriss) → ui-review+
(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.
Blocks: tiles-dev
Assignee: edilee → mzhilyaev
Depends on: 975228
Whiteboard: [tiles] → [tiles] p=0
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.
Flags: needinfo?(jboriss)
(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.
Flags: needinfo?(jboriss)
My concern was more that the icon is facing the wrong way for RTL. It would be pointing back into the text.
Attached patch v1 (obsolete) — Splinter Review
Also fixes bug 976638.
Attachment #8386887 - Flags: review?(adw)
(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?
(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.
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.
(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
Flags: needinfo?(adw)
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.
Attachment #8386887 - Flags: review?(adw) → feedback+
(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.
Flags: needinfo?(adw)
(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.
(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?
(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.
(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.
Instead of using sponsored attribute, set type="value_from_type_field", in which case css rules have to check for [type=sponsored]
Blocks: 975210
Attached patch v2 (obsolete) — Splinter Review
Attachment #8386887 - Attachment is obsolete: true
Attachment #8391051 - Flags: review?(adw)
Status: NEW → ASSIGNED
Whiteboard: [tiles] p=0 → [tiles] p=3 s=it-31c-30a-29b.1
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.
Flags: in-testsuite?
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.1 → [tiles] p=3 s=it-31c-30a-29b.1 [qa-]
Attachment #8391051 - Flags: review?(adw) → review+
(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?
Attached patch for checkin (obsolete) — Splinter Review
Attachment #8391051 - Attachment is obsolete: true
Attached patch for checkinSplinter Review
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. ;)
Attachment #8395252 - Attachment is obsolete: true
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.1 [qa-] → [tiles] p=3 s=it-31c-30a-29b.2 [qa-]
https://hg.mozilla.org/mozilla-central/rev/5e07a425befc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Blocks: 976638
No longer depends on: 976638
Status: RESOLVED → VERIFIED
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Blocks: 1040369
Component: Tabbed Browser → New Tab Page
You need to log in before you can comment on or make changes to this bug.