963688 - Add a default bookmark for feedback (input.mozilla.org)

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Bookmarks, defect, P1)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We want to provide some quick, simple (fx28) way for people to provide some feedback. We plan to add a default bookmark that links to a Metro-specific Input URL.

We need:
* Icon (what size is best?)
* Text
* Metro-specific URL

Comment 1

[Marco Mucci [:MarcoM]]

Hi Flod, would this require localization?

Comment 2

[Francesco Lodolo [:flod]]

Never had to touch bookmarks so far, CCing Pike for more expertise on the matter. 

For sure each locale on Firefox has a bookmarks.inc to localize default bookmarks titles, e.g.
http://hg.mozilla.org/l10n-central/it/file/3e57a9271912/browser/profile/bookmarks.inc

And this file is subject to productization (no changes without approval from l10n-drivers).

If this bookmark is going to have more text than just "Metro Feedback" or similar (i.e. something easy enough to be understood with a limited knowledge of English), we definitely need a localization, but I honestly don't know how this can happen for Firefox 28.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Francesco Lodolo [:flod] from comment #2)

> If this bookmark is going to have more text than just "Metro Feedback" or
> similar (i.e. something easy enough to be understood with a limited
> knowledge of English), we definitely need a localization, but I honestly
> don't know how this can happen for Firefox 28.

I'm happy to ship this in en-US so we don't cause l10n churn. We are looking for some way to get feedback. And some feedback is better than no feedback.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I received information on the URL from the User Advocacy team:

> The new schema that we will support looks like this:
> 
> * https://input.mozilla.org/%LOCALE%/feedback/%PRODUCT%/%VERSION%/%CHANNEL%/
> 
> Version and Channel are not required, but if you want to add that information we can sort the feedback
> in an even more granular fashion. Up to you. So for example, you could also use:

> * https://input.mozilla.org/%LOCALE%/feedback/%PRODUCT%/%VERSION%/
> * https://input.mozilla.org/%LOCALE%/feedback/%PRODUCT%/

> I'm assuming product will just be "metro", but you can use whatever you like just so long as we know
> ahead of time.

Since this is a bookmark URL, we can't auto-replace %LOCALE% using the nsIURLFormatter. I just tried loading a version without the locale and it seems to do language sniffing anyway. We can use:
* https://input.mozilla.org/feedback/metro

(assuming we want to use "metro", which I am fine with using)

Comment 5

[Francesco Lodolo [:flod]]

> I'm happy to ship this in en-US so we don't cause l10n churn. 
> We are looking for some way to get feedback. And some feedback is better than no feedback.

I think this is probably a good compromise, considering also the temporary nature of this link.

> Since this is a bookmark URL, we can't auto-replace %LOCALE% using the
> nsIURLFormatter. I just tried loading a version without the locale and it
> seems to do language sniffing anyway. We can use:
> * https://input.mozilla.org/feedback/metro

Yes, input has locale detection, so I'm redirected to https://input.mozilla.org/it/feedback/metro when I click this link.

