Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copying links other than text doesn't appear to be working? #5

Open
Radagast opened this issue Jun 19, 2017 · 14 comments
Open

Copying links other than text doesn't appear to be working? #5

Radagast opened this issue Jun 19, 2017 · 14 comments

Comments

@Radagast
Copy link

Thanks for fixing the missing context menu options on Nightly. Unfortunately, copying links in anything other than plain text doesn't appear to be working. For example, if I select the FormatLink-Firefox at the top of this page and select copy as markdown, nothing gets added to the clipboard and hence there's nothing to paste.

Linux/Nightly.

@IzzySoft
Copy link

Same behavior for me with all links I've tried on AMO. The only place where it worked for me there was to copy the page link (when invoked from either the tab's context menu or the icon it places next to the address bar). So this seems to apply to AMO only, but works fine on all other sites I've tried so far.

For the books: I've had the very same issues with Copy URL, so this could be a bug either in WebExtensions, or in the AMO page design (or in both, or AMO could willingly have done so).

BUT: while on sites other than AMO the URL was captured fine, the link title was always set to that of the page the link was on – not to the link description (i.e. {{text}} is always replaced by the page title). Unless I've "marked" ("selected") the entire URL manually beforehand (ah, I see: that must be this code; is this a limitation by WebExt? Can't one get the URL text (a.nodeValue) from the same place you get the link (a.href) from? Always having to select the link first is a bit annoying). Using v1.3.1 here, in case it matters.

@Radagast can you confirm this?

Looks like this could become a replacement for CoLT when legacy addons are shot down with FF57 – provided the {{text}} issue is solved. Funnily, CoLT has no issues with the AMO site.

(PS: AMO = Addons.Mozilla.Org)

@hnakamur
Copy link
Owner

@IzzySoft Thanks for your comment. Your observations are the same as mine. I described them at "KNOWN LIMITATIONS" in https://addons.mozilla.org/ja/firefox/addon/format-link3/

I believe these are limitations of the web extension APIs.

For context menus, I use
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/onClicked
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/OnClickData
at

