Closed
Bug 1258964
Opened 8 years ago
Closed 8 years ago
Emphasize algorithm misses overlapping occurrences in the navigation bar search results
Categories
(Toolkit :: Autocomplete, defect)
Toolkit
Autocomplete
Tracking
()
VERIFIED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | verified |
People
(Reporter: layus, Assigned: layus)
References
Details
Attachments
(4 files, 3 obsolete files)
18.88 KB,
image/png
|
Details | |
15.43 KB,
image/png
|
Details | |
7.17 KB,
patch
|
Details | Diff | Splinter Review | |
2.55 KB,
patch
|
Details | Diff | 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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
This bug is related to bug 407946.
Assignee | ||
Updated•8 years ago
|
Mentor: edilee
Flags: needinfo?(edilee)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → layus.on
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=410e4e2ef49f
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8da3fb057432
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8733758 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8735426 -
Flags: review?(mak77)
Assignee | ||
Updated•8 years ago
|
Attachment #8735427 -
Flags: review?(mak77)
Assignee | ||
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8735427 -
Flags: review?(mak77) → review+
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8735426 -
Attachment is obsolete: true
Attachment #8736675 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8736675 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8735427 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
(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.
Assignee | ||
Comment 23•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(MattN+bmo)
Comment 24•8 years ago
|
||
Oh ok, I thought the test was testing the new behaviour.
Flags: needinfo?(MattN+bmo)
Comment 25•8 years ago
|
||
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.
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7de386dfe2bb https://hg.mozilla.org/integration/fx-team/rev/5c2450c268e4
Keywords: checkin-needed
Assignee | ||
Comment 27•8 years ago
|
||
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 :-).
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7de386dfe2bb https://hg.mozilla.org/mozilla-central/rev/5c2450c268e4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Flags: qe-verify+
Comment 29•8 years ago
|
||
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.
Description
•