Ryan, can you specify padding amount and locations?

Specified here a while back: https://bugzilla.mozilla.org/show_bug.cgi?id=1319919#c0

Binding is defined here: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/content/widgets/autocomplete.xml#1466 The asesombar binding it extends is: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/content/widgets/autocomplete.xml#1540 _handleOverflow is the code that may need to be adjusted if CSS alone isn't enough: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/content/widgets/autocomplete.xml#2283-2284 I would suggest trying CSS-only changes to different ancestors of the richlistitems to hopefully avoid touching that calculation code.

I initially tried to solve this using flexbox until I found that the _handleOverflow code actually respects an attribute called overflowpadding, and using that seems to solve the issues (unless I'm missing something). I'll add some screenshots. Let me know what you think.

Quick comparison

This would probably also fix bug 1333756.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. Looking good except the right padding. I still see text touching the edge at certain sizes (at least in latest Nightly). Hard for me to validate in screenshot because it's only a problem when the text goes right to the edge.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. Ryan, you should be able to download the build from try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ed9aa912c2b26be65e5d8e31c9b94e7201dc04&selectedJob=74394336 Next time I'll make sure a try build is pushed before requesting ui-review, thanks for clarifying that :)

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. https://reviewboard.mozilla.org/r/109512/#review111058 The ::: browser/base/content/browser.xul:149 (Diff revision 1) > > <panel type="autocomplete-richlistbox" > id="PopupAutoComplete" > noautofocus="true" > hidden="true" > + overflowpadding="8" This is going to affect form history too ::: browser/themes/shared/autocomplete.inc.css:31 (Diff revision 1) > padding: 0; > } > > > +:root { > + --login-autocomplete-padding: 8px; The padding now looks too large everywhere. I'm on my phone doing a quick review but I recall Ryan saying to divide his numbers by 2 since they were at 2dppx. I really just care about the right padding for 52 and I believe your attempt to use a single number is oversimplifying as there are other borders, padding and margin in play.

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #10) > Comment on attachment 8833265 [details] > Bug 1330840 - Add Padding to in-content Insecure Password Warning. > > https://reviewboard.mozilla.org/r/109512/#review111058 > > The > > ::: browser/base/content/browser.xul:149 > (Diff revision 1) > > > > <panel type="autocomplete-richlistbox" > > id="PopupAutoComplete" > > noautofocus="true" > > hidden="true" > > + overflowpadding="8" > > This is going to affect form history too Ok, seeing that form history already has a left padding of 4px and you'd like the password autocomplete to also be 4px I think this should actually be fine. I'll just change that number to 4 then. > > ::: browser/themes/shared/autocomplete.inc.css:31 > (Diff revision 1) > > padding: 0; > > } > > > > > > +:root { > > + --login-autocomplete-padding: 8px; > > The padding now looks too large everywhere. I'm on my phone doing a quick > review but I recall Ryan saying to divide his numbers by 2 since they were > at 2dppx. I really just care about the right padding for 52 and I believe > your attempt to use a single number is oversimplifying as there are other > borders, padding and margin in play. Alright, setting this single variable to 4px will achieve what you want while making sure the padding is consistent. As far as I can tell, the same padding also applies to the autocomplete. Ryan specified in bug 1319919 comment 0: > matches left margin of usernames below So it's our goal to keep these paddings tied. I really don't want to bikeshed over variable vs. no-variable, but we would have the same number around six different times otherwise.

Turns out you're right, assuming we leave this at 4px the padding is already defined by the non-specific selectors for title and site-icon, we don't need any new CSS variable. I attached a patch with only the 4px overflowpadding applied. Sorry for trying to over-complicate this, as you mentioned it seemed more complex initially.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. https://reviewboard.mozilla.org/r/109512/#review111504 Awesome! This looks good. Nice find on the attribute. Please request uplift as I think it's pretty straightforward. You may want to double check the interaction on beta since the Learn More text landed there but hopefully the padding goes after it.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/2edf2a4c017a Add Padding to in-content Insecure Password Warning. r=MattN

Just checked, the padding seems to apply to the learn more and it even breaks as a whole. Very nice. Requesting uplift.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. Approval Request Comment [Feature/Bug causing the regression]: Improvement to bug 1217162 [User impact if declined]: No right padding on the in-content insecure password warning, which looks ugly. [Is this code covered by automated tests?]: No, it's just a small style change [Has the fix been verified in Nightly?]: Not yet. [Needs manual test from QE? If yes, steps to reproduce]: Not sure, but I don't think so. You could check this by opening e.g. http-login.badssl.com and dragging the window so that the input box becomes small enough for the warning to wrap lines. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Just adding an attribute that adds a few pixels of padding to autocomplete. [String changes made/needed]: None

Hi Rares, could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!

-tested on 54.0a1 / 20170208110248. I can confirm the fix and that I see no apparent issue with the Contextual warning layout. Although I also agree that the Insecure contextual message looks a bit better with padding,I would note though that in some cases (dependent on the size of the user/pass control) the old wrapping of the Contextual text looked a bit more 'solid'. Not sure though if this is something that we can do anything about.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. Fix an UI issue of in-content insecure password warning and was verified. Aurora53+.

Comment on attachment 8833265 [details] Bug 1330840 - Add Padding to in-content Insecure Password Warning. style fix for insecure password warning, beta52+

Verified fixed FX 52b6, 53.0a2 (2017-02-16) Win 10.