Wondering if there are alternative ways to display that link without touching strings in the product (e.g. what's new page for Metro?).

Comment 6

[Jim Mathies [:jimm]]

%PRODUCT% will need to be hard coded since our MOZ_APP_BASENAME is Firefox due to profile sharing. 'metrofirefox' or 'MetroFirefox' would be best. Using the app id would probably be better but isn't as pretty.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jim Mathies [:jimm] from comment #6)
> %PRODUCT% will need to be hard coded since our MOZ_APP_BASENAME is Firefox
> due to profile sharing. 'metrofirefox' or 'MetroFirefox' would be best.
> Using the app id would probably be better but isn't as pretty.

We hard things like this for Firefox for Android too. There, we use 'android' for %PRODUCT%, so here we have candidates: 'metro' and  'metrofirefox'

Anyone have a strong reason to use one or the other? Or add a third? If we already use 'metrofirefox' in other places, that would be enough for me.

...<mfinkle uses MXR>...

We use 'metrofirefox' as the client name in search plugins:
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/searchplugins/googlemetrofx.xml#11

Good enough for me. Let's use 'metrofirefox' for the Input %PRODUCT% too.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

Hi! I work on Input. Input doesn't currently support the urls we're talking about here--I'm working on the changes required this week.

Everything I'm seeing here should work with Input after the changes I need to make are made. I'll make sure to test the final url used and the bookmark with Input.

The one thing I don't have is an icon. Someone else will have to provide that.

Comment 9

[Axel Hecht [:Pike]]

Just to clarify the expectations here:

Adding a default bookmark will only affect profiles created with the updated defaults.

It will neither add a bookmark to existing profiles, nor will removing the bookmark from our defaults remove them from profiles in the future.

Comment 10

[Axel Hecht [:Pike]]

... alternatively, I wonder if the snippet service is specific enough to serve a "give us feedback on metro"?

Comment 11

[Jim Mathies [:jimm]]

(In reply to Axel Hecht [:Pike] from comment #10)
> ... alternatively, I wonder if the snippet service is specific enough to
> serve a "give us feedback on metro"?

We currently don't support that in the metro browser. What we want to do is add a bookmark to the about:start page in metro for easy access.

Comment 12

[Matt Grimes [:Matt_G]]

As willkg mentioned, we don't have a favicon. The current input favicon is simply the Firefox logo, but it is way too small. If you have a larger version of that, it should suffice. Unless we want to do something more unique.

Comment 13

[Will Kahn-Greene [:willkg] ET needinfo? me]

I added support for product urls and pushed it to production just now.

For example, the following urls will all work:

* https://input.mozilla.org/feedback/metrofirefox
* https://input.mozilla.org/feedback/metrofirefox/29
* https://input.mozilla.org/feedback/metrofirefox/29/nightly
* https://input.mozilla.org/feedback/metrofirefox/29/beta
* https://input.mozilla.org/feedback/metrofirefox/29/aurora
* https://input.mozilla.org/feedback/metrofirefox/29/stable

These can be used as a bookmark as well as in blog posts, Metro Firefox website, etc.

This url has the locale in it, too. I'd avoid using this url since it'll force the user to leave feedback in English rather than their preferred language:

* https://input.mozilla.org/en-US/feedback/metrofirefox/29/stable

Hope that helps!

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Adding (In reply to Mark Finkle (:mfinkle) from comment #0)
> We want to provide some quick, simple (fx28) way for people to provide some
> feedback. We plan to add a default bookmark that links to a Metro-specific
> Input URL.
> 
> We need:
> * Icon (what size is best?)

Adding uiwanted keyword for this part

> * Text
> * Metro-specific URL

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: feedback-bookmark

I've got the new bookmark in there, I used the base64 encoded favicon that's on input.mozilla.org. I made it the 2nd bookmark. You'll need to move/rename your AppData\Roaming\Mozilla\Firefox directory to get the first run behavior and pull in the new defaults. 
I think this represents the outcome of the discussion on this issue: we link direct to https://input.mozilla.org/feedback/metrofirefox - there's no app version or locale or anything in the url, just the app name we picked 'metrofirefox'. 
This bookmarks file is in locales\generic\profile\bookmarks.json.in, is that correct? Also, the title on the bookmark is "Feedback" - hardcoded, no new l10n strings. Is that what we want?

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8370344 [details] [diff] [review]
feedback-bookmark

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

::: browser/metro/theme/firstrun.css
@@ +23,5 @@
>  }
>  
>  /* Keep only first few items */
>  #start-container:not([viewstate="snapped"]) #start-history-grid richgriditem:nth-child(n+3),
> +#start-container:not([viewstate="snapped"]) #start-bookmarks-grid richgriditem:nth-child(n+4),

Would it make sense to expand the history grid also, just to keep them aligned?

Or keep the bookmark grid unchanged, and just not show all the bookmarks during firstrun...  This only affects the first 3 times the Firefox Start page is loaded.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Attachment: Adds "Give Feedback" as default bookmark

Per discussion in #windev with :mfinkle, :mbrubeck, this patch adds "Give Feedback" as a default bookmark. The new string for this title is in browser/locales/.../bookmarks.inc. Minor adjustment to first run to accomodate 3 initial bookmarks

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8370389 [details] [diff] [review]
Adds "Give Feedback" as default bookmark

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

r=mbrubeck but let's figure out our plan for Aurora and Beta before landing this.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Attachment: Screenshot with attachment 8370389 applied

Here's how it looks on first run

Comment 20

[Sam Foster [:sfoster] (he/him)]

On IRC we discussed an alternative option which would be to add a feedback item to the charms bar. We might be able to reuse an existing localized string there, where it is difficult/impossible here. We may want this as well as the bookmark. Either way, the beta window is closing fast so we need a plan.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sam Foster [:sfoster] from comment #20)
> On IRC we discussed an alternative option which would be to add a feedback
> item to the charms bar. We might be able to reuse an existing localized
> string there, where it is difficult/impossible here. We may want this as
> well as the bookmark. Either way, the beta window is closing fast so we need
> a plan.

We do have plans for the Charm in bug 963691, but if you could find a simple place to put a link in the existing panel and that could use an existing string, then put up a separate patch.

We could consider uplifting only one of the patches.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

We already have a link to "Help (online)" so we could add a link there to feedback.

Comment 23

[Jim Mathies [:jimm]]

Is this ready to land?

Comment 24

[Sam Foster [:sfoster] (he/him)]

I've re-tagged it for triage. We just need to finish deciding what to do with it - do we use the same hack to get the existing string in, which would allow uplift. Or, do we create a new string and let it ride the trains. Or both. Or, now that we've got the settings charm item, maybe we don't need it at all. I think we agree that the charm menu isn't very discoverable, so the bookmark remains a good idea, but lets discuss at the next triage meeting.

Comment 25

[Matt Grimes [:Matt_G]]

Let me know the outcome of that meeting. I'd be happy to join as well. I'll be aggregating all the feedback coming in, so it would help to be kept in the loop as much as possible!

Comment 26

[Jim Mathies [:jimm]]

The string we originally decided to use in the charm isn't the right format. So I'd suggest:

1) we put the bookmark in using the "Submit Feedback..." string hack, and uplift to aurora/beta
2) update the bookmark string to something more appropriate on mc
3) update the settings charm string currently on mc and let the charm percolate up normally.

