Closed
Bug 974736
Opened 11 years ago
Closed 11 years ago
Add icon to title bar of Sponsored Tiles
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
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)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
jboriss
:
ui-review+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
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)
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.
Reporter | ||
Comment 2•11 years ago
|
||
Reporter | ||
Comment 3•11 years ago
|
||
Reporter | ||
Comment 4•11 years ago
|
||
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.
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.
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?
Flags: needinfo?(jboriss)
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.
Flags: needinfo?(jboriss) → needinfo?(edilee)
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.
Flags: needinfo?(edilee) → needinfo?(adw)
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.
Reporter | ||
Comment 11•11 years ago
|
||
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.
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=
Reporter | ||
Updated•11 years ago
|
Attachment #8381140 -
Attachment description: controls.png → controls.png (osx)
Reporter | ||
Comment 16•11 years ago
|
||
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.
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.
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?
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.
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
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.
Assignee: nobody → edilee
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.
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
Attachment #8381697 -
Flags: ui-review?(jboriss)
Comment 25•11 years ago
|
||
Attachment #8382513 -
Flags: ui-review+
Attachment #8382513 -
Flags: feedback+
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?
Updated•11 years ago
|
Attachment #8382512 -
Flags: feedback+ → ui-review?(jboriss)
Updated•11 years ago
|
Attachment #8382513 -
Flags: ui-review?(jboriss)
Attachment #8382513 -
Flags: ui-review+
Attachment #8382513 -
Flags: feedback+
Updated•11 years ago
|
Attachment #8382512 -
Flags: ui-review?(jboriss)
Updated•11 years ago
|
Attachment #8382513 -
Flags: ui-review?(jboriss)
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.
Flags: needinfo?(jboriss)
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.
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.
Flags: needinfo?(jboriss)
Reporter | ||
Updated•11 years ago
|
Attachment #8381697 -
Flags: ui-review?(jboriss) → ui-review+
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.
Updated•11 years ago
|
Assignee: edilee → mzhilyaev
Updated•11 years ago
|
Whiteboard: [tiles] → [tiles] p=0
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.
Flags: needinfo?(jboriss)
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.
Flags: needinfo?(jboriss)
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.
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?
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/
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.
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.
Updated•11 years ago
|
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
Flags: needinfo?(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.
Attachment #8386887 -
Flags: review?(adw) → feedback+
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.
Flags: needinfo?(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.
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?
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.
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.
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]
Comment 47•11 years ago
|
||
Attachment #8386887 -
Attachment is obsolete: true
Attachment #8391051 -
Flags: review?(adw)
Updated•11 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [tiles] p=0 → [tiles] p=3 s=it-31c-30a-29b.1
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.
Flags: in-testsuite?
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.1 → [tiles] p=3 s=it-31c-30a-29b.1 [qa-]
Updated•11 years ago
|
Attachment #8391051 -
Flags: review?(adw) → review+
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?
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. ;)
Attachment #8395252 -
Attachment is obsolete: true
Comment 53•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b, see bug 975228 comment 30.
Updated•11 years ago
|
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Updated•10 years ago
|
Component: Tabbed Browser → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•