Closed Bug 1067903 Opened 10 years ago Closed 9 years ago

Autoselect first autocomplete result when it's guaranteed to be a special result

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox36 --- verified

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

Attachments

(4 files, 5 obsolete files)

38 bytes, text/x-review-board-request
mak
: review+
Details
38 bytes, text/x-review-board-request
mak
: review+
Details
38 bytes, text/x-review-board-request
mak
: review+
Details
38 bytes, text/x-review-board-request
mak
: review+
Details
The various parts of bug 951624 aim to make the first autocomplete result always a special result that corresponds to what happens when you type anything and hit enter without explicitly selecting any autocomplete result. this makes it obvious what happens when you press enter.

However, as-is is also breaks the use-case of typing something, hitting down to select the first *real* result, and using that. To accommodate that, we want to auto-select the first special result. This will mean that when you type anything, the action that will be taken when pressing enter will implicitly be the one that's already selected.
Flags: qe-verify+
Flags: firefox-backlog+
QA Contact: andrei.vaida
Iteration: --- → 35.2
Depends on: 1070778
Iteration: 35.2 → 35.3
Blocks: 1070778
No longer depends on: 1070778
Iteration: 35.3 → 36.1
Attached patch WiP Part 2 (v1) - textValue (obsolete) — Splinter Review
Because it's now autoselecting an item, I've needed to fixup how we handle the real value of the URLbar vs the display value. Unfortunately, this uncovering how fragile the urlbar and autocomplete code is :(
Attached patch WiP Part 3 (v1) - test fixes (obsolete) — Splinter Review
And the test fixes.
I have two test failures left:

browser/base/content/test/general/browser_bug556061.js
- Basically, the urlbar isn't catching all changes to the edit box (the display) and updating the real value accordingly
browser/base/content/test/general/browser_tabopen_reflows.js
- focusAndSelectUrlBar is causing a reflow. I don't know why this isn't happening on m-c today; it's already expected in the new-window reflow test.
Jared: Do you know why focusAndSelectUrlBar isn't already causing a reflow? (see above for some more context)
Flags: needinfo?(jaws)
As a reminder, browser_tabopen_reflows.js only reports uninterruptible reflows so it's possible your changes made an interruptible reflow become uninterruptible.
I can't reproduce the test failure in browser_tabopen_reflow, with the patches from this bug and bug 1070778. I'm guessing that it is coming from the following though,
> +            if (this._matchCount > 0 && this.selectedIndex == -1)
> +              this.selectedIndex = 0;
Flags: needinfo?(jaws)
Update here:
I put this on hold for a bit while I worked on a grand plan for autocomplete (bug 1071344). But I figured out my last major problem (turns out it's not getting notified of all changes to the input element). AFAICT it won't require any significant changes to urlbar/autocomplete to fix, which was what I was worried about.
great, does that mean we can expect reviewable patches this week?
Iteration: 36.1 → 36.2
Attached patch WiP Part 2 (v2) - textValue (obsolete) — Splinter Review
Well, my ideas on how to fix that issue I mentioned in comment 11 didn't exactly end up as I expected. Turned out to be an issue in the URLbar's custom copy/cut controller.

Not quite done yet - some tests are having issues with e10s. But it seems like a reasonable point to get a feedback pass on this part.
Attachment #8507613 - Attachment is obsolete: true
Attachment #8516635 - Flags: feedback?(mak77)
Comment on attachment 8516635 [details] [diff] [review]
WiP Part 2 (v2) - textValue

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

it looks good afaict.

::: browser/base/content/urlbarBindings.xml
@@ +128,5 @@
>        <!--
>          onBeforeValueSet is called by the base-binding's .value setter.
>          It should return the value that the setter should use.
>        -->
> +      <!-- TODO: refactor this! -->

not very useful without a bug number, unless you planned to do this here...

@@ +163,5 @@
>        </method>
>  
> +      <property name="value">
> +        <getter><![CDATA[
> +          return this._value;

the original getter uses onBeforeValueGet, and it's the only thing using it... so likely it should be removed if we do this.

though at this point, I wonder why copying the whole value getter to override it, rather than just changing onBeforeValueGet... this is copying a lot of code and increasing the risk of making toolkit and browser out of sync. I'd avoid that for now, unless we have compelling reasons to do that.
I think for this milestone we should try to change as few code as possible.
Attachment #8516635 - Flags: feedback?(mak77) → feedback+
Attachment #8519624 - Flags: review?(mak77)
/r/349 - Bug 1070778 - Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item. r=mak
/r/351 - Bug 1067903 - Part 1: Autoselect first autocomplete result.
/r/353 - Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar
/r/355 - Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue

Pull down these commits:

hg pull review -r 205a9f0ab25eacafa20aef377a47bb28190a989e
Attachment #8516635 - Attachment is obsolete: true
Attachment #8507614 - Attachment is obsolete: true
Attachment #8507610 - Attachment is obsolete: true
Note: https://reviewboard.mozilla.org/r/349/#review119 (comment 17) is there because that patch is depended on by the patches for this bug, and I can't find a way to not have that descendant changeset show up here.
Iteration: 36.2 → 36.3
https://reviewboard.mozilla.org/r/347/#review237

::: browser/base/content/urlbarBindings.xml
(Diff revision 1)
> -          var action = this._parseActionUrl(url);
> +          var action = this._parseActionUrl(this._value);

s/var/let

::: browser/base/content/urlbarBindings.xml
(Diff revision 1)
> +            if (index > 0)
> +              newIndex = 0;
> +            else
> +              newIndex = maxRow;

this would be better served by a

newIndex = index > 0 ? 0 : maxRow;

::: browser/base/content/urlbarBindings.xml
(Diff revision 1)
> +            if (index < maxRow)
> +              newIndex = maxRow;
> +            else
> +              newIndex = 0;

ditto

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 1)
> +  for (let i = 0; i < limit; i++)
> +    func(i);

please always brace loops

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 1)
> +  yield new Promise(resolve => addVisits(visits, resolve));

promiseAddVisits (as I said in the other bug, you have to copy it to the local head or to a testing module)

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 1)
> +  is(results.length, 11, "Should get 11 results");

It might be worth to add a second task that will match more than maxRichResults and still ensure we loop right

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 1)
> +  yield promiseClearHistory();

