Closed Bug 1040369 Opened 6 years ago Closed 5 years ago

Replace sponsored icon with identifying text with overlay description

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(10 files, 9 obsolete files)

82.44 KB, video/webm
Details
301.37 KB, image/png
Details
130.40 KB, image/png
Details
140.37 KB, image/png
Details
121.25 KB, image/png
Details
v3
67.74 KB, patch
Details | Diff | Splinter Review
281.81 KB, image/png
Details
292.06 KB, image/png
Details
301.71 KB, image/png
Details
4.95 KB, patch
adw
: review+
Details | Diff | Splinter Review
This will likely revert bug 974745 (remove panel) and bug 974736 (remove icon).
Flags: firefox-backlog+
Depends on: 1040282
Blocks: 1030832
Attached video sample animation webm
Here's a video of a WIP that starts as a slightly rounded blue square that expands to the full size of some sponsored text. This works for different languages as it will cover up the title text (now always centered independent of the sponsored text).
Attached image sample animation stills
If we always need to show the text without user interaction, how about showing it opaque by default then making it transparent on hover. This will show the "paid tile" text by default but allow the user to see the title if the "paid" text happens to cover some of the title.
Attached patch wip (obsolete) — Splinter Review
adw, this patch removes the now unused controls/@2x.png files
Bug 1037341 and bug 1037091 remove other uses of controls.png @2x
Depends on: 1037341, 1037091
In the prototype: http://ytjbre.axshare.com/enhanced_view_-_concept_1.html

The "paid tile" button stays visible and is clickable like the current sponsored icon. So should we just reuse the current sponsored panel instead of implementing a whole new overlaid text?

Also, when the indicator was an icon, it has a fixed size but with text, it might cover up or be covered up by other title text. What should happen if the title text and sponsored text overlaps?

Perhaps related to the previous question, are we changing the tile's title from what it would have been even though the tile brings the user back to the original page? E.g., should bugzilla.mozilla.org tile have a title of "the nonprofit behind firefox" ?
Flags: needinfo?(athornburgh)
Notes:

Labels should say "SPONSORED" 4px side padding
Paid tile text/treatment
use enhanced title replacing history title
overlay sponsored description: #333333 background, white text
Summary: Replace sponsored icon with identifying text → Replace sponsored icon with identifying text with overlay description
More notes from dcrobot:

Static label: Border = 1, #DCDCDC, corner = 2, fill = #F9F9F9, text = Arial, Regular, 10 px, #9B9B9B
Rollover: fill = #DCDCDC, text = #666666
Flags: needinfo?(athornburgh)
(In reply to Ed Lee :Mardak from comment #7)
> overlay sponsored description: #333333 background, white text
dcrobot, your prototype seems to have a semitransparent background. What % opacity should this be? rgba(51, 51, 51, .9) ?
Flags: needinfo?(athornburgh)
95% opacity.
Flags: needinfo?(athornburgh)
Blocks: 1042214
Attached image v1 screenshot classic
webmaker tile is history with title Tools, sponsored text open for Wired
Attached image v1 screenshot enhanced
webmaker tile is enhanced with title Webmaker + enhanced text open, sponsored text open for Wired
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → edilee
Attachment #8462230 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8475738 - Flags: review?(adw)
Comment on attachment 8475738 [details] [diff] [review]
v1

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

For which branch is this patch? It's OK if it's for mozilla-central, not really for other branches (if you only target en-US as discussed, you shouldn't use the standard localization files).

Also: using in Gecko Gaia's conventions ({{var}}) for variable naming is confusing. It's probably my fault (linked document), but I assumed it was clear you just need to follow the standard conventions (%1$S, %2$S, etc. in .properties files).

::: browser/locales/en-US/chrome/browser/newTab.properties
@@ +6,5 @@
>  newtab.unpin=Unpin this site
>  newtab.block=Remove this site
> +newtab.sponsored.button=Sponsored
> +# LOCALIZATION NOTE(newtab.sponsored.explain): {{icon}} will be the (X) block
> +# icon inline. {{link}} will use newtab.learn.link as a link

I think it's not that clear.

XXX will be replaced inline by the X icon used to close tabs. YYY will be replace by an active link using string newtab.learn.link as text.

@@ +10,5 @@
> +# icon inline. {{link}} will use newtab.learn.link as a link
> +newtab.sponsored.explain=This tile is being shown to you on behalf a Mozilla partner. You can remove it at anytime by clicking the {{icon}} button. {{link}}
> +# LOCALIZATION NOTE(newtab.enhanced.explain): {{icon}} will be the customization
> +# icon inline. {{link}} will use newtab.learn.link as a link
> +newtab.enhanced.explain=A Mozilla partner has visually enhanced this tile replacing the screenshot. You can turn off enhanced tiles by clicking the {{icon}} button for in-page preferences. {{link}}

Same here

XXX will be replaced inline by the gear icon used to customize the new tab window. YYY will be replace by an active link using string newtab.learn.link as text.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8475738 - Attachment is obsolete: true
Attachment #8475738 - Flags: review?(adw)
Attachment #8475746 - Flags: review?(adw)
Attached patch v2 (obsolete) — Splinter Review
(In reply to Francesco Lodolo [:flod] from comment #14)
> For which branch is this patch? It's OK if it's for mozilla-central, not
> really for other branches (if you only target en-US as discussed, you
> shouldn't use the standard localization files).
This code will land initially on mozilla-central then uplifted to aurora. How should the strings work there? Basically put them inline or anywhere outside of the locales directory?

> Also: using in Gecko Gaia's conventions ({{var}}) for variable naming is
> confusing.
Either way works for me. I do see some l20n documentation saying it'll look like "{{ var }}" but maybe that just confuses things.

> follow the standard conventions (%1$S, %2$S, etc. in .properties files).
Done.

> > +# LOCALIZATION NOTE(newtab.sponsored.explain): {{icon}} will be the (X) block
> > +# icon inline. {{link}} will use newtab.learn.link as a link
> XXX will be replaced inline by the X icon used to close tabs. YYY will be
> replace by an active link using string newtab.learn.link as text.
Thanks.
Attachment #8475746 - Attachment is obsolete: true
Attachment #8475746 - Flags: review?(adw)
Attachment #8475765 - Flags: review?(adw)
Attached patch aurora extras (obsolete) — Splinter Review
Attachment #8475768 - Flags: feedback?(francesco.lodolo)
Attached patch aurora extras (obsolete) — Splinter Review
Attachment #8475768 - Attachment is obsolete: true
Attachment #8475768 - Flags: feedback?(francesco.lodolo)
Attachment #8475772 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8475772 [details] [diff] [review]
aurora extras

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

I think they're fine in the code (Pike is already CCed, so he might comment further).

f- for the newTab.properties: even if unused, don't remove those strings. 
Removal creates unnecessary noise (technically it's still "breaking the string freeze"), and it should ride the train from m-c
Attachment #8475772 - Flags: feedback?(francesco.lodolo) → feedback-
Also not sure what you mean with "aurora extra": you need an entire patch that doesn't touch string files, that patch makes me assume you want to land both, which is not OK.
(In reply to Francesco Lodolo [:flod] from comment #20)
> Also not sure what you mean with "aurora extra": you need an entire patch
> that doesn't touch string files, that patch makes me assume you want to land
> both, which is not OK.
You're saying there cannot be a changeset that adds the strings then another changeset that removes the strings in the same push?

You also said in comment 19 to not remove the strings, but that would be creating strings on aurora, no?
Attached patch v2 (obsolete) — Splinter Review
Getting late and attaching wrong files.. zzzz
Attachment #8475765 - Attachment is obsolete: true
Attachment #8475765 - Flags: review?(adw)
Attachment #8475782 - Flags: review?(adw)
(In reply to Ed Lee :Mardak from comment #21)
> You're saying there cannot be a changeset that adds the strings then another
> changeset that removes the strings in the same push?
> 
> You also said in comment 19 to not remove the strings, but that would be
> creating strings on aurora, no?

I realized only after what you were trying to do, I thought you were just removing existing strings in Aurora.

The entire point of string freeze is "do not touch string files". I find a specific patch for Aurora a lot safer and clearer than 2 patches (one for both branches, the other only for aurora).

In any case, the Aurora patch is wrong: patch 1 removes newtab.sponsored, patch doesn't add it back. See why I think it's safer?
Flags: qe-verify?
Attached patch v2 for aurora (obsolete) — Splinter Review
Attachment #8475772 - Attachment is obsolete: true
Comment on attachment 8475782 [details] [diff] [review]
v2

l10n question: Should the string just be SPONSORED? Or there should be a localization note saying that css will convert this to uppercase?

>+++ b/browser/locales/en-US/chrome/browser/newTab.properties
>+newtab.sponsored.button=Sponsored

>+++ b/browser/base/content/newtab/newTab.css
>+.newtab-sponsored {
>+  text-transform: uppercase;
It would be safer to put "SPONSORED" directly in the string with a note saying that the translation should be uppercase as well.

On the beauty of text-transform: uppercase
https://bugzilla.mozilla.org/show_bug.cgi?id=830002
Note, we're still waiting for input on how we'll eventually localize "sponsored" from legal.

So far we know little to nothing about the actual details, and how they affect local legal systems and what wording might be required where. That's really not a translation question, but needs local legal feedback based on a detailed description on what we're actually doing.
(In reply to Francesco Lodolo [:flod] from comment #26)
> On the beauty of text-transform: uppercase
> https://bugzilla.mozilla.org/show_bug.cgi?id=830002
Interesting Irish examples:
ÁR NATHAIR = OUR SNAKE
ÁR nATHAIR = OUR FATHER

-  text-transform: uppercase;
-newtab.sponsored.button=Sponsored
+# LOCALIZATION NOTE(newtab.sponsored.button): This text appears for sponsored
+# and enhanced tiles on the same line as the tile's title, so prefer short
+# strings to avoid overlap. This string should be uppercase.
+newtab.sponsored.button=SPONSORED

(In reply to Axel Hecht [:Pike] from comment #27)
> Note, we're still waiting for input on how we'll eventually localize
> "sponsored" from legal.
Is there something I should put in the localization note?
> (In reply to Axel Hecht [:Pike] from comment #27)
> > Note, we're still waiting for input on how we'll eventually localize
> > "sponsored" from legal.
> Is there something I should put in the localization note?

Actually, I've asked Bryan to get us data, I don't see a purpose until we get it from you guys. We need this answer to come out of the group that knows what tiles do.

Sadly, legal ducked away on the first try.
Comment on attachment 8475782 [details] [diff] [review]
v2

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

The code looks OK to me, and I guess it needs the text-transform change in comment 28, so r+.  I can't really comment on the legal question in comment 27, but it doesn't seem like that should block landing and iteration in Nightly.

::: browser/base/content/newtab/newTab.css
@@ +194,5 @@
> +.newtab-site:-moz-any([type=enhanced], [type=sponsored]) .newtab-sponsored {
> +  display: block;
> +}
> +
> +.sponsored-explain, .sponsored-explain a {

Nit: Please put the second selector on a new line to make this easier to read.

::: browser/base/content/newtab/newTab.js
@@ +36,5 @@
>  });
>  
> +function newTabString(name, args) {
> +  let stringName = "newtab." + name;
> +  if (args == undefined) {

Nit: if (!args) {

@@ +40,5 @@
> +  if (args == undefined) {
> +    return gStringBundle.GetStringFromName(stringName);
> +  }
> +  else {
> +    return gStringBundle.formatStringFromName(stringName, args, args.length);

`else` isn't necessary, just return.

::: browser/base/content/newtab/sites.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  #endif
>  
> +const SPONSORED_EXPLAIN_LINK = "https://wiki.mozilla.org/Tiles";

Would be good to have something here for curious Nightly users.

::: browser/locales/en-US/chrome/browser/newTab.properties
@@ +8,5 @@
> +newtab.sponsored.button=Sponsored
> +# LOCALIZATION NOTE(newtab.sponsored.explain): %1$S will be replaced inline by
> +# the (X) block icon. %2$S will be replace by an active link using string
> +# newtab.learn.link as text.
> +newtab.sponsored.explain=This tile is being shown to you on behalf a Mozilla partner. You can remove it at anytime by clicking the %1$S button. %2$S

"Anytime"'s an adverb, you need "any time" here.  Or, remove the "at," so, "You can remove it anytime."

@@ +12,5 @@
> +newtab.sponsored.explain=This tile is being shown to you on behalf a Mozilla partner. You can remove it at anytime by clicking the %1$S button. %2$S
> +# LOCALIZATION NOTE(newtab.enhanced.explain): %1$S will be replaced inline by
> +# the gear icon used to customize the new tab window. %2$S will be replace by an
> +# active link using string newtab.learn.link as text.
> +newtab.enhanced.explain=A Mozilla partner has visually enhanced this tile replacing the screenshot. You can turn off enhanced tiles by clicking the %1$S button for in-page preferences. %2$S

Need a comma between "tile" and "replacing."

"For in-page preferences" sure is awkward, and "in-page" is jargon.  Can't we just say "the %1$S button above"?

::: browser/themes/shared/newtab/newTab.inc.css
@@ +61,5 @@
>    outline: 1px dotted;
>  }
>  
>  /* CUSTOMIZE */
> +#newtab-customize-button, .newtab-customize {

Nit: Please put the second selector on a new line.
Attachment #8475782 - Flags: review?(adw) → review+
Attached patch v3Splinter Review
Attachment #8475782 - Attachment is obsolete: true
Attached image v3 screenshot
Attached patch aurora delta (obsolete) — Splinter Review
This patch is on top of the one landing in m-c, but when landing this patch for aurora, it'll be squashed into the same commit so there's no churn of locales files.
Attachment #8476013 - Attachment is obsolete: true
Points: --- → 8
Comment on attachment 8476907 [details]
v3 screenshot

Can you test this with a fake translation that has 1.5 - 2 times as much text?

Translations are generally a good deal longer.

I tried a few languages in google translate, 

Un partenaire Mozilla a amélioré visuellement cette tuile, le remplacement de la capture d'écran. Vous pouvez désactiver tuiles améliorés en cliquant sur le bouton X pour vos préférences. apprendre encore plus

might be a good example. (Probably google-translate-horrible, but the spacing might work)

Same for the "What is this page" box.
Attached image v3 capture d'écran
Attached image v3 screeeenshoot
Blocks: 1057602
https://hg.mozilla.org/mozilla-central/rev/df2c6fd11ac8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: --- → 34.3
Attached patch aurora deltaSplinter Review
Here's a patch on top of what landed in m-c but to make it string-freeze-friendly for aurora uplift.
Attachment #8477110 - Attachment is obsolete: true
Attachment #8477761 - Flags: review?(adw)
Uplift has been managed in bug 1057602
Depends on: 1057781
Blocks: 1057781
No longer depends on: 1057781
Attachment #8477761 - Flags: review?(adw) → review+
Ed, can you clarify what this is supposed to do so we can give it a little QA attention and verify it for some different locales? It sounds like it would benefit from some extra manual testing, but we would like to have a good definition of what tiles should be doing. Thanks!
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(edilee)
QA Contact: cornel.ionce
Sorry for the short comment 0 as the functionality wasn't super well defined then.

There are 3 main parts to this bug:
1) there's a "sponsored" button for both sponsored directory tiles and enhanced tiles
2) for sponsored directory tiles, e.g., wired, that show on an empty profile, clicking the "sponsored" button shows some explanation on top of the tile
3) for enhanced history tiles, e.g., a visit to a mozilla.org page, will have a different message shown when "sponsored" is clicked
Flags: needinfo?(edilee)
(In reply to Liz Henry :lizzard from comment #40)
> Ed, can you clarify what this is supposed to do so we can give it a little
> QA attention and verify it for some different locales?

Is this enabled for locales, even on trunk? My understanding was "no".
On trunk, only en-US has content to display although technically the code is enabled for all locales. You should be able to force the content to appear on no-en-US builds by changing general.useragent.locale to en-US.
Thanks, it works. 

Noticed this: after I click the "sponsored" tag, I have no way to hide the "infobar". I have to click the tile, close the tab or close the tile. It looks quite bad. Is there a bug already for this behavior?
You should be able to click "sponsored" again or any part of the overlaid description (other than the link) to dismiss.
(In reply to Ed Lee :Mardak from comment #45)
> You should be able to click "sponsored" again or any part of the overlaid
> description (other than the link) to dismiss.

Yes, it works, but I would have never imagined (and expect other users to be in the same situation).
Verified fixed on Mac OS X 10.8.5, Ubuntu 14.04 32bit and Windows 7 64bit using:
- latest Nightly, build ID: 20140828030205
- latest Aurora, build ID: 20140829004003
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.