Closed Bug 963688 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: mfinkle, Assigned: sfoster)

References

Details

(Whiteboard: [release28] p=1 s=it-30c-29a-28b.2 r=ff28)

Attachments

(2 files, 2 obsolete files)

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
Hi Flod, would this require localization?
Flags: needinfo?(francesco.lodolo)
Whiteboard: [triage] [feature] p=0
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.
Flags: needinfo?(francesco.lodolo)
(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.
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)
> 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?).
%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.
(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.
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.
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.
... alternatively, I wonder if the snippet service is specific enough to serve a "give us feedback on metro"?
(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.
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.
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!
Whiteboard: [triage] [feature] p=0 → [release28] [feature] p=0
Whiteboard: [release28] [feature] p=0 → [release28] [feature] p=1
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
Keywords: uiwanted
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: jbecerra
Whiteboard: [release28] [feature] p=1 → [release28] [feature] p=1 s=it-30c-29a-28b.1
Target Milestone: --- → Firefox 28
Attached patch feedback-bookmark (obsolete) — Splinter Review
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?
Attachment #8370344 - Flags: feedback?(mbrubeck)
Attachment #8370344 - Flags: feedback?(mark.finkle)
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.
Attachment #8370344 - Flags: feedback?(mbrubeck) → feedback+
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
Attachment #8370344 - Attachment is obsolete: true
Attachment #8370344 - Flags: feedback?(mark.finkle)
Attachment #8370389 - Flags: review?(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.
Attachment #8370389 - Flags: review?(mbrubeck) → review+
Here's how it looks on first run
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.
(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.
We already have a link to "Help (online)" so we could add a link there to feedback.
Depends on: 969045
Whiteboard: [release28] [feature] p=1 s=it-30c-29a-28b.1 → [release28] p=1 s=it-30c-29a-28b.1 r=ff28
Target Milestone: Firefox 28 → ---
Is this ready to land?
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.
Whiteboard: [release28] p=1 s=it-30c-29a-28b.1 r=ff28 → [release28] p=1 s=it-30c-29a-28b.1 r=ff28 [triage]
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!
Whiteboard: [release28] p=1 s=it-30c-29a-28b.1 r=ff28 [triage] → [triage] [release28] p=1 r=ff28
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.
(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?
Let's also get the patch landed on Nightly (m-c) ASAP. There are no string concerns there and we want the bookmark.
(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
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?
(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.
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.
OS: Linux → Windows 8.1
Whiteboard: [triage] [release28] p=1 r=ff28 → [triage] p=1
Whiteboard: [triage] p=1 → [release28] p=1
Whiteboard: [release28] p=1 → [release28] s=it-30c-29a-28b.2 p=1
Whiteboard: [release28] s=it-30c-29a-28b.2 p=1 → [release28] p=1 s=it-30c-29a-28b.2 r=ff28
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)
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.
Attachment #8370389 - Attachment is obsolete: true
Attachment #8380798 - Flags: review?(mbrubeck)
Attachment #8380798 - Flags: review?(mbrubeck) → review+
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.
Keywords: uiwantedleave-open
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?
Flags: needinfo?(mark.finkle)
ah, MWC. Can you direct me Matt?
Flags: needinfo?(mark.finkle) → needinfo?(mbrubeck)
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?
Flags: needinfo?(mbrubeck) → needinfo?(francesco.lodolo)
(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)
Flags: needinfo?(francesco.lodolo)
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.
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Keywords: leave-open
Target Milestone: --- → Firefox 30
Blocks: 1002650
You need to log in before you can comment on or make changes to this bug.