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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Screen Shot 2015-12-28 at 15.24.39.png

See attached screenshot.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Yeah it's a known issue, I'm currently doing a refactoring on contextMenus, I'll fix this soon too after landing those patches.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: Handling %s in contextMenus. v1

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?

Comment 4

[Kris Maglione [:kmag]]

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...'.

Comment 5

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81e407daa222

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81e407daa222

Comment 8

[Vasilica Tamas, QA [:vtamas]]

Is manual testing necessary here? If yes, could you provide the proper webextension?

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: 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.

Comment 10

[Andreas Tolfsen ❲:ato❳]

FWIW I should add I tested this in Firefox Nightly 48.0a1 (2016-04-03) and it works as expected there.

Comment 11

[Vasilica Tamas, QA [:vtamas]]

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.