1180827 - Datalist doesn't autocomplete anymore for longer words

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:Harald Kirschner :digitarald]

Tested on 42.0a1 (2015-07-06), OS X 10.10.3.

Steps:
1. Open http://codepen.io/matt-west/full/jKnzG
2. Type "Java"

Expected behavior:
- Show Java and JavaScript as options

Actual result:
- No dropdown

Comment 1

[Boris Zbarsky [:bzbarsky]]

Works fine for me on that page in a 2015-07-06 nightly on Mac (also 10.10.3).

Harald, do you see the problem in safe mode?

Comment 2

[bomfog]

Windows 2015-07-06 nightly works with a virgin profile.

Turn off e10s and let the browser restart, and get at least a version of the reported bug:

Type "j", get dropdown with choices, "a" ditto, "v" dropdown disappears, backspace to delete the "v" dropdown reappears.

Skip the "j", type "ava", no problem, dropdown appears.

This is in the "programming language" textbox only, not the "HTML element" textbox.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I do see the problem comment 2 describes in non-e10s mode (including in a non-e10s window, opened via the File menu, of a recent nightly with a clean profile).

Looks like the behavior changed in this commit range:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f986e55c4e0b&tochange=45a4d6336c73

Comment 4

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:  Functionality regression in non-e10s, which is what we're shipping.

The first bad revision is:
changeset:   246066:7837f943758c
user:        Blake Kaplan <[e-mail]>
date:        Thu May 28 09:55:46 2015 -0700
summary:     Bug 1024437 - Make <datalist> work in e10s. r=MattN

Comment 5

[Kate Glazko]

Tracking on Firefox 42. Is 41 currently affected? If so, please set the 'affected' flag.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Is 41 currently affected?

Yes, it is.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I see this too. I have a patch, but I'm working on writing a test for this before I attach it.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I didn't realize there wasn't a test for this. There were three bugs to fix:
  * Iterating from the back to the front made no sense and would cause the results to reverse themselves for every new character when reusing previous results.
  * The comparison was supposed to be case insensitive (lowercased on both sides).
  * The comparison is against the *labels* and not the values.

This patch fixes all of these issues and adds a test for them.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8631920 [details] [diff] [review]
Patch v1

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

The fix is fine but I have some notes about the test.

::: toolkit/components/satchel/test/test_form_autocomplete_with_list.html
@@ +208,5 @@
> +        doKey("down");
> +        break;
> +
> +    case 10:
> +       sendString("PAS");

indentation in case 10 and 11 is wrong (3 space)

@@ +213,5 @@
> +       waitForMenuChange(2);
> +       break;
> +
> +    case 11:
> +      synthesizeKey("S", {});

* can you add a comment in the new cases to describe what's being tested (like case 9 has).

@@ +216,5 @@
> +    case 11:
> +      synthesizeKey("S", {});
> +      synthesizeKey("1", {});
> +      setTimeout(function() {
> +        doKey("down");

Why are you using setTimeout instead of splitting the case into two?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.1

I had copied that pattern from elsewhere in the test. I've fixed the new setTimeout and have a patch to fix the other two.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Remove other setTimeouts

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7874df919e1b

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8633828 [details] [diff] [review]
Remove other setTimeouts

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

Thanks!

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Thanks for the quick reviews!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ac3fd4ccc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda42fc9d928

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95ac3fd4ccc5
https://hg.mozilla.org/mozilla-central/rev/eda42fc9d928

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Blake, should we uplift this to Aurora so as to fix it in FF41 as well? I was able to repro it on 41.0a2 buildID: 20150727004009

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8633827 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: bug 1024437
[User impact if declined]: Non-e10s users will have trouble using datalist entries longer than 2 characters.
[Describe test coverage new/current, TreeHerder]: Tests written for this case in this bug and have been running on TreeHearder.
[Risks and why]: Low risk, targeted fix to make an algorithm perform as originally intended.
[String/UUID change made/needed]: n/a

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8633828 [details] [diff] [review]
Remove other setTimeouts

This isn't required, but it makes a test theoretically less flaky.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8633827 [details] [diff] [review]
Patch v1.1

Approved. Safe as it's been in m-c for 2 weeks now.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e936bc5444ad
https://hg.mozilla.org/releases/mozilla-aurora/rev/16c7bedeca10