[Affected versions]: Firefox 47.0a1 (2016-02-04) [Affected platforms]: Ubuntu 14.04 32-bit Windows 10 64-bit Mac OS X 10.11.1 STR 1.Launch Firefox with clean profile. 2.Click menu [≡] → Synced tabs → Sign in to sync. 3.Login using a Firefox account. 4.Click menu [≡] → Synced tabs → View synced tabs sidebar. 5.Right click on one of the tabs from the list. ER The context menu contains the following options: - Open - Open this Tab in a new Tab - Open this Tab in a new Window - Open this Tab in a Private Window - Bookmark this Tab - (Copy) - Refresh List AR Some of the above mentioned options are not displayed in Synced tabs sidebar context menu. See screenshot: http://i.imgur.com/VTOtXNS.jpg

Review commit: https://reviewboard.mozilla.org/r/39321/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39321/

Comment on attachment 8729262 [details] MozReview Request: Bug 1246156 - Show additional link options in the synced tabs sidebar context menu. r?markh https://reviewboard.mozilla.org/r/39321/#review36071 My only issues are with the strings - could you please attach a screenshot and needinfo rfeeley for the strings (and as mentioned, if we do end up using identical strings to bookmarks, we should then needinfo :flod and explain the situation and see if he says we can use those places strings directly or if we should copy them? ::: browser/components/syncedtabs/TabListView.js:316 (Diff revision 1) > + initiatingDoc: event.target.ownerDocument, If I'm reading openUILinkIn correctly, initiatingDoc is only used for where="save", which we aren't doing, so can we kill this? ::: browser/locales/en-US/chrome/browser/browser.dtd:807 (Diff revision 1) > -<!ENTITY syncedTabs.context.openTab.label "Open This Tab"> > +<!ENTITY syncedTabs.context.openTab.label "Open Tab"> We can't change existing strings - we must use a new ID ::: browser/locales/en-US/chrome/browser/browser.dtd:809 (Diff revision 1) > -<!ENTITY syncedTabs.context.bookmarkSingleTab.label "Bookmark This Tab…"> > +<!ENTITY syncedTabs.context.openTabInNewTab.label "Open Tab in New Tab"> I'm not sure we want "Tab" here (and the other strings) - "Open Tabs in New Tab" is particularly grating (and is probably missing an "a"). I think we probably need rfeeley to help us out here, but my first reaction is that the same strings as used by the bookmarks sidebar would make sense - just "Open", "Open in a New Tab" etc (and if we did decide to use identical strings we probably need to check whether we can reuse those strings directly from places.dtd, or whether we need to duplicate them) Simlarly for the accesskey - I feel we should definately use the same as bookmarks if possible eg, "Open in a New Tab" uses "w" as the access key - it would help with muscle-memory. ::: browser/locales/en-US/chrome/browser/browser.dtd:815 (Diff revision 1) > +<!ENTITY syncedTabs.context.bookmarkSingleTab.label "Bookmark This Tab…"> hmm - mozreview is showing this string as being changed too, but that doesn't seem to be the case. Maybe that's just a mozreview quirk ::: browser/locales/en-US/chrome/browser/browser.dtd:817 (Diff revision 1) > +<!ENTITY syncedTabs.context.copyTabLocation.label "Copy Tab Location"> similarly here, I'd have thought "Copy" would be fine, just like bookmarks - but yeah, rfeeley :)

https://reviewboard.mozilla.org/r/39321/#review36071 > If I'm reading openUILinkIn correctly, initiatingDoc is only used for where="save", which we aren't doing, so can we kill this? Oops, meant to remove that. Thanks! > We can't change existing strings - we must use a new ID I see. So we can only remove old strings or add new ones? > I'm not sure we want "Tab" here (and the other strings) - "Open Tabs in New Tab" is particularly grating (and is probably missing an "a"). I think we probably need rfeeley to help us out here, but my first reaction is that the same strings as used by the bookmarks sidebar would make sense - just "Open", "Open in a New Tab" etc (and if we did decide to use identical strings we probably need to check whether we can reuse those strings directly from places.dtd, or whether we need to duplicate them) > > Simlarly for the accesskey - I feel we should definately use the same as bookmarks if possible eg, "Open in a New Tab" uses "w" as the access key - it would help with muscle-memory. Ah, reusing the bookmarks strings and access keys makes more sense. I based these on the ones for links ("Open Link in..."), but I'm not fond of "Open Tab in New Tab," either. I'll upload a screenshot for :rfeeley and :flod's perusal. > hmm - mozreview is showing this string as being changed too, but that doesn't seem to be the case. Maybe that's just a mozreview quirk I changed the spacing between the entity name and value.

Ryan, here are the new strings for the sidebar context menu. Do you think we can remove some of the echoing and just say "Open," "Open in a New Tab," and so on?

Comment on attachment 8729301 [details] Screen Shot 2016-03-10 at 6.02.34 PM.png Good idea to remove the echoing "tab". The context is there.

Aside: https://bugzilla.mozilla.org/show_bug.cgi?id=1257613 seems like it will be relevant at some point in the future.

flod, would you object to reusing the Places strings in https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.dtd for this? Some background: we'd like to show actions in the menu when the user right-clicks a synced tab in the sidebar. Even though Synced Tabs aren't part of Places, I think the context is similar: a list of links that can be opened, bookmarked, and copied. Does it make sense to use these strings for consistency in the UI, or would you prefer separate entities with the same text?

I would use different strings for at least a couple of reasons: * Accesskeys: the complete menu will be different, this one is shorter and localizers might be able to pick better accesskeys. * Less chance of confusion (pieces of menu coming from one file, others coming from a completely different one). Also really hard to figure out accesskey conflicts. Translation tools should be able to provide the existing translation as suggestion given it's the same text. As for string changes: you can't update an existing string unless it's a minor typo that only affects English (it's not the case for "Open Tab"). https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings You can change an accesskey since each locale has its own, but you should try to keep the connection between string and accesskey. Practical examples: * If 'syncedTabs.context.openTab.label' becomes 'syncedTabs.context.openTab2.label', the accesskey should become 'syncedTabs.context.openTab2.accesskey'. This helps tools matching string and accesskey. * If you need to change 'syncedTabs.context.openTab.accesskey' from 'O' to 'T', no need to change the string ID. That change affects only English.

Comment on attachment 8729262 [details] MozReview Request: Bug 1246156 - Show additional link options in the synced tabs sidebar context menu. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39321/diff/1-2/

Thanks, flod! Incorporated your feedback to use different strings, and rfeeley's feedback to remove the echoing "tab". https://treeherder.mozilla.org/#/jobs?repo=try&revision=7956d5be0679

Comment on attachment 8729262 [details] MozReview Request: Bug 1246156 - Show additional link options in the synced tabs sidebar context menu. r?markh https://reviewboard.mozilla.org/r/39321/#review40761

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Nightly 48.0a1 (buildID: 20160413030239) - context menu options are: Open, Open in a New Tab, Open in a New Window, Open in a New Private Window, Bookmark This Tab, Copy, Sync Now.

Hi Mark, Kit, should we consider uplifting this to Beta47? SV suggested it and I wanted to get your opinion on the risks.

Hi Ritu! I think it's a low risk change with good test coverage, but it has several new strings. Is there a policy for uplifting those?

(In reply to Kit Cambridge [:kitcambridge] from comment #17) > Hi Ritu! I think it's a low risk change with good test coverage, but it has > several new strings. Is there a policy for uplifting those? Policy is no new strings uplifted :/