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.

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?

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.

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.

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.

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

Pushed to fx-team branch: https://hg.mozilla.org/integration/fx-team/rev/3be3bb423482

Backed out in https://hg.mozilla.org/integration/fx-team/rev/7dd5c624d563 for mochitest-1 bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=35094522&tree=Fx-Team

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

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.