'Search for' context menu item should quote selected text

RESOLVED FIXED

Status

Firefox for Metro
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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 "..".

Updated

5 years ago
Whiteboard: feature=work
(Assignee)

Updated

5 years ago
Duplicate of this bug: 843988
(Assignee)

Comment 2

5 years ago
According to bug 844371, this should only be on content text. The search option should not show up on selected text in text inputs.
(Assignee)

Comment 3

5 years ago
Created attachment 719108 [details] [diff] [review]
patch

This quotes and if needed trims the search string, and also insures the menu option does not show up in text inputs.
Attachment #719108 - Flags: review?(fyan)
(Assignee)

Comment 4

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

Comment 5

5 years ago
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.
Attachment #719108 - Flags: review?(fyan) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 719136 [details] [diff] [review]
patch v.2

- updated per comments.
Attachment #719108 - Attachment is obsolete: true
Attachment #719136 - Flags: review?(fyan)

Comment 7

5 years ago
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?
Attachment #719136 - Flags: review?(fyan) → review+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/46c7aef70f6d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.