I've filed bug 972608 and posted a patch on updating the charm string on mc.

Comment 27

[Francesco Lodolo [:flod]]

(In reply to Jim Mathies [:jimm] from comment #26)
> 1) we put the bookmark in using the "Submit Feedback..." string hack, and
> uplift to aurora/beta

I'm lost: what string hack?

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's also get the patch landed on Nightly (m-c) ASAP. There are no string concerns there and we want the bookmark.

Comment 29

[Jim Mathies [:jimm]]

(In reply to Francesco Lodolo [:flod] from comment #27)
> (In reply to Jim Mathies [:jimm] from comment #26)
> > 1) we put the bookmark in using the "Submit Feedback..." string hack, and
> > uplift to aurora/beta
> 
> I'm lost: what string hack?

We pilfered a suitable string from desktop - 

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd#35
http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/jar.mn#42

Comment 30

[Francesco Lodolo [:flod]]

Thanks, I was worried about reusing a string that looked OK in English, but not in other locales. 
As a temporary solution it should work, problem is that bookmark is going to stick forever once created, isn't it?

Comment 31

[Jim Mathies [:jimm]]

(In reply to Francesco Lodolo [:flod] from comment #30)
> Thanks, I was worried about reusing a string that looked OK in English, but
> not in other locales. 
> As a temporary solution it should work, problem is that bookmark is going to
> stick forever once created, isn't it?

Yeah I think that's correct. Personally I think we should just roll out with the right strings and forgo uplift using the work around. We'll triage this bug again on Monday to make a final decision.

Comment 32

[Francesco Lodolo [:flod]]

You could also add the bookmark now with an hard-coded string just for en-US on aurora/beta (string directly in the code, not exposed in properties/dtd file), and let the external string ride the train for other locales+en-US.

Since the text is already definitive for en-US, it wouldn't be a problem having this bookmarks stick.

Comment 33

[Sam Foster [:sfoster] (he/him)]

The plan - from triage meeting:
 
* Land default bookmark with new string on nightly
* Land hard-coded en-us bookmark on aurora and beta (using same en-us label)

Comment 34

[Sam Foster [:sfoster] (he/him)]

Attachment: Add feedback title to bookmarks.inc, new default bookmark for metro

For m-c only (I'll create a separate patch for uplift). This is not much changed from previous, reviewed patch I just need a sanity check that it implements what we agreed. The bookmark title is added to shared bookmarks.inc - The string itself isn't metro-specific but as its only currently used in metro I tried to communicate that in the comment, without precluding its later adoption elsewhere. The new bookmark should be there for all locales in metro for m-c.

Comment 35

[Sam Foster [:sfoster] (he/him)]

Landed attachment #8380798 [details] [diff] [review] on fx-team: https://hg.mozilla.org/integration/fx-team/rev/8a77900b6fbe
As this includes a new string, it is not for uplift. I'll get an uplift-ready, aurora/beta-only patch review and landed here shortly.

Comment 36

[Sam Foster [:sfoster] (he/him)]

I'm struggling to see how to create the en-US only default bookmark for the aurora/beta targeted patch. I see in browser/metro/locales/import/Makefile.in we pick up the strings in bookmarks.inc, but the bookmarks themselves are always coming from http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/import/Makefile.in#41

.. Am I right in thinking that we want some locale-specific condition in there to pull in a different file, such as $(srcdir)/../en-US/profile/bookmarks.json.in?

Comment 37

[Sam Foster [:sfoster] (he/him)]

ah, MWC. Can you direct me Matt?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a77900b6fbe

Comment 39

[Sam Foster [:sfoster] (he/him)]

Unless a safe/clean way exists to add a specific default bookmark for a single (en-US or just en) locale only, I think we should just let this ride the trains and rely on bug 969045 as our "give feedback" UI in the meantime. See comment 32 and comment 36. Do you know if such a mechanism exists or what is involved in making this happen :flod?

Comment 40

[Francesco Lodolo [:flod]]

(In reply to Sam Foster [:sfoster] from comment #39)
> Do you know if such a mechanism exists or what is involved in making this happen :flod?

Unfortunately I don't know. 

If you add the bookmark in bookmarks.inc, it will break string freeze, and worse, all locales will get the English text because of the fallback system.

If you go with "include another file for en-US", you need to pay attention not to add the file in a section controlled by compare-locales (http://hg.mozilla.org/mozilla-central/file/default/browser/locales/l10n.ini)

Comment 41

[Axel Hecht [:Pike]]

https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/buildsystem/preprocessor.html#if shows how do to conditionals in preprocessed files, so you could just hard-code the en-US bookmarklet in bookmarks.json.in, and make it en-US only there.

Comment 42

[Sam Foster [:sfoster] (he/him)]

I think with the settings charm already in beta, we can let this slide as-is. We've got the bookmark landed for localization in m-c. I don't think the wins outweigh the cost/risks to attempt to get a en-US only into 28/29

Comment 43

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 44

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-01-03-02-04-mozilla-central/

- Ensured that "Give Feedback" is appearing under "Bookmarks" after a brand new installation with a new profile
- Ensured that taping on the "Give Feedback" tile opens the correct website using the correct URL
- Ensured that the tile is correctly aligned with the other tiles
- Ensured that it's using the Firefox favicon as discussed above
- Ensured that you can select the "Give Feedback" tile by swiping (hide, delete, undo delete, clear selection)
- Ensured that "Feedback (online)" under the settings flyout is still working correctly and directing users to the same website as the tile under "Bookmarks"
- Ensured that adding other bookmarks doesn't cause any visible regressions or visual issues
- Went through all of the above test cases in different variations of snapped view

Installed the following Nightly build and than added several different bookmarks (around 10):
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-24-03-02-03-mozilla-central/

Updated to the latest Nightly and restarted fxmetro, ensured that the "Give Feedback" tile didn't appear as per comment #9

Found and created Bug #978550 when going through the verification process.