Datalist doesn't autocomplete anymore for longer words

RESOLVED FIXED in Firefox 41

Status

()

Core
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Harald, Assigned: mrbkap)

Tracking

Trunk
mozilla42
All
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41+ fixed, firefox42+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 2

2 years ago
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
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
Flags: needinfo?(mrbkap)

Comment 5

2 years ago
Tracking on Firefox 42. Is 41 currently affected? If so, please set the 'affected' flag.
tracking-firefox42: ? → +
Flags: needinfo?(bzbarsky)
> Is 41 currently affected?

Yes, it is.
status-firefox41: --- → affected
Flags: needinfo?(bzbarsky)

Updated

2 years ago
tracking-firefox41: ? → +
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
Created attachment 8631920 [details] [diff] [review]
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.
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+
(Assignee)

Comment 10

2 years ago
Created attachment 8633827 [details] [diff] [review]
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.
Attachment #8631920 - Attachment is obsolete: true
Attachment #8633827 - Flags: review?(MattN+bmo)
(Assignee)

Comment 11

2 years ago
Created attachment 8633828 [details] [diff] [review]
Remove other setTimeouts
Attachment #8633828 - Flags: review?(MattN+bmo)
(Assignee)

Comment 12

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7874df919e1b
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+
(Assignee)

Comment 14

2 years ago
Thanks for the quick reviews!

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/95ac3fd4ccc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda42fc9d928
https://hg.mozilla.org/mozilla-central/rev/95ac3fd4ccc5
https://hg.mozilla.org/mozilla-central/rev/eda42fc9d928
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
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)
(Assignee)

Comment 18

2 years ago
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?
(Assignee)

Comment 19

2 years ago
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+

Updated

2 years ago
Attachment #8633828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e936bc5444ad
https://hg.mozilla.org/releases/mozilla-aurora/rev/16c7bedeca10
status-firefox41: affected → fixed
You need to log in before you can comment on or make changes to this bug.