Implement search terms hilighting

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: mak, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

Trunk
Firefox 35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox35 verified)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

5 years ago
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).
Reporter

Updated

5 years ago
Flags: firefox-backlog+
Reporter

Comment 1

5 years ago
5 points, cause will need a test.
Points: --- → 5
Flags: in-testsuite?
Reporter

Updated

5 years ago
Depends on: 1047472
Reporter

Comment 3

5 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Reporter

Updated

5 years ago
Depends on: 959563
Reporter

Comment 4

5 years ago
I made bug 959563 an investigation bug since it has some discussion ongoing already. Cleaned up a little bit the dependency tree.
QA Whiteboard: [qa+]
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 34.2
QA Contact: alexandra.lucinet
Based on bug 1047472 this seems to work well.
Attachment #8469233 - Flags: review?(dao)
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 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.
Attachment #8469233 - Flags: review?(dao) → review+
QA Contact: alexandra.lucinet → andrei.vaida
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
Iteration: 34.3 → 35.1
Turns out that using the search service here forces its sync initialization. We should use search.init() to not interfere with startup.
Attachment #8469233 - Attachment is obsolete: true
Attachment #8483549 - Flags: review?(dao)
Attachment #8483549 - Flags: review?(dao) → review+
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
Whiteboard: [fixed-in-fx-team]
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.
Attachment #8484446 - Flags: review?(dao)
Implemented what we talked about at the airport.
Attachment #8483549 - Attachment is obsolete: true
Attachment #8484446 - Attachment is obsolete: true
Attachment #8484446 - Flags: review?(dao)
Attachment #8485641 - Flags: review?(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.
Attachment #8485641 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/bd17f5c378a4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
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.
Flags: needinfo?(ttaubert)
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
Flags: needinfo?(ttaubert)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8490695 - Flags: review?(dao) → review-
_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?
Blocks: 1069903
Will fix the remaining issues in bug 1069903. The backlog doesn't handle reopening issues too well.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8490695 - Attachment is obsolete: true
No longer blocks: 1069903
Depends on: 1069903
Verified fixed based on Comment 24. Test results available in Comment 18. Follow-up: Bug 1069903.
Status: RESOLVED → VERIFIED
Reporter

Updated

5 years ago
Blocks: 1071461
Depends on: 1073175
wontfixing in favor of bug 1048223...
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite?
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.