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)

56 Branch
defect
Not set
normal

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)
See Also: → 1383744
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.
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.
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.
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
s/stick to comment 1/stick to comment 2/
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 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.
Thanks
Keywords: checkin-needed
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
Whiteboard: [form autofill] → [form autofill:MVP]
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.