1330731 - insecure password warning is not wide enough to fit text, causes excessive/ugly text wrapping

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: 250px-dropdown.png

If it's true [1] that the 95% of email addresses are 31 characters long or less, we should probably try to acommodate them with a minimum-width dropdown to fit 31 characters. Based on my calculations [2] that's comfortably 250px.

BONUS: Looks like a good width for insecure password warning in English.

[1] http://www.freshaddress.com/fresh-perspectives-blog/long-email-addresses/

[2] Randomly generated 31 character emails:
a0gnbajbl7na1tpc5@hy66munv6.3y1
[e-mail]6
[e-mail]
f11m59eaaqtuf6chk@071wt924v.46n
lkwwxaneo08bo6zf6@7q0tw6bkk.5xj
[e-mail]
hb2y5ir6op99irt1s@wfnk3wc8o.t0s
i0f2spiv9776lz85s@o2mb8q8n7.p3r
stu4ir90wlqv5ckq2@mg3tlbl4n.b67
3rjvqsvyo6m54zrko@kfciv035c.l1l

Comment 1

[Ryan Feeley [:rfeeley]]

Matt brought to my attention that setting a min-width means that we risk going off screen to the right. Any comments or ideas? Could change min-width in these cases?

Comment 2

[Matthew N. [:MattN]]

(In reply to Ryan Feeley [:rfeeley] from comment #0)
> If it's true [1] that the 95% of email addresses are 31 characters long or
> less, we should probably try to acommodate them with a minimum-width
> dropdown to fit 31 characters. Based on my calculations [2] that's
> comfortably 250px.

It's usually not necessary to see all characters to distinguish an email though. e.g. the TLD the is often not necessary as users don't normally have two email addresses differing only by TLD (though I can imagine some cases).

I just learned that we actually have a minimum of 100px already for all autocomplete dropdowns already:
* https://dxr.mozilla.org/mozilla-central/rev/e677ba018b22558fef1d07b74d416fd3a28a5dc3/toolkit/components/satchel/AutoCompletePopup.jsm#160
* https://dxr.mozilla.org/mozilla-central/search?q=%22width+%3E+100+%3F+width+%3A+100%22&redirect=false

Comment 3

[Ryan Feeley [:rfeeley]]

It looks like an unstyled text input without any styles is 133px wide. Does that sound right?

You are right that emails are often distinguisable with less. Am just trying to find a balance between truncation and a too-wide dropdown. Is there a more scientific way to make this call?

Comment 4

[Ryan Feeley [:rfeeley]]

Attachment: out-of-view.png

It looks like the dropdown responds well to narrow browser windows and even being pushed off screen. Any other reasons why min-width might be a bad idea?

Comment 6

[Johann Hofmann [:johannh]]

Bug 1342401 shows that there are issues with the existing minimum width for locales with long words (and I guess we want to keep break-work behavior), so it might be good to make it larger.

Comment 8

[Matthew N. [:MattN]]

(In reply to Ryan Feeley [:rfeeley] from comment #4)
> Created attachment 8838159 [details]
> out-of-view.png
> 
> It looks like the dropdown responds well to narrow browser windows and even
> being pushed off screen. Any other reasons why min-width might be a bad idea?

My only concern is that 250px is too large and different from the Form Autofill UX spec which I believe called for 150px (the current is 100px). Can we try 150px for now?

Comment 9

[Ryan Feeley [:rfeeley]]

Sure, send a screenshot? Follow form autofill design. They have more states to take into account.

Comment 10

[:garvan]

Attachment: 150-looks-like-haiku.png

Comment 11

[:garvan]

Attachment: 325-looks-good.png

Comment 12

[:garvan]

Here it is at 150px: attachment 8855865 [details]
Looks pretty bad still.

Here it is at 325px: attachment 8855866 [details] 
Which looks good.

Ignore the yellow color BTW.

However, have I borked something else in the UI by CSS style of all XUL panels of class 'autocomplete-richlistbox'?
Is there some way to maximize the specificity of the CSS selector to only affect the insecure popup?

Comment 13

[:garvan]

Attachment: Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

to 150pt. r?MattN

Review commit: https://reviewboard.mozilla.org/r/128894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128894/

Comment 14

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/1-2/

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

https://reviewboard.mozilla.org/r/128894/#review134198

::: toolkit/content/autocomplete.css:42
(Diff revision 2)
> +#PopupAutoComplete[firstresultstyle="insecureWarning"] {
> +  min-width: 150px;
> +}

This file is for more generic autocomplete styling, please move this to above https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/browser/base/content/browser.css#525

::: toolkit/content/autocomplete.css:43
(Diff revision 2)
>  .ac-separator[type=keyword] {
>    display: none;
>  }
> +
> +#PopupAutoComplete[firstresultstyle="insecureWarning"] {
> +  min-width: 150px;

In the meantime we have reverted to the longer text (bug 1355534) so I think this should be (at least) 200px.

Comment 16

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/2-3/

Comment 17

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/3-4/

Comment 18

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/4-5/

Comment 19

[:garvan]

Attachment: 200px-insecure.png

MattN thanks for review, here is the 200px version.

rfeeley: might be good for you to eyeball this also

Comment 20

[Matthew N. [:MattN]]

(In reply to :garvan from comment #19)
> Created attachment 8860062 [details]
> 200px-insecure.png
> 
> MattN thanks for review, here is the 200px version.
> 
> rfeeley: might be good for you to eyeball this also

I think you didn't rebase to pick up the newer string from bug 1355534

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

https://reviewboard.mozilla.org/r/128894/#review134920

::: commit-message-a6c77:2
(Diff revision 5)
> +Bug 1330731 - Insecure form warning popup min-width increased
> + to 200px, old width has 4 lines of text wrap (ugly), this has 2 (less ugly). r?MattN

This actually has 3 with the new string which you will see if you rebase.

Comment 22

[:garvan]

Thanks for the review Matt, I'll check it against the latest string now, then land.

Comment 23

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Clearing review flag. Code has no effect after it was moved from
toolkit/content/autocomplete.css to
browser/base/content/browser.css

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

https://reviewboard.mozilla.org/r/128894/#review135024

::: browser/base/content/browser.css:528
(Diff revision 5)
>  }
>  
>  #PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureWarning"] {
>    -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem-insecure-field");
>    height: auto;
> +  min-width: 200px;

Oops, I didn't look closely. I meant to put the whoel rule above that one:
```css
#PopupAutoComplete[firstresultstyle="insecureWarning"] {
  min-width: 200px;
}
```

Comment 25

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/5-6/

Comment 26

[:garvan]

Attachment: 200px-insecure-3-lines.png

Latest patch, correctly on m-c TIP this time

Comment 27

[:garvan]

Huh, I removed the r+ (i thought) and it appeared again, 
reading RB docs..

Comment 28

[:garvan]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128894/diff/6-7/

Comment 29

[:garvan]

Got it. Marked obsolete previously r+'d patch, new patch up for review.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8856981 [details]
Bug 1330731 - Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly).

https://reviewboard.mozilla.org/r/128894/#review135078

Marking r+ again just in case you didn't see my IRC messages.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3e173383b3d
Insecure form warning popup min-width increased to 200px, old width has 4 lines of text wrap (ugly), this has 3 (less ugly). r=MattN

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3e173383b3d

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift to beta to bring this fix to 54?

Comment 34

[:garvan]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> Want to request uplift to beta to bring this fix to 54?

I have no opinion on that, but I am ni rfeeley as he might

Comment 35

[Ryan Feeley [:rfeeley]]

I don't think it warrants an uplift.