1047469 - Implement search terms hilighting

Status: RESOLVED WONTFIX

(Firefox :: Address Bar, defect)

Description

[Marco Bonardo [:mak]]

this is the implementation bug for bug 1034382.

We should expand formatValue to also hilight search terms when an url is recognized as a search.

We have a new API to parse urls into search terms implemented in bug 1040721 we can use here. We should expand it though to provide us the ranges directly. Then this should become quite trivial.

Note: the UX mock-up is here: https://bug1029848.bugzilla.mozilla.org/attachment.cgi?id=8450207 (see bug 1029848 for further insight).

Comment 1

[Marco Bonardo [:mak]]

5 points, cause will need a test.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Dupe of bug 959563? See also bug 959569.

Comment 3

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> Dupe of bug 959563? See also bug 959569.

ugh, why do we have 2 breakdowns about the same thing?!

I think I'm going to dupe everything here cause at least this breakdown is basically done.

Comment 4

[Marco Bonardo [:mak]]

I made bug 959563 an investigation bug since it has some discussion ongoing already. Cleaned up a little bit the dependency tree.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1047469-Implement-search-terms-hilighting.patch

Based on bug 1047472 this seems to work well.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot of search term highlighting

Here is what the patch looks like.

For multiple search terms, should we also add multiple selections? So that the "+" that divides the search terms would not be highlighted?

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8469233 [details] [diff] [review]
0001-Bug-1047469-Implement-search-terms-hilighting.patch

>+          let parsed = Services.search.parseSubmissionURL(value);

Since this method doesn't primarily deal with search URLs, please rename "parsed" to something less ambiguous.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1047469-Implement-search-terms-hilighting-r-dao.patch, v2

Turns out that using the search service here forces its sync initialization. We should use search.init() to not interfere with startup.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/bf39866cc3a5

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Was so excited to finally land this that I forgot to push to try and fix a test :/

https://hg.mozilla.org/integration/fx-team/rev/3da93d34897d

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1047469-Follow-up-test-fixes.patch

The plan is to fold this patch into the first one but I attached it separately for easier review. Had to rewrite the test to handle async formatting due to waiting for Services.search.init(). Included two small other tiny fixes as well.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=4f2bf44a6c86

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1047469-Implement-search-terms-hilighting-r-dao.patch

Implemented what we talked about at the airport.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=dc9933974d24

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8485641 [details] [diff] [review]
0001-Bug-1047469-Implement-search-terms-hilighting-r-dao.patch

>+          function addSelectionRange(start, end) {
>+            let range = document.createRange();
>+            range.setStart(textNode, start);
>+            range.setEnd(textNode, end);
>+            selection.addRange(range);
>+          }

This could be declared later, after the early returns.

>+          if (!this._searchServiceInitialized) {
>+            // Initialize the search service asynchronously
>+            // if that hasn't happened yet.
>+            Services.search.init(() => {
>+              this._searchServiceInitialized = true;
>+              this.formatValue();
>+            });
>+          } else {

I think I'd prefer doing this as an early return rather redundantly doing part of the work and bailing out somewhere in the middle.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/bd17f5c378a4

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bd17f5c378a4

Comment 18

[Andrei Vaida [:avaida]]

Testing performed on Nightly 35.0a1 (2014-09-16), using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.7.5.

Potential issues:
1. Strings are not highlighted for all the search engines available.
 * Bing, Amazon, eBay and Wikipedia are only highlighting the domains in the URL bar
 * screenshot: http://i.imgur.com/Ai4QAcZ.png 

Tim, what's your take on this?

Acceptance criteria for this patch:
- Search strings containing numbers, letters and special characters are highlighted.
- Long search strings containing various characters are highlighted.
- Search strings are highlighted for all the available search engines.
- Search strings are highlighted in case of private browsing and/or e10s window.
- Search strings are highlighted in case of small display resolution and/or browser window.
- Search strings are highlighted in case of keywords containing 1 or multiple spaces.
- Search strings are still highlighted in case of switch-to-tab.
- Search strings are still highlighted in case of bookmarked searches.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Weird. I only tested it for Google, unfortunately not for Bing. parseSubmissionURL() fails in NetUtil.newURI() and yields NS_ERROR_MALFORMED_URI. The URL looks quite ok to me though:

http://www.bing.com/search?q=foo+bar&pc=MOZI&form=MOZSBR

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Wow, so we're cutting off the "http" and "www.bing.com/..." isn't a valid URI of course. This works for Google because they use HTTPS.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 22

[Dão Gottwald [:dao]]

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

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 23

[:Gavin Sharp [email: [e-mail]]]

_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 24

[Tim Taubert [:ttaubert] (inactive)]

Will fix the remaining issues in bug 1069903. The backlog doesn't handle reopening issues too well.

Comment 25

[Andrei Vaida [:avaida]]

Verified fixed based on Comment 24. Test results available in Comment 18. Follow-up: Bug 1069903.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

This was backed out by bug 1075069.

Comment 27

[Dão Gottwald [:dao]]

wontfixing in favor of bug 1048223...