hnakamur / FormatLink-Firefox

A Firefox web-extension to copy the URL and the title or the selected text to clipboard
https://addons.mozilla.org/ja/firefox/addon/format-link3/
MIT License
45 stars 8 forks source link

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

Open Radagast opened 7 years ago

Radagast commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 https://github.com/hnakamur/FormatLink-Firefox/blob/d494ccf41df63fbd22350804867baafc932ac8b8/background.js#L34

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 commented 7 years ago

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 :disappointed: Maybe filing a bug anyway, in the hope they show at least an alternative approach?

IzzySoft commented 7 years ago

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 commented 7 years ago

@IzzySoft Thanks for your info and code. I modified your code and created and merged the pull request https://github.com/hnakamur/FormatLink-Firefox/pull/6 I just submitted Format Link version 1.3.2 for the review.

IzzySoft commented 7 years ago

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 commented 7 years ago

@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. https://github.com/hnakamur/FormatLink-Firefox/pull/7

Could you test it again? Thanks!

IzzySoft commented 7 years ago

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. :+1:

hnakamur commented 7 years ago

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 commented 7 years ago

Typical win-win situation then :grin:

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 :rofl:

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 :wink:

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

hnakamur commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

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":

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

cmcqueen commented 5 years ago

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.