search

brettz9 / jump-to-anchor

Jump to the closest anchor for a selected element
MIT License
9 stars 2 forks source link

Convert to WebExtensions #8

Closed jomo closed 6 years ago

jomo commented 6 years ago

Fixes #7. Also changed:

Sorry it's one single commit, but I was pretty much touching every file already.

jomo commented 6 years ago

@brettz9 any chance this will be merged?

brettz9 commented 6 years ago

My apologies, I hadn't been getting email notifications, so I unfortunately missed your info here.

I intend to take a look when I have a little time, hopefully within the next week or so. Let me know if you have any other changes coming through the pipeline...

brettz9 commented 6 years ago

This is looking very good--I have a couple questions...

  1. Are there any browsers you know of supporting WebExtensions and the non-standard document.caretRangeFromPoint but aren't supporting the standard document.caretPositionFromPoint?
  2. By "Fixed selected text detection", are you referring to avoiding the Caret selection? Otherwise, I didn't see any fixes per se, except for some useful optimizations.

Also, IIRC, the reason I had two context menu entries before was because I found a good number of times that I had forgotten that I had some text highlighted on a page and I would then inadvertently give myself the anchor relative to that point. I might try it again this way with just one selection and see whether I feel I need to add back the extra menu item or not.

And I seem to remember having tried the "contextmenu" event over the "click" event myself, but maybe it just wasn't working for the Addons SDK and now it is ok with WebExtensions...

brettz9 commented 6 years ago

I've made a number of changes, mostly smaller ones, though more significantly I switched our using the heavier content_script privileges to the lesser activeTab privilege, and then (also adding the webextension-polyfill), conditionally injecting via executeScript. It turns out we also don't need the tabs privilege, as that is only needed for privileged portions of that API (which we don't need).

Would you take a look at my "webext" branch and see if that works for you so I can release?

brettz9 commented 6 years ago

@jomo : Please let me know if you intend to review and confirm it's working for you, as I may go ahead with a release if I don't hear back.

jomo commented 6 years ago

Sorry for the late reply.

browsers supporting document.caretRangeFromPoint but not document.caretPositionFromPoint?

Good point. Theoretically Google Chrome 43 - 52, guess we can ignore that.
Also, it looks like Firefox has completely dropped support for this.

"Fixed selected text detection"

I probably meant that selection is only used when there is an actual selection range, not just a selection caused by clicking on the page.

The changes in the webext branch look fine to me, I've tested the plugin and it seems to be working as it should.

brettz9 commented 6 years ago

@jomo : I've unfortunately realized that my solution with activeTab privileges is inadequate. I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1437497 so that I can use it. I may see about reverting this functionality to use a content script for the time being. :-(

rugk commented 6 years ago

Great work, here! So does it work with Firefox 58 (even without that last bug you've filled)? So can we expect a release (on AMO), soon?

brettz9 commented 6 years ago

Yes, it's working with Firefox 58 and available now on https://addons.mozilla.org/en-US/firefox/addon/jump-to-anchor/ .

I'm currently trying to tweak it to work fully in Chrome, as Chrome is having a problem with the SVG icon. When I may get it working, I intend to post back here for any interested.

But it's all good now for Firefox, though with the unfortunate requirement that you have to agree to content script privileges and have the code injected into every page rather than only when you use it.

rugk commented 6 years ago

It works, great!

brettz9 commented 6 years ago

Available for Chrome now as well at https://chrome.google.com/webstore/detail/jump-to-anchor/fhbjjkmbahpmoegppmljagmakkeomlmb

brettz9 commented 6 years ago

And very many thanks to @jomo for doing all the work of migrating into the new format! If the manifest starts accepting multiple authors, I'll gladly add you there...

Btw, it turns out document.caretRangeFromPoint was needed after all--it appears MDN might be wrong about Chrome support as document.caretPositionFromPoint turns up undefined and there's an open issue for it: https://bugs.chromium.org/p/chromium/issues/detail?id=388976

brettz9 commented 6 years ago

Btw, I've just published an update (approved on AMO and at the Chrome Web Store) which avoids adding, regardless of user setting, the selection context menu if no selection has been made. Because this feature is now available, I've changed the default for the option to be true, but people can still opt back to having only a single menu item regardless. I think this way is better for discoverability (~even if it is mildly annoying as a default on Chrome since Chrome always ends up with a selection~).

brettz9 commented 6 years ago

Actually, I pushed a fix and improvement to the update I had mentioned a few hours ago (also available on both stores already) which makes the default on Chrome (and any other browsers besides Firefox) different from Firefox. So I think things are now where they should be.

rugk commented 6 years ago

Thanks, that looks quite good. However, is it possible to not put that into a submenu? So I have these two items directly without looking into the submenu?