1343236 - contextMenus.OnClickData should have the linkText of the link

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[kernp25]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170227030203



Actual results:

No linkText in contextMenus.OnClickData


Expected results:

contextMenus.OnClickData should have the linkText of the link

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I also face to this problem when I port my extension to WebExtensions.

In XUL add-on era, there is `gContextMenu.linkTextStr` to get the link text. But it's not in WebExt API.

Comment 2

[kernp25]

This popular add-on also uses `gContextMenu.linkTextStr`:
https://addons.mozilla.org/firefox/addon/colt/

Comment 3

[Caitlin Neiman [:caitmuenster]]

Hi all, this has been added to the agenda for the May 16 WebExtensions API triage. Kernp25 and Tetsuharu, would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage

Agenda: https://docs.google.com/document/d/1vrhHNOelBty4hXcjQ8VbFk-azHRFFDVyGka7H0VpEa8/edit#

Comment 4

[kernp25]

(In reply to Caitlin Neiman (http://pronoun.is/she) from comment #3)
> Hi all, this has been added to the agenda for the May 16 WebExtensions API
> triage. Kernp25 and Tetsuharu, would you be able to join us? 

I don't think i'm able to join, sorry :(

But here are my thoughts about it:

If the browser already provides the linkText of the link, why not add it to contextMenus.OnClickData?
I don't think this is much trouble.

Because, otherwise we have to call these functions our self again (in content-script):
1) getLinkText [1]
2) gatherTextUnder [2]

We also must do this (to get the link that was clicked):
> while (srcElement && getLinkURL(srcElement) != request.linkUrl) {
>   srcElement = srcElement.parentElement;
> }

I copied the getLinkURL [3] function and modified it a bit:
> function getLinkURL(node) {
>   var href = node.href;
>   if (href)
>     return href;
> 
>   href = node.getAttribute("href") ||
>          node.getAttributeNS("http://www.w3.org/1999/xlink", "href");
> 
>   if (!href || !/\S/.test(href)) {
>     return "";
>   }
> 
>   var url = new URL(href, node.baseURI);
>   return url.toString();
> }

I'm also thinking for performance reasons this is really bad.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1711
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#634
[3] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1676

Comment 5

[kernp25]

I think this is also useful for some translate add-ons to translate the linkText of the link.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

We discussed about this during the last community API triage meeting and we agreed to approve this feature request.

The rationale is that, even if the Bug 1325814 is going to provide the CSS selector of the clicked element and an extension would be able to use it to retrieve the needed webpage content from its own content script, we already have the requested content available in the internals and the use case is common enough to prefer to not force an extension to inject a content script just to retrieve it.

Comment 7

[Thomas Wisniewski]

Attachment: 1343236-add_support_for_linkText_to_contextMenus.OnClickData.diff

Will this patch do? It works as expected for me with a simple test addon, but I don't see any existing tests for related properties like linkUrl, and I'm not sure if that's an oversight or not. I can at least do a try run if we'd like?

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Thomas Wisniewski from comment #7)
> Will this patch do? It works as expected for me with a simple test addon,
> but I don't see any existing tests for related properties like linkUrl, and
> I'm not sure if that's an oversight or not. I can at least do a try run if
> we'd like?

Yes, that's an oversight, you should add a test to [1].  Also, you need to add the property to the JSON schema [2].

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/menus.json#75

Comment 9

[Thomas Wisniewski]

Attachment: 1343236-add_support_for_linkText_to_contextMenus.OnClickData.diff

Thanks :zombie! Here's a fresh patch with those changes, try seems fine with it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc079c89220084f165153ffd98fbebda4dd9032

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8879770 [details] [diff] [review]
1343236-add_support_for_linkText_to_contextMenus.OnClickData.diff

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bc9a8ef370
Add support for linkText to contextMenus.OnClickData. r=mixedpuppy

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3bc9a8ef370

Comment 13

[Rob Wu [:robwu]]

Note: linkText is set to the link's URL if the link has no text (e.g. <a href><img></a>).

Source:
https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/modules/ContextMenu.jsm#956
https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/modules/ContextMenu.jsm#304-315

Comment 14

[Will Bamberg [:wbamberg]]

I've updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/OnClickData for this.

Marking as dev-doc-complete, but let me know if we need anything else here.