Closed Bug 1352620 Opened 2 years ago Closed 2 years ago

<datalist> can only suggest the first 20 after upgrading version 52

Categories

(Toolkit :: Form Manager, defect, P1)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: yxu, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(3 files)

Attached image only show the first 20
Use <datalist> as a drop-down suggest list of <input>, it can only suggest the first 20 in list no matter how many data we have when click the <input> box.

E.g
<!DOCTYPE HTML>
<html>
<body>
<label>Choose a number from this list:
<input list="number" name="mynumber"/></label>
<datalist id="number">
<option value="1">
<option value="2">
<option value="3">
<option value="4">
<option value="5">
<option value="6">
<option value="7">
<option value="8">
<option value="9">
<option value="10">
<option value="11">
<option value="12">
<option value="13">
<option value="14">
<option value="15">
<option value="16">
<option value="17">
<option value="18">
<option value="19">
<option value="20">
<option value="21">
<option value="22">
<option value="23">
</datalist>
</body>
</html>

see attachment.

It work fine in 51 version. But in fact the data can be retrieved, for example, input number 2, it will still be prompted 23 in list. So I'm not sure if it is intentional, or is it really a bug.
Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c8216f48c38a8498f251fe044509b930af44de6&tochange=c22f0df11dd8dc7671017fb517f5a9deb3c6fce8
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Layout: Form Controls → Form Manager
Keywords: regression, testcase
Product: Core → Toolkit
Uh...I see what happened, This is due to the repair of bug 1320525.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(In reply to yxu from comment #2)
> Uh...I see what happened, This is due to the repair of bug 1320525.

I don't understand. It does not have a patch.
Flags: needinfo?(yxu)
(In reply to YF (Yang) from comment #3)
> (In reply to yxu from comment #2)
> > Uh...I see what happened, This is due to the repair of bug 1320525.
> 
> I don't understand. It does not have a patch.

I misunderstood the meaning of that bug. The problem still exists.
Status: RESOLVED → REOPENED
Flags: needinfo?(yxu)
Resolution: FIXED → ---
Likely from bug 1296638 but I don't know who has time to fix this…
Blocks: 1296638
Status: REOPENED → NEW
Priority: -- → P1
Mike, can you take a look? Or suggest someone else to work on this?
Attached file Test case
Attachment #8868226 - Attachment mime type: text/plain → text/html
Flags: needinfo?(mconley)
Oh yikes. It looks like the rich autocomplete binding hardcodes a limit of 20 results. :(

http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/toolkit/content/widgets/autocomplete.xml#1198-1206
Assignee: nobody → mconley
Dunno how you feel about the nomaxresults attribute. I could also just extend the binding and override the property. I don't have a preference.
Comment on attachment 8868240 [details]
Bug 1352620 - Don't have a hardcoded limit of 20 items in AutoCompletePopups.

https://reviewboard.mozilla.org/r/139814/#review144508

Thanks. I guess this wfm. I forget if there was a reason we were avoiding extending the binding? It seems like that would be the cleaner solution but I don't know if it's worth the potential wasted time if it doesn't work out. So I only slightly prefer extending at this time (especially given we're both busy).
Attachment #8868240 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #11)
> Comment on attachment 8868240 [details]
> Bug 1352620 - Don't have a hardcoded limit of 20 items in AutoCompletePopups.
> 
> https://reviewboard.mozilla.org/r/139814/#review144508
> 
> Thanks. I guess this wfm. I forget if there was a reason we were avoiding
> extending the binding?

Honestly, just laziness - as you say, we're busy with other stuff.

> So I only slightly prefer extending at this time (especially given we're
> both busy).

I'll file a follow-up bug to extend the binding as a solution instead.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/190c25686f1c
Don't have a hardcoded limit of 20 items in AutoCompletePopups. r=MattN
https://hg.mozilla.org/mozilla-central/rev/190c25686f1c
Status: NEW → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request uplift on this to beta if you feel it's appropriate.
Flags: needinfo?(mconley)
Comment on attachment 8868240 [details]
Bug 1352620 - Don't have a hardcoded limit of 20 items in AutoCompletePopups.

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1296638

[User impact if declined]:

Autocomplete results, or datalists will have a hard-coded cap of 20 entries.

[Is this code covered by automated tests?]:

Yes, autocomplete and datalist has automated tests. Unfortunately, none of them caught this particular bug.

[Has the fix been verified in Nightly?]:

I have manually verified it.

[Needs manual test from QE? If yes, steps to reproduce]: 

No, I don't think so.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're bypassing a hardcoded result limit in the richlist autocomplete popup in a way that is pretty straight-forward.

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8868240 - Flags: approval-mozilla-beta?
Blocks: 1367240
Comment on attachment 8868240 [details]
Bug 1352620 - Don't have a hardcoded limit of 20 items in AutoCompletePopups.

Fix a regression that <datalist> can only suggest the first 20. Beta54+. Should be in 54 beta 11.
Attachment #8868240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Mike Conley (:mconley) from comment #17)
> [Is this code covered by automated tests?]:
> 
> Yes, autocomplete and datalist has automated tests. Unfortunately, none of
> them caught this particular bug.
> 
> [Has the fix been verified in Nightly?]:
> 
> I have manually verified it.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> No, I don't think so.

Setting qe-verify- based on Mike's assessment on manual testing needs.
Flags: qe-verify-
Duplicate of this bug: 1351368
Is this something we should consider backporting to ESR52 or should we let it ride the trains?
Flags: needinfo?(mconley)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is this something we should consider backporting to ESR52 or should we let
> it ride the trains?

I think it's a safe uplift, but I wouldn't call it security critical, which (I imagined) was what we did ESR uplifts for. Does that help clear it up?
Flags: needinfo?(mconley)
Patches for severe regressions can also be considered for ESR52, just not sure whether this meets the bar or not.
Flags: needinfo?(mconley)
I wouldn't call this severe, tbh. It's an annoying bug, but we shipped it in 53, and didn't feel cause to do a dot release for the fix or anything.

So I think we should leave it be.
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.