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

VERIFIED FIXED in Firefox 36

Status

()

Toolkit
Places
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox36 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 5 obsolete attachments)

38 bytes, text/x-review-board-request
mak
: review+
Details | Review
38 bytes, text/x-review-board-request
mak
: review+
Details | Review
38 bytes, text/x-review-board-request
mak
: review+
Details | Review
38 bytes, text/x-review-board-request
mak
: review+
Details | Review
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

Updated

3 years ago
Iteration: --- → 35.2

Updated

3 years ago
Depends on: 1070778
Duplicate of this bug: 1073609

Updated

3 years ago
Blocks: 1071461, 995091

Updated

3 years ago
Iteration: 35.2 → 35.3

Updated

3 years ago
Duplicate of this bug: 1045985
Blocks: 1045985

Updated

3 years ago
Blocks: 1070778
No longer depends on: 1070778

Updated

3 years ago
Iteration: 35.3 → 36.1
Duplicate of this bug: 1084538
Created attachment 8507610 [details] [diff] [review]
WiP Part 1 (v1) - autoselect item
Created attachment 8507613 [details] [diff] [review]
WiP Part 2 (v1) - textValue

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 :(
Created attachment 8507614 [details] [diff] [review]
WiP Part 3 (v1) - test fixes

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)
Blocks: 1085374
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?

Updated

3 years ago
Iteration: 36.1 → 36.2
Created attachment 8516635 [details] [diff] [review]
WiP Part 2 (v2) - textValue

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+
Created attachment 8519624 [details]
MozReview Request: bz://1067903/Unfocused
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
https://reviewboard.mozilla.org/r/349/#review119

Ship It!
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.

Updated

3 years ago
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?

Updated

3 years ago
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*!

Updated

3 years ago
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)
Blocks: 1075450
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/).
\o/


https://hg.mozilla.org/integration/fx-team/rev/a3829376e88f
https://hg.mozilla.org/integration/fx-team/rev/77a8dfd5559e
https://hg.mozilla.org/integration/fx-team/rev/1791d8198cc3
https://hg.mozilla.org/mozilla-central/rev/a3829376e88f
https://hg.mozilla.org/mozilla-central/rev/77a8dfd5559e
https://hg.mozilla.org/mozilla-central/rev/1791d8198cc3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Updated

3 years ago
Depends on: 1104794

Updated

3 years ago
Duplicate of this bug: 1104138

Updated

3 years ago
Depends on: 1105244
Flags: in-testsuite+

Updated

3 years ago
Depends on: 1105361
Depends on: 1105385
Depends on: 1104985

Updated

3 years ago
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
status-firefox36: --- → verified
Comment on attachment 8519624 [details]
MozReview Request: bz://1067903/Unfocused
Attachment #8519624 - Attachment is obsolete: true
Attachment #8618366 - Flags: review+
Attachment #8618367 - Flags: review+
Attachment #8618368 - Flags: review+
Attachment #8618369 - Flags: review+
Created attachment 8618366 [details]
MozReview Request: Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue
Created attachment 8618367 [details]
MozReview Request: Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue
Created attachment 8618368 [details]
MozReview Request: Bug 1067903 - Part 1: Autoselect first autocomplete result.
Created attachment 8618369 [details]
MozReview Request: Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar

Updated

2 years ago
Depends on: 1254503

Updated

2 years ago
No longer depends on: 1254503
You need to log in before you can comment on or make changes to this bug.