Closed Bug 1330731 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- fix-optional
firefox53 --- wontfix
firefox54 --- ?
firefox55 --- fixed

People

(Reporter: rfeeley, Assigned: garvan)

References

Details

Attachments

(6 files, 1 obsolete file)

Attached image 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
wcczzrmf4r6s8bsqa@eonxlwi0s.sc6
mvz2frluigp1imikn@v1i1qtb41.xgs
f11m59eaaqtuf6chk@071wt924v.46n
lkwwxaneo08bo6zf6@7q0tw6bkk.5xj
b1uatoovsynhuhbzb@y9pvbuzf2.jza
hb2y5ir6op99irt1s@wfnk3wc8o.t0s
i0f2spiv9776lz85s@o2mb8q8n7.p3r
stu4ir90wlqv5ckq2@mg3tlbl4n.b67
3rjvqsvyo6m54zrko@kfciv035c.l1l
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?
(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
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?
Flags: needinfo?(MattN+bmo)
Attached image 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?
Duplicate of this bug: 1342401
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.
Duplicate of this bug: 1344399
(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?
Flags: needinfo?(MattN+bmo)
Sure, send a screenshot? Follow form autofill design. They have more states to take into account.
Assignee: nobody → gkeeley
Attached image 325-looks-good.png
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?
Attachment #8856981 - Flags: review?(MattN+bmo)
Attachment #8856981 - Attachment is obsolete: true
Attachment #8856981 - Flags: review?(MattN+bmo)
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.
Attachment #8856981 - Flags: review?(MattN+bmo)
Summary: Set min-width to password dropdown to 250px → insecure password warning is not wide enough to fit text, causes excessive/ugly text wrapping
Attached image 200px-insecure.png (obsolete) —
MattN thanks for review, here is the 200px version.

rfeeley: might be good for you to eyeball this also
(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 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.
Attachment #8856981 - Flags: review?(MattN+bmo) → review+
Thanks for the review Matt, I'll check it against the latest string now, then land.
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
Attachment #8856981 - Flags: review+
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;
}
```
Attachment #8856981 - Flags: review+
Attachment #8860062 - Attachment is obsolete: true
Latest patch, correctly on m-c TIP this time
Huh, I removed the r+ (i thought) and it appeared again, 
reading RB docs..
Attachment #8856981 - Attachment is obsolete: true
Attachment #8856981 - Flags: review+
Got it. Marked obsolete previously r+'d patch, new patch up for review.
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.
Attachment #8856981 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
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
https://hg.mozilla.org/mozilla-central/rev/e3e173383b3d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Want to request uplift to beta to bring this fix to 54?
Flags: needinfo?(gkeeley)
(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
Flags: needinfo?(gkeeley) → needinfo?(rfeeley)
I don't think it warrants an uplift.
Flags: needinfo?(rfeeley)
You need to log in before you can comment on or make changes to this bug.