Closed Bug 1180827 Opened 4 years ago Closed 4 years ago

Datalist doesn't autocomplete anymore for longer words

Categories

(Core :: General, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 + fixed
firefox42 + fixed

People

(Reporter: Harald, Assigned: mrbkap)

References

Details

Attachments

(2 files, 1 obsolete file)

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
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?
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.
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
[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 <mrbkap@gmail.com>
date:        Thu May 28 09:55:46 2015 -0700
summary:     Bug 1024437 - Make <datalist> work in e10s. r=MattN
Blocks: 1024437
Flags: needinfo?(mrbkap)
Tracking on Firefox 42. Is 41 currently affected? If so, please set the 'affected' flag.
Flags: needinfo?(bzbarsky)
> Is 41 currently affected?

Yes, it is.
Flags: needinfo?(bzbarsky)
I see this too. I have a patch, but I'm working on writing a test for this before I attach it.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8631920 - Flags: review?(MattN+bmo)
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?
Attachment #8631920 - Flags: review?(MattN+bmo) → feedback+
Attached patch Patch v1.1Splinter Review
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.
Attachment #8631920 - Attachment is obsolete: true
Attachment #8633827 - Flags: review?(MattN+bmo)
Attachment #8633827 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8633828 [details] [diff] [review]
Remove other setTimeouts

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

Thanks!
Attachment #8633828 - Flags: review?(MattN+bmo) → review+
Thanks for the quick reviews!
https://hg.mozilla.org/mozilla-central/rev/95ac3fd4ccc5
https://hg.mozilla.org/mozilla-central/rev/eda42fc9d928
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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
Flags: needinfo?(mrbkap)
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
Flags: needinfo?(mrbkap)
Attachment #8633827 - Flags: approval-mozilla-aurora?
Comment on attachment 8633828 [details] [diff] [review]
Remove other setTimeouts

This isn't required, but it makes a test theoretically less flaky.
Attachment #8633828 - Flags: approval-mozilla-aurora?
Comment on attachment 8633827 [details] [diff] [review]
Patch v1.1

Approved. Safe as it's been in m-c for 2 weeks now.
Attachment #8633827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8633828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.