1069903 - Search term highlighting fails for trimmed URLs

Status: RESOLVED WONTFIX

(Firefox :: Address Bar, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 1047469 implemented search term highlighting but that unfortunately works for Google only. For every other search engine using HTTP we trim off the protocol and parseSubmissionURL() thus fails.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #22)
> this._value can be something like
> 'moz-action:switchtab,{"url":"https://bugzilla.mozilla.org/"}', which I
> don't think you want here. I think you can call nsIURIFixup::createFixupURI
> to get the proper URI out of this.value.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #23)
> _getSelectedValueForClipboard already has a comment highlighting its
> assumption that trimValue only trims at the start or the end. I was going to
> suggest you should include a similar comment here, or even just move the
> comment to trimURL to ensure we never break that invariant, but thinking
> about it further I think this patch doesn't properly handle trimValue
> stripping trailing slashes? I guess it's unlikely that such URLs would end
> up being parsed as search URLs, but I don't know that we want to rely on
> that.
> 
> Seems like we're also missing test coverage in general here?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1047469-Follow-up-to-support-search-keyword-high.patch, v2

Added a simple check to ensure we don't try calling .formatValue() after the window is gone. Saw that popping up when running tests lately.

Instead of this._value this patch now uses .createFixupURI(). It also adds a few tests for search term highlighting including url trimming.

The issue that Gavin mentioned wrt to trimming trailing slashes works when I test it manually but I just can't get the url bar to trim it in the test. Both the schema and the trailing slash will always occur before search parameters so it should be safe to just subtract the difference between the original and the trimmed URL from .termsOffset.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8492170 [details] [diff] [review]
0001-Bug-1047469-Follow-up-to-support-search-keyword-high.patch, v2

>+              // Scheme and/or a trailing slash might be missing from |value|
>+              // if it's trimmed. Need to subtract that from the offset.
>+              let strippedPrefixLen = uri.spec.length - value.length;

This looks like it won't work correctly when 'value' contains decoded special characters (via losslessDecodeURI) that would be encoded in uri.spec...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Adding a test for that is certainly a good idea. We'll find out then.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1047469-Follow-up-to-support-search-keyword-high.patch, v3

So I think we just have to decode the URI again before passing it to parseSubmissionURL(). Added two tests and they work fine.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8494542 [details] [diff] [review]
0001-Bug-1047469-Follow-up-to-support-search-keyword-high.patch, v3

The amount of work we're doing here (creating an nsIURI with nsIURIFixup, decoding it, letting the search service parse it) for _all_ URLs, not only those with a search term, seems somewhat excessive at this point. Can you verify that the overhead is insignificant or find a different way to implement this stuff? Otherwise I don't think I'm comfortable with this, since frankly the gain for the user seems minuscule.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

I honestly don't have a better idea right now. It seems like the work the patch is doing is necessary to get it right... But I also agree that the overhead here *might* be non-negligible.

Comment 9

[Marco Bonardo [:mak]]

search terms hilight has been backed out cause it's a failing approach (see bug 1075069)

New hilight is bug 1048223.