you can do this in registerCleanupFunction(function* () { yield...

::: browser/base/content/test/general/browser_bug1024133-switchtab-override-keynav.js
(Diff revision 1)
> -  EventUtils.synthesizeKey("VK_DOWN" , {});
> +  info(`\n\n\t### gURLBar.value = ${gURLBar.value}\n\n`);

debug leftover?

::: browser/base/content/test/general/browser_bug1024133-switchtab-override-keynav.js
(Diff revision 1)
> +  debugger;

leftover?

::: browser/base/content/test/general/browser_bug1070778.js
(Diff revision 1)
> +    itemIds.forEach(PlacesUtils.bookmarks.removeItem);

or you could just PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledbookmarksFolderId)

::: browser/base/content/test/general/browser_locationBarCommand.js
(Diff revision 1)
>          is(gURLBar.value, "", "Urlbar reverted to original value");       

while here, could you please remove these trailing spaces?

::: browser/components/sessionstore/test/browser_522545.js
(Diff revision 1)
> +      /*gURLBar.inputField.value = "example.org";
>        let event = document.createEvent("Events");
>        event.initEvent("input", true, false);
> -      gURLBar.dispatchEvent(event);
> +      gURLBar.inputField.dispatchEvent(event);*/

leftover?
Attachment #8519624 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/347/#review299

> It might be worth to add a second task that will match more than maxRichResults and still ensure we loop right

I'd totally do that if I could get a reliable test for it. Alas, I beleive bug 1060750 is getting in the way. So I'd like to just stick with the test coverage we have, get this landed, and move to fixing the underlying issues via the controller rewrite.
Attachment #8519624 - Flags: review?(mak77)
/r/349 - Bug 1067903 - Part 1: Autoselect first autocomplete result.
/r/351 - Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar
/r/353 - Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue

Pull down these commits:

hg pull review -r 2c4b9ba02760ca4c8ff25c4de15d6e9886cb7093
https://reviewboard.mozilla.org/r/347/#review303

::: toolkit/components/places/tests/PlacesTestUtils.jsm
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Filed bug 1100082 to convert other tests to use this. Because there's a *lot*!
Attachment #8519624 - Flags: review?(mak77) → review+
https://reviewboard.mozilla.org/r/347/#review305

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 2)
> +let {PlacesTestUtils} = Cu.import("resource://testing-common/PlacesTestUtils.jsm", {});

defineLazyModuleGetter in head.js please

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
(Diff revision 2)
> +function times(limit, func) {

"repeat" would probably be clearer than "times"

::: browser/base/content/test/general/browser_bug1070778.js
(Diff revision 2)
> -    itemIds.forEach(PlacesUtils.bookmarks.removeItem);
> +    PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledbookmarksFolderId);

oops, should be .unfiledBookmarksFolderId (notice the uppercase B)

::: browser/base/content/test/general/browser_bug1070778.js
(Diff revision 2)
> -  // Select keyword item
> +  // First item should alreasdy be selected

typo: alreasdy

::: toolkit/components/places/tests/PlacesTestUtils.jsm
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please use the PD header in tests code

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

::: toolkit/components/places/tests/PlacesTestUtils.jsm
(Diff revision 2)
> +this.PlacesTestUtils = {

please Object.freeze this

::: toolkit/components/places/tests/PlacesTestUtils.jsm
(Diff revision 2)
> +      for (let i = 0, len = places.length; i < len; ++i) {

for (let place of places)
https://reviewboard.mozilla.org/r/347/#review505

> Please use the PD header in tests code
> 
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/ */

Oh, indeed. But if it's PD, we no longer need the PD license header at all, since it's covered by the licence policy (http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/).
Depends on: 1104794
Depends on: 1105244
Flags: in-testsuite+
Depends on: 1105361
Depends on: 1104985
Depends on: 1105911
This is now verified fixed on Aurora 36.0a2 (2014-12-01), with found issues already on file. Platforms covered: Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 LTS 32-bit.
Status: RESOLVED → VERIFIED
Attachment #8519624 - Attachment is obsolete: true
Attachment #8618366 - Flags: review+
Attachment #8618367 - Flags: review+
Attachment #8618368 - Flags: review+
Attachment #8618369 - Flags: review+
Depends on: 1254503
No longer depends on: 1254503
You need to log in before you can comment on or make changes to this bug.