Closed Bug 1235376 Opened 7 years ago Closed 7 years ago

Selection context menu does not replace %s format specifier with selection text

Categories

(WebExtensions :: Untriaged, defect)

x86_64
macOS
defect
Not set
normal

Tracking

(firefox46 verified, firefox48 verified)

VERIFIED FIXED
mozilla46
Tracking Status
firefox46 --- verified
firefox48 --- verified

People

(Reporter: ato, Assigned: gkrizsanits)

Details

Attachments

(3 files)

The %s format specifier is according to https://developer.chrome.com/extensions/contextMenus#method-create supposed to be replaced with the selectionText if a context menu item’s "contexts" array contains "selection".

Example:

    var translateSelection = ev => console.log(ev.selectionText);

    chrome.contextMenus.create({
      title: "Translate “%s”",
      contexts: ["selection"],
      onclick: translateSelection
    });

On Firefox Nightly 46.0a1 (2015-12-18), "%s" is left untouched.  If selecting a text "foo" in content, I’d expect "foo" to appear in the context menu as "Translate “foo”", instead "Translate “%s”" is displayed.
See attached screenshot.
Yeah it's a known issue, I'm currently doing a refactoring on contextMenus, I'll fix this soon too after landing those patches.
Assignee: nobody → gkrizsanits
It feels to me that we should have a max length for menu item labels so I've added that too. Kris, what do you think?
Attachment #8702868 - Flags: review?(kmaglione+bmo)
Comment on attachment 8702868 [details] [diff] [review]
Handling %s in contextMenus. v1

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> It feels to me that we should have a max length for menu item labels so I've
> added that too. Kris, what do you think?

I agree, but I think it would probably be better to leave it to the layout
engine, and just limit the length of the selection text here, so something
like 'Translate "%s"' would become 'Translate "foo bar..."' rather than
'Translate "foo bar...'.
Attachment #8702868 - Flags: review?(kmaglione+bmo) → review+
(In reply to Kris Maglione [:kmag] from comment #4)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> > It feels to me that we should have a max length for menu item labels so I've
> > added that too. Kris, what do you think?
> 
> I agree, but I think it would probably be better to leave it to the layout
> engine, and just limit the length of the selection text here, so something
> like 'Translate "%s"' would become 'Translate "foo bar..."' rather than
> 'Translate "foo bar...'.

Yes.  The most obvious behaviour to me would be to leave the overall length of the context menu item to be decided by whatever the toolkit’s default hard limit is, and not to apply a special restriction to a string that happens to have a %s substitution in it.

Limiting the length of the substitution explicitly seems like a much nicer approach.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/81e407daa222
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Is manual testing necessary here? If yes, could you provide the proper webextension?
Flags: needinfo?(ato)
Attached file format-specifier.xpi
Attaching an extension you can use to test this.  Install, select any text in a content document, and verify that a "Translate “selected text”" context menu item is added on right-click.  The "selected text" above should be replaced by what you select and not exceed 64 characters.
Flags: needinfo?(ato)
FWIW I should add I tested this in Firefox Nightly 48.0a1 (2016-04-03) and it works as expected there.
Thanks Andreas!

I was able to reproduce the initial issue on Firefox 46.0a1 (2015-12-28).

Confirm as fixed on Firefox 48.0a1 (2016-04-04) and Firefox 46 beta 6 under Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 12.04 32-bit.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.