Closed Bug 1298204 Opened 8 years ago Closed 8 years ago

Auto-complete on form fields does show some white lines or do not show up sometimes

Categories

(Toolkit :: Form Manager, defect, P1)

Product:
Toolkit
Component:
Form Manager
Version:
51 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- verified

People

(Reporter: clement.lefevre, Assigned: mconley)

References

Details

Attachments

(3 files, 1 obsolete file)

Auto-complete list bug
12.41 KB, image/png
Details
Bug 1298204 - Make sure the form autocomplete popup tree gets updated when invalidated.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Test case
130 bytes, text/html
Details
Attached image Auto-complete list bug —
Sometimes on websites search field (in a form), it's supposed to offer completion previous inputs when you type something in. This regression result in sometimes having no auto-complete list appearing, and sometimes having some white lines. These lines are still containing datas so if you go down on them with arrow keys and press enter, you still get their value. See the joined screenshot for that. Bug reproduced on both OS X and Linux.
Assignee: nobody → mconley
Keywords: nightly-community, regression
Whiteboard: [mozfr-community]
Likely from bug 1294502.
Blocks: 1294502
Status: NEW → ASSIGNED
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox-esr45: --- → unaffected
Priority: -- → P1
Thanks Clément. I'm having difficulty reproducing this. Can you give me some steps to reproduce? It's okay if it only reproduces sometimes, but I need some tips on how to hit this (it'd be best if the steps assumed a fresh profile).
Flags: needinfo?(clement.lefevre)
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #2) > Thanks Clément. > > I'm having difficulty reproducing this. Can you give me some steps to > reproduce? It's okay if it only reproduces sometimes, but I need some tips > on how to hit this (it'd be best if the steps assumed a fresh profile). I've no real STR excepted go to a website with input forms with no JS inside it to interfere with the behavior and having previously enter several values, then trying to autocomplete them either for the beginning part or as a substring (in the middle of the string). In my case I could see it for example on leboncoin.fr or hackernews search field. And the bug usually happens when there are several matches and I'm not sure about this part, but it could happens more often on middle of the string matches instead of beginning. I don't know if those informations can help though, but I hope so…
Flags: needinfo?(clement.lefevre)
Keywords: nightly-community, regression
Whiteboard: [mozfr-community]
Comment on attachment 8786863 [details] Bug 1298204 - Make sure the form autocomplete popup tree gets updated when invalidated. https://reviewboard.mozilla.org/r/75740/#review73638 I attempted to add a test for this in toolkit/components/satchel/test_autocomplete.html, but the framework in there is pretty tricky, and I just couldn't make it happen. :(
Attachment #8787243 - Attachment mime type: text/plain → text/html
Attachment #8787243 - Attachment is obsolete: true
Attached file Test case —
Attachment #8787244 - Attachment mime type: text/plain → text/html
STR: 1) Visit the test case page 2) In the input, type "beatles" and press enter 3) Now type in "breaking bad" and press enter 3) Now type in "bruce lee" and press enter 4) Now, in the empty field, type in "b" and wait for the autocomplete popup to appear. It should show "beatles, breaking bad, bruce lee". 5) Now type in "ea", so the text should be "bea". There should just be "beatles" in the autocomplete popup now. 6) Now type in "l" so the text should be "beal". There shouldn't be any autocomplete popup now. 7) Hit backspace once so that the text input contains "bea", and "beatles" shows up again in the autocomplete popup. 8) Hit backspace twice so that the text input contains "b" and wait for the autocomplete popup to update. Expected results: The autocomplete popup should contain "beatles, breaking bad, bruce lee" Actual results: The autocomplete popup contains "beatles" followed by an empty white space.
Comment on attachment 8786863 [details] Bug 1298204 - Make sure the form autocomplete popup tree gets updated when invalidated. https://reviewboard.mozilla.org/r/75740/#review75280 ::: toolkit/components/satchel/AutoCompletePopup.jsm:163 (Diff revision 1) > } else { > AutoCompleteTreeView.setResults(results); > + // We need to re-set the view in order for the > + // tree to know the view has changed. > + this.openedPopup.view = AutoCompleteTreeView; > this.openedPopup.invalidate(); Sorry for the delay but I honestly don't really follow what's going on. Could you explain more about the wrong state before the fix and how the fix helps? i.e. what is .view equal to here without the fix?
Flags: needinfo?(mconley)
(In reply to Matthew N. [:MattN] from comment #10) > Comment on attachment 8786863 [details] > Bug 1298204 - Make sure the form autocomplete popup tree gets updated when > invalidated. > > https://reviewboard.mozilla.org/r/75740/#review75280 > > ::: toolkit/components/satchel/AutoCompletePopup.jsm:163 > (Diff revision 1) > > } else { > > AutoCompleteTreeView.setResults(results); > > + // We need to re-set the view in order for the > > + // tree to know the view has changed. > > + this.openedPopup.view = AutoCompleteTreeView; > > this.openedPopup.invalidate(); > > Sorry for the delay but I honestly don't really follow what's going on. > Could you explain more about the wrong state before the fix and how the fix > helps? Sure thing. XUL trees need to be told when the number of rows they have are changed. See http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/layout/xul/tree/nsITreeBoxObject.idl#165 Another way of updating the nsITreeBoxObject is by just setting the view again - see: http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/layout/xul/tree/nsTreeBodyFrame.cpp#479 Specifically, see: http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/layout/xul/tree/nsTreeBodyFrame.cpp#527 The rowCountChanged stuff is usually for when the tree is being updated, but not invalidated (like what we're doing when we get a fresh batch of results from AutoCompletePopup in browser-content.js. The old implementation in AutoCompleteE10S.jsm used to do this setting of the view here: https://hg.mozilla.org/mozilla-central/rev/f2ea401ab10c#l3.186, inside _showPopup. So we were setting the view every time the nsITreeView was invalidated, which is correct. In my change from bug 1294502, I only set the view when the popup was first opened: https://hg.mozilla.org/mozilla-central/rev/f2ea401ab10c#l3.218 So the patch in this bug causes us to go back to the old behaviour, which was to set the view not just at opening time, but also when the nsITreeView is invalidated, which causes the tree to update. > i.e. what is .view equal to here without the fix? Actually the same old view, but setting the view is how you can signal to the nsITreeObject that the view has updated. Don't ask me why, this stuff is old and cranky. Does that clear it up?
Flags: needinfo?(mconley) → needinfo?(MattN+bmo)
Gentle review ping.
(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #11) > (In reply to Matthew N. [:MattN] from comment #10) > So the patch in this bug causes us to go back to the old behaviour, which > was to set the view not just at opening time, but also when the nsITreeView > is invalidated, which causes the tree to update. > > > i.e. what is .view equal to here without the fix? > > Actually the same old view, but setting the view is how you can signal to > the nsITreeObject that the view has updated. Don't ask me why, this stuff is > old and cranky. So you're saying that calling .invalidate isn't enough to invalidate some data but resetting the view property is? I guess you're saying that rowCountChanged is the usual way to invalidate this kind of state but since we're not doing this, we can reset the view which invalidates the proper data. I guess this is fine even though it seems like ideally we would call `rowCountChanged` but that requires a lot more of our own state tracking IIUC.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8786863 [details] Bug 1298204 - Make sure the form autocomplete popup tree gets updated when invalidated. https://reviewboard.mozilla.org/r/75740/#review76802 Sorry about the delay.
Attachment #8786863 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/73153f2fee67 Make sure the form autocomplete popup tree gets updated when invalidated. r=MattN
(In reply to Matthew N. [:MattN] from comment #13) > (In reply to Mike Conley (:mconley) - (Digging through needinfos and > reviews) from comment #11) > > (In reply to Matthew N. [:MattN] from comment #10) > > So the patch in this bug causes us to go back to the old behaviour, which > > was to set the view not just at opening time, but also when the nsITreeView > > is invalidated, which causes the tree to update. > > > > > i.e. what is .view equal to here without the fix? > > > > Actually the same old view, but setting the view is how you can signal to > > the nsITreeObject that the view has updated. Don't ask me why, this stuff is > > old and cranky. > > So you're saying that calling .invalidate isn't enough to invalidate some > data but resetting the view property is? I guess you're saying that > rowCountChanged is the usual way to invalidate this kind of state but since > we're not doing this, we can reset the view which invalidates the proper > data. > > I guess this is fine even though it seems like ideally we would call > `rowCountChanged` but that requires a lot more of our own state tracking > IIUC. Correct! Thanks for the autoland!
https://hg.mozilla.org/mozilla-central/rev/73153f2fee67
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: qe-verify+
I reproduced this issue using Fx 51.0a1, build ID: 20160825030226, on Ubuntu 14.04 LTS, using steps from comment 9. I can confirm this issue is fixed, I verified using Fx 51.0b10, build ID: 20161222080852, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11. Cheers!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: