Closed Bug 1258964 Opened 4 years ago Closed 4 years ago

Emphasize algorithm misses overlapping occurrences in the navigation bar search results

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox48 --- verified

People

(Reporter: layus, Assigned: layus)

References

Details

Attachments

(4 files, 3 obsolete files)

Attached patch highlightAllResults.patch (obsolete) — Splinter Review
The current algorithm emphasizes multiple occurrences of the same word if they do not overlap, and overlapping occurrences of different words.

For example (the <> indicate the emphasized parts).
the search terms [cheval valise] match "Le <chevalise>" and "Le <cheval>ier"
but the search term [tat] matches only "a<tat>a<tat>a" and not "a<tatatat>a" as could be expected.

The attached patch addresses this issue and slightly optimizes the code by lowercasing only the substring on which we actually search.
(Before, the lowercasing was done before extracting the substring.)

The two attached screen captures show how the patch improves the UI.
Attached image highlight_old.png
Attached image highlight_new.png
This bug is related to bug 407946.
Mentor: edilee
See Also: → 407946
Mentor: edilee
Flags: needinfo?(edilee)
I have no idea of who is responsible/concerned by this change.
Tagging :nbp, as I know him, and :Mardak as he worked on bug 407946.
Flags: needinfo?(nicolas.b.pierron)
Thanks Guillaume :)
I will forward the question to the Firefox Front-end team.

Matt, Jared, do you have a better idea who might be able to review these changes?
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jaws)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8733758 [details] [diff] [review]
highlightAllResults.patch

Thanks for the patch. At a glance it looks good. Marco (:mak) will be a good person to review this.

We may need to update a test or add a new test to validate this change as well as protect this change from regressing. You can look at the tests at /toolkit/content/tests/chrome/test_autocomplete* to get an idea of how this already being tested.
Flags: needinfo?(jaws)
Flags: needinfo?(edilee)
Flags: needinfo?(MattN+bmo)
Attachment #8733758 - Flags: review?(mak77)
Assignee: nobody → layus.on
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I can already say that `./mach test /toolkit/content/tests/chrome/test_autocomplete*` passes all the tests.

It would we good to have anti-regression tests indeed, but I do not feel too confident about writing a test suite in javascript, not to mention the xml/xul/javascript used here :). Will see what I can do.
Comment on attachment 8733758 [details] [diff] [review]
highlightAllResults.patch

Review of attachment 8733758 [details] [diff] [review]:
-----------------------------------------------------------------

yes the patch looks ok. It could also be done with a RegExp, but from my empirical measurements it would be a tiny bit slower.

Now, looks like we don't have any test for emphasis, and it would be really nice to have one.
But it's not trivial for a newcomer and since we already didn't have one, and it's not a critical functionality (AC will work even with a broken emphasis) I'd also accept a follow-up bug filed to add such a test to 
/toolkit/content/tests/chrome/ before this lands.
The test should be feasible copying from the existing test_autocomplete_NNN.xul tests, and checking the ._title property, timilarly to what http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_action_keyword.js is doing.
Attachment #8733758 - Flags: review?(mak77) → review+
once the test follow-up bug is filed, please post an updated version of the patch with the author and commit message, as explained in https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Then you need a Try push, if you have access to the try server just post the treeherder link here, otherwise needinfo me or jared and we can push for you.

Once Try is "green" enough (no related failures), set the checkin-needed keyword to have it pushed to the tree.
Ok, thanks Marco for these instructions.

About the Try push, I think I still have access, but do you want a full Try build, or just some subset of the tests ?
Flags: needinfo?(mak77)
all mochitests on win/lin/mac should be enough.
Flags: needinfo?(mak77)
Attached patch emphasis_test.patch (obsolete) — Splinter Review
Attached patch emphasis_fix.patch (obsolete) — Splinter Review
Attachment #8733758 - Attachment is obsolete: true
Attachment #8735426 - Flags: review?(mak77)
Attachment #8735427 - Flags: review?(mak77)
Ok, the tests passed on try (see comment 13).

I have added test cases for the old behavior (emphasis_test.patch), and updated my previous patches with the modified behavior in the test cases (emphasis_fix.patch).

Waiting for reviex+, then checkin :-).
Flags: needinfo?(mak77)
Comment on attachment 8735426 [details] [diff] [review]
emphasis_test.patch

