843014 - 'Search for' context menu item should quote selected text

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

Chatted with Yuan about this. Currently the context menu on selected text has a string in it:

Search (search engine) for "..."

the "..." should be the first fifteen characters of selection text plus "..".

Comment 2

[Jim Mathies [:jimm]]

According to bug 844371, this should only be on content text. The search option should not show up on selected text in text inputs.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

This quotes and if needed trims the search string, and also insures the menu option does not show up in text inputs.

Comment 4

[Jim Mathies [:jimm]]

(Note I have tests for this in bug 834370.)

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 719108 [details] [diff] [review]
patch

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

Thanks for taking this bug. :)

This shouldn't require two strings in browser.properties.
See nsContextMenu.js for string trimming and ellipsis localization:

Trim and append ellipsis like this:
http://dxr.mozilla.org/mozilla-central/browser/base/content/nsContextMenu.js#l1291

Define ellipsis like this:
http://dxr.mozilla.org/mozilla-central/browser/base/content/nsContextMenu.js#l28

Note for future improvements: Since we're creating our context menu from a richlistbox, we should be able to have the ellipsis automatically inserted using CSS `text-overflow: ellipsis`, but that requires some max-width setting and other overhead, so let's not worry about that for now.

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch v.2

- updated per comments.

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 719136 [details] [diff] [review]
patch v.2

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

r+ with comments addressed.

::: browser/metro/base/content/ContextCommands.js
@@ +17,5 @@
> +                               .getComplexValue("intl.ellipsis",
> +                                                Ci.nsIPrefLocalizedString)
> +                               .data;
> +    } catch (ex) { }
> +  },

Instead of requiring an init call, we can do:

get _ellipsis() {
  delete this._ellipsis;
  this._ellipsis = "\u2026";
  try {
    this._ellipsis = Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data;
  } catch (ex) { }
  return this._ellipsis;
}

::: browser/metro/base/content/browser.xul
@@ +577,5 @@
>            <richlistitem id="context-paste" type="paste" onclick="ContextCommands.paste();">
>              <label value="&contextTextPaste.label;"/>
>            </richlistitem>
> +          <!-- Search Bing for "(text..)", displayed on selected content text only -->
> +          <richlistitem id="context-search" type="selected-text,!input-text" onclick="ContextCommands.searchText(this);">

Firefox desktop, Chrome, and Safari all display this context menu item even for <input/> elements, but IE desktop and metro do not. Just a note that we're breaking consistency with our other products but gaining some consistency with IE.

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +10,5 @@
>  browser.search.order.2=Google
>  browser.search.order.3=Yahoo
>  
>  # l10n: search context menu item text will be: |Search (browser.search.defaultenginename) for ".."
> +browser.search.contextTextSearchLabel1=Search %S for "%S"

The convention is to use 2 here, instead of 1.
Could you also update the comment?

Comment 8

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46c7aef70f6d

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/46c7aef70f6d