Closed Bug 970746 Opened 6 years ago Closed 6 years ago

Context menu should include option to search for link text on search engine

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30

People

(Reporter: vtsatskin, Assigned: vtsatskin)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch contextMenuSearchOnLinks.patch (obsolete) — Splinter Review
Steps to reproduce:

1. Find a hyperlink on a webpage
2. Right-click it
3. No 'Search <engine> for "<selection or link text>"' option exists like when selecting text

Expected Behavior: 

The option to search the link's text in your search engine should be available.
Attachment #8373803 - Flags: review?(bmcbride)
Status: NEW → ASSIGNED
Comment on attachment 8373803 [details] [diff] [review]
contextMenuSearchOnLinks.patch

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

Tests?

::: browser/base/content/browser-context.inc
@@ +292,5 @@
>                  label="&keywordfield.label;"
>                  accesskey="&keywordfield.accesskey;"
>                  oncommand="AddKeywordForSearchField();"/>
>        <menuitem id="context-searchselect"
> +                oncommand="BrowserSearch.loadSearchFromContext(this.searchTerms);"/>

Where does searchTerms come from?

::: browser/base/content/nsContextMenu.js
@@ +281,5 @@
>      this.showItem("frame", this.inFrame);
>  
> +    let showSearchSelect = isTextSelected || this.onLink;
> +    this.showItem("context-searchselect", showSearchSelect);
> +    if(showSearchSelect) {

Nit: Space after if

@@ +1445,2 @@
>    isTextSelection: function() {
> +    return getBrowserSelection().length != 0;

Refactor this so getBrowserSelection() only needs to be called once - at the moment you have it so every time the menu opens it gets called here and in formatSearchContextItem(), but it can be an expensive call.

@@ +1663,5 @@
> +
> +  // Formats the 'Search <engine> for "<selection or link text>"' context menu.
> +  formatSearchContextItem: function() {
> +    var menuItem = document.getElementById("context-searchselect");
> +    var selectedText = this.onLink ? this.linkText() : getBrowserSelection();

Also, passing in a character limit param to getBrowserSelection() can make it a bit faster.

@@ +1686,5 @@
> +    var menuLabel = gNavigatorBundle.getFormattedString("contextMenuSearch",
> +                                                        [engineName,
> +                                                         selectedText]);
> +    menuItem.label = menuLabel;
> +    menuItem.accessKey = gNavigatorBundle.getString("contextMenuSearch.accesskey"); 

Nit: Fix trailing whitespace since you're already modifying this line?
Attachment #8373803 - Flags: review?(bmcbride) → review-
This patch addresses the issues Unfocused mentioned in the feedback.

Within the nsContextMenu object, I've opted to have this.isTextSelected be the preferred way of checking if there is text selected. In addition, there is a new variable called this.textSelected which contains the selected text. These two values are ensured to only be calculated once and used elsewhere in the object. This ensures there aren't any extra calls the expensive getBrowserSelection() function.

> ::: browser/base/content/browser-context.inc
> @@ +292,5 @@
> >                  label="&keywordfield.label;"
> >                  accesskey="&keywordfield.accesskey;"
> >                  oncommand="AddKeywordForSearchField();"/>
> >        <menuitem id="context-searchselect"
> > +                oncommand="BrowserSearch.loadSearchFromContext(this.searchTerms);"/>
> 
> Where does searchTerms come from?

This comes from formatSearchContextItem setting value of searchTerms. It was chosen to be added as a property because I could not figure out a good way of sharing the search terms between the nsContextMenu and the menu item's oncommand listener. If you have any suggestions on how this should be done, I would gladly like to hear.
Attachment #8376025 - Flags: review?(bmcbride)
Comment on attachment 8376025 [details] [diff] [review]
contextMenuSearchOnLinks.feedback.patch

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

Can you post the full diff? It's hard to review just a diff of a diff - generally reviewers want each revision to be a complete standalone diff, to see the whole picture. We have tools to get an interdiff (diff between two revisions of a patch) when it's useful.
Attachment #8376025 - Flags: review?(bmcbride)
Attached patch contextMenuSearchOnLinks.patch (obsolete) — Splinter Review
Attachment #8373803 - Attachment is obsolete: true
Attachment #8376025 - Attachment is obsolete: true
Attachment #8378059 - Flags: review?(bmcbride)
Comment on attachment 8378059 [details] [diff] [review]
contextMenuSearchOnLinks.patch

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

Good to go, with just one fixup:

::: browser/base/content/nsContextMenu.js
@@ +543,5 @@
>      this.isDesignMode      = false;
>      this.onCTPPlugin       = false;
>      this.canSpellCheck     = false;
> +    this.textSelected      = getBrowserSelection();
> +    this.isTextSelected    = this.textSelected.length != 0;

Move setting these two to where isTextSelected was originally set in initMenu() - some add-ons use isTextSelected, so better off not changing when we set it.
Attachment #8378059 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] from comment #6)
> Move setting these two to where isTextSelected was originally set in
> initMenu() - some add-ons use isTextSelected, so better off not changing
> when we set it.

Er, heh, please ignore this, it's late.
Depends on: 976906
Currently blocked due to try access required to test fixes to tests that broke.
Turns out some of the tests needed updated.

I've also made it so the context menu item does not appear when right clicking a image that is nested inside of a link.

I ran a subset of the tests on the platforms that were failing on the try server: https://tbpl.mozilla.org/?tree=Try&rev=3359e3451219
Attachment #8378059 - Attachment is obsolete: true
Attachment #8385644 - Flags: review?(bmcbride)
Attachment #8385644 - Flags: review?(bmcbride) → review+
Attachment #8385644 - Flags: checkin?(bmcbride)
Summary: Context menu should include option to for search link text on search engine → Context menu should include option to search for link text on search engine
Comment on attachment 8385644 [details] [diff] [review]
contextMenuSearchOnLinks.patch

In the future, please just set the checkin-needed bug keyword when your patch is ready to land (the checkin? flag is intended for rarer situations).

Also, please make sure your hg commit information includes your full email address.
Attachment #8385644 - Flags: checkin?(bmcbride) → checkin+
https://hg.mozilla.org/integration/fx-team/rev/120a6f93c1d4
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/120a6f93c1d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Blocks: 985824
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.