Review of attachment 8735426 [details] [diff] [review]:
-----------------------------------------------------------------

what can I say, it looks very nice, thank you for making a test case.

::: toolkit/content/tests/chrome/chrome.ini
@@ +55,5 @@
>  [test_autocomplete3.xul]
>  [test_autocomplete4.xul]
>  [test_autocomplete5.xul]
>  [test_autocomplete_change_after_focus.html]
> +[test_autocomplete_emphasis.xul]

please keep the ini files alphabetically ordered (this should go after _delayOnPaste)

::: toolkit/content/tests/chrome/test_autocomplete_emphasis.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
> +
> +<window title="Autocomplete Widget Test 5"

please change to "Autocomplete emphasis test"

@@ +139,5 @@
> +  let result = autocomplete.popup.richlistbox.firstChild;
> +
> +  for (let attribute of [result._title, result._url]) {
> +    is(attribute.childNodes.length, currentTest.emphasis.length, "The element should have the expected number of children.");
> +    for( let i=0; i<currentTest.emphasis.length; i++) {

nit: add whitespaces around = and <

@@ +142,5 @@
> +    is(attribute.childNodes.length, currentTest.emphasis.length, "The element should have the expected number of children.");
> +    for( let i=0; i<currentTest.emphasis.length; i++) {
> +      let node = attribute.childNodes[i];
> +      // Emphasized parts strictly alternate.
> +      if( (i % 2 == 0) == currentTest.emphasizeFirst ) {

fix whitespaces as if (condition) {
Attachment #8735426 - Flags: review?(mak77) → review+
Attachment #8735427 - Flags: review?(mak77) → review+
please merge the 2 patches and add r=mak to the commit message, to simplify work of our tree sheriffs. when ready please set the checkin-needed keyword for having someone push the patch to the tree.
Flags: needinfo?(mak77)
Attachment #8735426 - Attachment is obsolete: true
Attachment #8736675 - Flags: review+
Attachment #8736675 - Flags: review+
Attachment #8735427 - Attachment is obsolete: true
Ok, fixed the small issues.

I kept the two patch separated as they clearly show what worked before, and what the fix changes to the old behavior. I made it clear in which order these should be merged by prefixing them with their part number.
Keywords: checkin-needed
(In reply to Guillaume Maudoux [:layus] from comment #21)
> I kept the two patch separated as they clearly show what worked before, and
> what the fix changes to the old behavior. I made it clear in which order
> these should be merged by prefixing them with their part number.

If you want to keep them separate then the order should be switched as we try to have tests passing at the commit level to help with bisection. With your ordering the test would fail if I update my repo to part 1.
Comment on attachment 8736677 [details] [diff] [review]
part2_emphasis_fix.patch

Review of attachment 8736677 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/tests/chrome/test_autocomplete_emphasis.xul
@@ +99,5 @@
>        emphasizeFirst: true
>      },
>      { search: "tat",
>        result: "tatatat",
> +      emphasis: ["tatatat"],

This line is the only tested behavior affected by the fix.
As it is included in part2, part1 compiles by itself and passes the tests. For now you have to trust me on this, but I can make another try push if you want.
The tests pass at commit level, and in this order the commits are a self-description of the introduced change.

Part1: Introduce tests for the old behavior,
Part2: Introduce the actual fix and alter the test suite to reflect them.
Flags: needinfo?(MattN+bmo)
Oh ok, I thought the test was testing the new behaviour.
Flags: needinfo?(MattN+bmo)
I don't think it's a critical requirement to have that logical separation, but you're clearly free to do what you like with your patches. no worries.
It is not really important, but I spent some time to ensure that the tests passed before and after the fix, so I just wanted it to be checked in. Maybe a bit stupid, but at least this makes a nice history, with the second commit explicitly showing the behavior change :

>      { search: "tat",
>        result: "tatatat",
>-       emphasis: ["tat", "a", "tat"],
>+       emphasis: ["tatatat"],
>        emphasizeFirst: true
>      },

This has a nice feeling of well-done things :-).
https://hg.mozilla.org/mozilla-central/rev/7de386dfe2bb
https://hg.mozilla.org/mozilla-central/rev/5c2450c268e4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reproduced on Nightly 2016-03-23 Win 7.
Verified fixed FX 48b1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.