Closed Bug 1069903 Opened 6 years ago Closed 6 years ago

Search term highlighting fails for trimmed URLs


(Firefox :: Address Bar, defect)

Not set





(Reporter: ttaubert, Assigned: ttaubert)




(1 file, 1 obsolete file)

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.
(In reply to Dão Gottwald [:dao] from comment #22)
> this._value can be something like
> 'moz-action:switchtab,{"url":""}', which I
> don't think you want here. I think you can call nsIURIFixup::createFixupURI
> to get the proper URI out of this.value.
(In reply to :Gavin Sharp [email:] 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?
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Blocks: 1047469
No longer depends on: 1047469
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.
Attachment #8492170 - Flags: review?(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...
Attachment #8492170 - Flags: review?(dao) → review-
Adding a test for that is certainly a good idea. We'll find out then.
So I think we just have to decode the URI again before passing it to parseSubmissionURL(). Added two tests and they work fine.
Attachment #8492170 - Attachment is obsolete: true
Attachment #8494542 - Flags: review?(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.
Attachment #8494542 - Flags: review?(dao)
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.
Iteration: 35.2 → 35.3
search terms hilight has been backed out cause it's a failing approach (see bug 1075069)

New hilight is bug 1048223.
Closed: 6 years ago
Resolution: --- → WONTFIX
Flags: qe-verify+ → qe-verify-
Iteration: 35.3 → ---
You need to log in before you can comment on or make changes to this bug.