Closed
Bug 1383707
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Autofill Options and Autofill phishing are not visible (w/o scrolling) in autofill dropdown when 6+ profiles
Categories
(Toolkit :: Form Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | disabled |
firefox55 | --- | disabled |
firefox56 | --- | fixed |
People
(Reporter: aflorinescu, Assigned: ralin)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [form autofill:MVP])
Attachments
(1 file)
[Version:] 56.0a1 20170723030206 [Description:] a. as described in the spec.: https://mozilla.invisionapp.com/share/AP8TFZ22G#/screens/188574112 , the Autofill MVP should not have a maxumin height for the DD suggestion (it should show all profiles at once w/o scrolling). b. even with scroll enabled as the current implementation, the phishing message and the Form Autofill options are not visible by default. [Steps:] 1. Create/Have 6+ profiles. 2. Open https://luke-chang.github.io/autofill-demo/basic.html (or any form autofill). 3. Trigger autofill for any of the input fields. [Actual Result:] The phishing warning message and the Form Autofill options are at the bottom of the dropdown: reaching/seeing them them require scrolling. [Expected Result:] Both the phishing message and the form autofill options should be visible by default unrelated to the dd implementation (scroll enabled/dissabled for the dd)
Assignee | ||
Comment 1•7 years ago
|
||
confirm this, I can spot the issue with 6 profiles in one-row layout dd. I thought in another bug we've removed the limit of result number, but it seems that the unlimited "maxResults" doesn't mean unlimited "maxRows" in display. I just got an one-liner patch to this issue in local, and will upload it soon. Thanks.
Assignee | ||
Comment 2•7 years ago
|
||
Hey Matt, I realized that "maxRows" could not be overwritten directly, so we would need something like: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/content/widgets/autocomplete.xml#421-428 to temporarily set maxRows to infinite before calling adjustHeight(). Do you think it's fine to do that in http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/components/satchel/AutoCompletePopup.jsm#139? Thanks.
Assignee | ||
Comment 3•7 years ago
|
||
ah, I made a test trying to modify .maxRows in AutoCompletePopup#139 but it threw `TypeError: setting getter-only property "maxRows"`, maybe we should look for another place... I'll try to find out where can we change "maxRows" in autocomplete.xml tomorrow.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I was wrong in comment 3, the maxRows of popup is not writable as it's just a getter. In the patch, the idea is still stick to comment 1, which temporarily increase maxRows in AutoCompletePopup.jsm#139, and we can even reuse the existing mechanism that restore maxRows from _normalMaxRows property every time the popup closed[0]. Still the code isn't so clean because we have to check the "originaltype" to determine whether to increase the value or not, however, that's finest approach I can think of right now. Could you help me to review it? Thank you. [0] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/content/widgets/autocomplete.xml#1050-1052
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Reporter | ||
Updated•7 years ago
|
Blocks: 1329903
status-firefox-esr52:
--- → unaffected
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8889961 [details] Bug 1383707 - Temporarily increase the maxRows for form autofill results in order not to show scrollbar in the popup. https://reviewboard.mozilla.org/r/161018/#review168588 ::: toolkit/components/satchel/AutoCompletePopup.jsm:155 (Diff revision 2) > // We were sent a message from a window or tab that went into the > // background, so we'll ignore it for now. > return; > } > > + let firtResultStyle = results[0].style; typo: firstResultStyle ::: toolkit/components/satchel/AutoCompletePopup.jsm:176 (Diff revision 2) > this.openedPopup.mInput = AutoCompleteResultView; > + // Temporarily increase the maxRows as we don't want to show > + // the scrollbar in form autofill popup. > + if (firtResultStyle == "autofill-profile") { > + this.openedPopup._normalMaxRows = this.openedPopup.maxRows; > + this.openedPopup.mInput.maxRows = 100; So we don't need to reset this after?
Attachment #8889961 -
Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8889961 [details] Bug 1383707 - Temporarily increase the maxRows for form autofill results in order not to show scrollbar in the popup. https://reviewboard.mozilla.org/r/161018/#review168588 Thanks :D > So we don't need to reset this after? No, http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/content/widgets/autocomplete.xml#1050-1052 reset maxRows from _normalMaxRows while popup hiding.
Comment 12•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/814bf2e58ade Temporarily increase the maxRows for form autofill results in order not to show scrollbar in the popup. r=MattN
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill] → [form autofill:MVP]
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/814bf2e58ade
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•