To fix this someone would want to update the regular expression at https://searchfox.org/mozilla-central/rev/840881e1232f664a58b39caaae6284c7bcf121df/toolkit/modules/SelectionUtils.jsm#94-99

The attached 10-year-old patch would be a place to start, but I'd really want to see the regular expression split up - we can check for the ip address cases separately, for instance - because as it is, the regex is not very readable.

Hi there! I'd love to take this! I was just curious as to why we don't use the URL api, or am I being naive? (does it have to do with it being implemented in C++ and being a part of the dom?). Alternatively, could we use UrlbarTokenizer.jsm (https://searchfox.org/mozilla-central/source/browser/components/urlbar/UrlbarTokenizer.jsm) for SelectionUtils.jsm?

(In reply to gliu20 from comment #16)

Hi there! I'd love to take this! I was just curious as to why we don't use the URL api, or am I being naive?

I'm not sure I understand the suggestion. I think without a protocol, www.mozilla.org does not parse as a URL (ie if you do new URL("www.mozilla.org"), it throws an exception). If you force-add a protocol, practically anything parses as a URL (in particular, any single English word, when prefixed with http:// or https:// will parse as a URL, cf http://cheese is a perfectly valid URL). We don't want to show the link context menu items unless the selection really is plausibly a URL, so checking whether it is strictly parsable as a full URL would cause either false negatives or false positives, depending on which way we go.

(does it have to do with it being implemented in C++ and being a part of the dom?). Alternatively, could we use UrlbarTokenizer.jsm (https://searchfox.org/mozilla-central/source/browser/components/urlbar/UrlbarTokenizer.jsm) for SelectionUtils.jsm?

This is a good idea. I wasn't aware that module existed; it post-dates the filing of this bug and the regexes in the existing code.

We couldn't quite do that directly, because SelectionUtils.jsm is a toolkit module (so is also usable from Firefox for Android, Thunderbird, etc.) and UrlbarTokenizer is desktop-Firefox-only. We could talk to the address bar team if it's worth moving some of the functionality into a toolkit module, or conversely, we could potentially change the browser context menu code to just talk to UrlbarTokenizer directly, not using SelectionUtils for this usecase. Marco, WDYT?

The behavior of the UrlbarTokenizer is deeply customized to the urlbar needs.
If we want to extract some regex or code from it and put it into toolkit that's fine, then the tokenizer itself could use that code, but I'd be wary of reusing the module in a non-urlbar context.

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:jaws, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

(In reply to Marco Bonardo [:mak] from comment #18)

The behavior of the UrlbarTokenizer is deeply customized to the urlbar needs.
If we want to extract some regex or code from it and put it into toolkit that's fine, then the tokenizer itself could use that code, but I'd be wary of reusing the module in a non-urlbar context.

Can we extract the looksLikeUrl and/or looksLikeOrigin methods? Certainly the second one doesn't look particularly URL-bar-specific, but I'm not very familiar with the code - moving the first one would perhaps be cleaner than moving the second one because there's some overlap in the regexes they both rely on, but I guess if we move to e.g. BrowserUtils we could in principle make the UrlbarTokenizer just depend on BrowserUtils...

yes we could. What I meant mostly is that we made best guesses that usually work in the urlbar but may not be the best choice for everything. But probably the javadoc is already clear enough about that.

(In reply to Marco Bonardo [:mak] from comment #21)

yes we could. What I meant mostly is that we made best guesses that usually work in the urlbar but may not be the best choice for everything. But probably the javadoc is already clear enough about that.

OK, then this sounds workable - gliu, does that sound like something you'd be happy to work on?

Yeah for sure! It might take me a while though--I'm also working on 1118553, which is a bit more complex than what I've previously worked on.

Refactoring to a separate module to promote code reuse since
UrlbarTokenizer.jsm has useful utility functions and regexps
for determining whether a string looks like a url.

Depends on D143116

Depends on D143116

Hey gliu! Are you still working on this? Can we help you get unstuck in some way?

Hey Gijs! Thanks for checking in and for your patience. I was hoping to work on this between studying and working part-time, but it looks like I don't have much bandwidth. If others would like to take this, feel free to do so.

=== Info in case others want to take this ===
I'm not sure how stale the current commits are, but I think what's there should be current with all the changes I made. However, the module is in the old .jsm style modules (and I think that would need to be migrated to the new module system? (I haven't followed too closely with the new module system news))

In terms of what would be required to make this commit ready for merging:

• bring back using the word boundaries of selections as a heuristic for whether link selection was intentional to avoid regressing. (see lines 91 onward in the old SelectionUtils.jsm)
• implementing some sort of solution for failure in toolkit/modules/tests/browser/browser_BrowserUtils.js caused by a float being recognized as url due to changes in SelectionsUtils.jsm
• merging the test for browser/base/content/test/general/browser_plainTextLinks.js with browser_contextmenu_linkselect.js in D143117

The other aspect is updating the open dependencies on the patches.

The part that could be a bit tricky is understanding browser_plainTextLinks.js and merging the tests.

=== Other thoughts ===
Right now, I'm eyeing around September (when I will be back in Toronto for university) for when I can continue work on this patch.

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

The bug assignee is inactive on Bugzilla, so the assignee is being reset.