browser.contextMenus.onClicked.addListener((info, tab) => {

But the info.selectionText is empty if you just right click on a link. As you say, if you select the link text manually, then info.selectionText is set to the text.

@IzzySoft
Copy link

IzzySoft commented Aug 22, 2017

Thanks for your feedback, @hnakamur – I vaguely remembered having read about some such limitation but didn't remember where (Mozilla drives me crazy with their breaking everything every few years; I'm using Firefox since back when it was called Phoenix, and only didn't switch because I haven't yet found a suitable alternative – so this is about the 3rd time I've got to completely "restock" my extensions).

With the WebExt API still not being "finished" by them, is there any chance this limitation might be removed in the future? I'm not a FF or Chrome dev and not familiar with it, but I would expect even an API as limited as that one should be able to retrieve the link's nodeValue as well – so my hope is this is something they'll add?

Btw: As you can get the URL by info.linkUrl, maybe the nodeValue is stored in the info object as well. Guess you've already tried that, but did console.log(info) yield something helpful? (I do some basic Javascript from time to time, and found console.log() often revealed helpful things)

Strike the last paragraph. No nodeValue/title/whatever available at the moment, I've just checked. And it seems unlikely to be added, as even the original Chrome API Mozilla is "cloning" doesn't have it. Sad. The only way to get those details really is selectionText 😞 Maybe filing a bug anyway, in the hope they show at least an alternative approach?

@IzzySoft
Copy link

PS: @hnakamur you might wish to check into this bugzilla issue. Is exactly about the "link title", and still open. As for the "alternative approach" I mentioned in my previous comment, it has even that:

function menuClick(info, tab) {
  let url = new URL(info.linkUrl);
  browser.tabs.executeScript(tab.id, {
    code: `
      var link = document.querySelector('a[href="${url.pathname}"]');
      ....
    `
  });

(if the same link exists multiple times, as a work-around you could just pick the first one. Shouldn't happen too frequently anyway, hopefully).

Chime in there, point out that your addon (and the other one I've mentioned I used to verify) would need that in contextMenus.OnClickData. Should rise its priority at least slightly.

@hnakamur
Copy link
Owner

@IzzySoft Thanks for your info and code. I modified your code and created and merged the pull request #6
I just submitted Format Link version 1.3.2 for the review.

@IzzySoft
Copy link

IzzySoft commented Aug 23, 2017

Thanks! Just tested it (loading it as "temporary extension" via about:debugging). Strange. Works for some links, but not for others. Reproducible:

  1. Go to the main page of your repo
  2. try these links:
    • MIT (the license in the header): Works as hoped for
    • the "LICENSE" file: Works as expected (title will be "MIT", as that's how the very same link appears first on the page)
    • top tabs (Code, Issues, Pull Requests): "Code" has "Github" as title, "Issues" works, everything else gets the page title
    • second row (commits … MIT): all except for "MIT" have the page title
    • all links from the file list have the page title
    • all links from within the README seem to work fine again

Same "mixed bag" on other sites. Tried at Stack Exchange: Links from the content work fine, from the headers not. Here at least is some clue: those which do not work have some event handler attached. Oh, guess what: for "click", so that could interfere (however, CoLT captures them correctly). Didn't see any event attached to those "failed links" on the Github page, though.

Some debug via Console (back again on your Github page) indeed matches that:

document.querySelector('a[href="https://github.com/hnakamur/FormatLink-Firefox/branches"]')
// returns: null
document.querySelector('a[href="https://github.com/hnakamur/FormatLink-Firefox/blob/master/LICENSE"]')
// <a href="https://github.com/hnakamur/FormatLink-Firefox/blob/master/LICENSE">

So for the failing links, document.querySelector does not return anything. Looking at the page source (Ctrl-U) they are actually not there it seems – that is, until you strip the https://github.com from the start!

Suggested fix: In background.js, extend the code: you pass to browser.tabs.executeScript and add an else, like

if (link) then link.innerText.trim() : "";
else {
  link = document.querySelector('a[href="${url}.pathname"]');
  link ? link.innerText.trim() : "";
}

(well, syntax error for the new URL – but you get what I mean). Basically: If the full URL was not found on the page, try again with the path only. But of course only if the hostname in the URL matches the hostname of the page.

I'm ready to test that again, just let me know when it's ready :)

@hnakamur
Copy link
Owner

@IzzySoft Thanks for your comment.
href may be relative URL such as ../foo, so I use selectQueryAll('a') and choose a link whose href property (which becomes a full URL) to the selected URL.
#7

Could you test it again?
Thanks!

@IzzySoft
Copy link

Fantastico! I was one step behind you (have tried in parallel, got the relative URLs working and was just about to deal with anchors as well – your fix already includes that. Strange that looping over all links (document.querySelectorAll('a')) always considers the full qualified link, while looking it up with document.querySelector('a[href="url"]') does not. Tested a bunch of links from my above list, none failed. Will test a bit more before giving a final answer, but it looks very good to me – thanks, and congrats! Now yours is the only WebExt that manages that (tried a few, all failed).

So: Works like a charm now – except for AMO pages, strangely, where it still doesn't catch a thing. Rushes right through tryToGetLinkSelectionText as if it would "return Promise…", though "text" is undefined. Tried selecting some text – and ran into an exception with that, claiming on line 78 that copyTextToClipboard would not be defined. With no text selected, it just rushes through and does nothing to the clipboard – last stop line 35, then straight to the end of that function. No idea what's behind that.

For now I'd suggest to publish the next version and leave the AMO issue for later (as that has its separate issue, guess this one can be considered solved and be closed now). Thanks a lot for your help! If I do the switch to FF57, this will now be my replacement for CoLT. 👍

@hnakamur
Copy link
Owner

Thanks for your test and comment.

I suppose document.querySelector('a[href="url"]') searches for a tags whose href attribute value written as is. On the other hand, HTMLHyperlinkElementUtils.href - Web APIs | MDN returns the whole URL.

Code for context menu and copying text to clipboard are copied from
https://github.com/mdn/webextensions-examples/tree/master/context-menu-demo
https://github.com/mdn/webextensions-examples/tree/master/context-menu-copy-link-with-types

This add-on injects JavaScript into web pages. The addons.mozilla.org domain disallows this operation, > so this add-on will not work properly when it's run on pages in the addons.mozilla.org domain.

To copy text to clipboard, you need to inject JavaScript to web pages, but you cannot do that in the addons.mozilla.org for security reason.

As for copyTextToClipboard, yes I see the error TypeError: copyTextToClipboard(...) is undefined in the console when I use dev tool debugger too.

I copied the code from
https://github.com/mdn/webextensions-examples/blob/d7bf874833d0f752e505047574594edf76ba5a07/context-menu-copy-link-with-types/background.js#L23-L42
and first it checks the copyTextToClipboard is defined and then define it if it is not defined yet.

I submitted the version 1.3.3 for the review.

Thanks a lot for your help, too!
I'm happy as a developer that this web extension is useful for you,
and also as a user myself now I don't need the link text manually!

@IzzySoft
Copy link

Typical win-win situation then 😁

As for AMO: the (legacy) addon CoLT can extract URLs fine from there. Guess one could play with parsing the DOM tree to see whether that's possible (and also working with AMO and other possibly "restricted sites"). For now I guess your addon will work on 95% of all sites or more, and that's a good thing! I've updated my Gist accordingly, naming your addon as only possible replacement for CoLT (as the other candidates fail on too many things). Yours has even an advantage over CoLT: I can chose to let the copied link have a different title by simply selecting some text from the page 🤣

One minor (cosmetic) suggestion (common.js, function createContextMenus): instead of "copy url in %s format" I'd rather name the menu items "as %s", looks cleaner:

Format Link -> as Default
            -> as Markdown
            -> as reST
            -> ...

Alternatively: "using %s format" (e.g. "using default format", "using Markdown format").

Nothing urgent, just cosmetical 😉

Finally, my big thanks for your great support! Need to remember to mention that in my Gist.

@hnakamur
Copy link
Owner

Thanks for your advice!
I renamed context menu item labels and submit it as version 1.3.4 for the review.
Also thanks for mentioning my support in your Gist!

@IzzySoft
Copy link

Thanks! Update arrived and looks great! Funnily I still found a few "misbehaving links" (unfortunately in a protected area you'll have no access to). If I figure what it is (and maybe cann present an openly accessible example) I'll let you know.

@IzzySoft
Copy link

Okay, checklist of my findings. Pages that do not work include:

  • links in mails displayed with Roundcube (no reaction at all, so regardless of format). If I "select" the link (ie. text marked), the text will be copied to the clipboard but never the link. I've set some breakpoints, but it seems the routines are not even called. Funny thing is the debugger doesn't even list background.js being available – only clipboardhelper.js, common.js and content.js are mentioned (but that doesn't seem to be the culprit, as it's neither visible here at Github where the addon works fine).
  • ownCloud files app: Links from the panel get the wrong title. This is "expected behavior" as they work via Javascript events, with their href attribute being identical – so the title is taken from the first link. Just mention it in case of "similar reportings".
  • Links in some pages of My Oracle Support, e.g. in SR comments. Same behavior as described with Roundcube above: links have hrefs, but FormatLink doesn't seem to be triggered at all, though it's available in the context menu.
  • Some Wordpress pages (e.g. this one)

Just removed some bullet points from above list. If someone reports "wrong titles" or "not working at all", there are a few cases where that is "expected behavior":

  • the second bullet point above
  • if any text on the page is selected, that's obviously used as title (face-palm – of course that's how it should be, but one must be aware of it. Takes some time getting used to 😉).
  • links are Javascript events (aka "OnClick") only, having no href
  • pages having right-click disabled (obviously we cannot hook into a non-existing context menu)
  • AMO

Will keep my eyes open. Apart from above "minor annoyances", it works pretty well now!

@cmcqueen
Copy link

It generally works fine for me on Windows. But on Linux, I find that it only works if I've never copied anything to the clipboard since logging in. Once I've copied something to the clipboard in another program, then Format Link stops working (clipboard still contains the previous thing copied). I am guessing this is a limitation of the Firefox WebExtensions implementation on Linux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants