97811 - use bullets instead of asterisks to block out password characters

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[J Luh]

When accepting passwords, other Mac applications echo bullets instead of what
you've typed. Mozilla uses asterisks (*). Is it possible to use bullets instead?

Comment 1

[Ashley Bischoff (blog at handcoding.com)]

No dupes found. Confirming.

-> All/All?

Reporter: Please include your BuildID in your bug reports. In future, I would
recommend that you use the Bugzilla Helper for bug reports:
http://www.mozilla.org/quality/help/bugzilla-helper.html

(among other things, it automagically includes the BuildID)

Comment 2

[Matthew T (active 1999-2002)]

Thankyou, james! This is a bug I've been meaning to file for, er, a year or so, 
but never got around to it. :-)

--> XP Toolkit/Widgets, -->trivial because it's a bug not an enhancement, CC 
Timeless because he mentioned this bug earlier, and CC brade coz she probably 
knows roughly where the offending code is.

Comment 3

[John Morrison]

The stuff inside a textbox (or html <input>) is controlled by editor.

Comment 4

[kinmoz]

--> brade (my mac buddy)

Comment 5

[Kathleen :Brade]

strategy:  nsILookAndFeel

Comment 6

[Kathleen :Brade]

this fix will be in nsTextEditRules.cpp (around line 1340)

Comment 7

[Kathleen :Brade]

I could fix this with an #ifdef in the right place but to use nsILookAndFeel
will require some changes to that API to handle the "char" type.  This will have
to wait.  :-(

Comment 8

[Kathleen :Brade]

I'm going to attach a patch which will only work on Mac builds (#ifdef
solution).  I know this might not be really desirable but the end for mozilla
1.0 is near.

I spoke with ftang and he preferred the unicode char here instead of 25CF.  It
seems a bit small to me but it isn't a '*' either.

I tested this by going to various websites and typing in their password fields
(including some non-latin1 charsets).

I could do a preference-based solution but I worry about a slight performance
hit if we go that route.

Comment 9

[Kathleen :Brade]

Attachment: #ifdef to use bullet unicode character on Macintosh builds

ftang please review
sfraser please super-review

Comment 10

[Ashley Bischoff (blog at handcoding.com)]

brade: I can't speak for everyone, but what about just using the bullets for all
platforms? I'm using win32 here, and I would actually welcome the updated look :).

Comment 11

[Kathleen :Brade]

-->push off for a while; attached patch isn't great because the bullet is small
(and 25CF isn't always available)

Comment 12

[scottputterman]

marking nsbeta1- for Mach V per ADT.

Comment 13

[Kathleen :Brade]

sadly, I don't have a solution for internationalization where the bullet
character is not in the font/character set.  The only i18n-friendly situation is
the patch above (which is a very tiny bullet and not really acceptable in my
opinion).
-->Future

Comment 14

[Jonathan Brecher]

I'm confused. This is a resource-level change, suggesting that the relevent 
edittext fields use the kControlEditTextPasswordProc rather than the usual 
kControlEditTextProc tag.  Much better to let the system handle everything it's 
designed to handle.  Or are there edittext fields that sometimes hold passwords 
and sometimes not?

Comment 15

[Simon Fraser [no longer active]]

Jonathan: we don't use native text widgets in Mozilla, so it's more complex than
a resource change.

Comment 16

[Jonathan Brecher]

Well, fooey.  OK, maybe someone at Apple would be nice enough to tell you what 
logic they use to select their bullet-ish character?

Comment 17

[Greg K.]

Although U+25CF is a black circle, U+2022 is actually called bullet. Is that a
better-looking character?

Any way to set up a fallback mechanism, to use asterisks if the bullet character
isn't available?

Comment 18

[Kathleen :Brade]

U+2022 is too small (imo and other's opinion too); it's about the size of a
period '.'

A fallback mechanism (use U+25CF if it's available) sounds great to me but I
don't have the time to investigate how that would work.  Can someone take the
current patch and tweak it to do the fallback?  (use U+25CF if charset is
certain things or ?)

Comment 19

[Kathleen :Brade]

removing nomination for nsbeta1

Comment 20

[Alexander Nguyen]

Windows XP uses bullets for password characters.

Comment 21

[Kathleen :Brade]

*** Bug 213730 has been marked as a duplicate of this bug. ***

Comment 22

[Chris Lawson (gone)]

I can take this but I need some guidance on the fallback logic. I'm afraid my linguistic skills lie entirely within the Roman character set, and as such I have no experience (or practical knowledge) elsewhere.

cl

Comment 23

[Chris Lawson (gone)]

*** Bug 168627 has been marked as a duplicate of this bug. ***

Comment 24

[Chris Lawson (gone)]

Removing dependency as bug 168627 is now a dupe of this.

cl

Comment 25

[timeless]

smontagu, neil, ere: thoughts?

Comment 26

[Kathleen :Brade]

Attachment: update previous patch

Comment 27

[Kathleen :Brade]

Comment on attachment 213371 [details] [diff] [review]
update previous patch

better patch coming tomorrow

Comment 28

[[e-mail]]

&#25CF; seems to work fine in Windows 95, and NT3.51 has it too :-)

Comment 29

[Kathleen :Brade]

I tested my trunk build (which finally finished building) on 10.4 Mac with these pages:
  http://my.yahoo.co.jp/  (EUC-JP)
  http://bugzilla.mozilla.org/  (Latin1)
  https://www.google.com/accounts/Login?continue=http://www.google.co.jp/&hl=ja
and some local pages that use various charsets.

A Windows XP trunk build was tested on similar pages as was a Linux build.

Comment 30

[Kathleen :Brade]

Attachment: tested patch

Comment 31

[Kathleen :Brade]

Note: the latest patch has #if defined check for Mac because 25CF was HUGE.  2022 looks fine on Mac (and similar to Safari).  25CF looks fine on Windows/Linux; 2022 was too small.  

Comment 32

[Kathleen :Brade]

suggested sites from timeless:
  http://www.orangesavingsbank.com/
  http://comcast.com/
  https://secure.ingdirect.com/myaccount/InitialINGDirect.html?command=displayLogin&device=web&locale=en_US&userType=Client

Comment 33

[Boris Zbarsky [:bzbarsky]]

So I'm not sure I buy the ifdef thing.  That's a property of the font being used, not of the OS.  So all using the ifdef does is introduce complexity without really solving the problem....

Comment 34

[Simon Fraser [no longer active]]

Comment on attachment 213498 [details] [diff] [review]
tested patch

>+#if defined(XP_UNIX) && defined(XP_MACOSX)
>+  PRUnichar pwchar = PRUnichar(0x2022);
>+#else
>+  PRUnichar pwchar = PRUnichar(0x25CF);
>+#endif

XP_UNIX is defined for XP_MACOSX, so I think the test can be
#if defined(XP_MACOSX).

Comment 35

[[e-mail]]

Comment on attachment 213498 [details] [diff] [review]
tested patch

>+#if defined(XP_UNIX) && defined(XP_MACOSX)
>+  PRUnichar pwchar = PRUnichar(0x2022);
>+#else
>+  PRUnichar pwchar = PRUnichar(0x25CF);
>+#endif
IMHO this should be const or #define or something.

>   aOutString->Truncate();
>   for (i=0; i<length; i++)
>   {
>-    aOutString->Append(PRUnichar('*'));
>+    aOutString->Append(pwchar);
>   }
Possibly better to switch these to iterators e.g.
nsWritingIterator<PRUnichar> iter, enditer;
aOutString->BeginWriting(iter);
aOutString->EndWriting(enditer);
while (iter != enditer)
{
#ifdef XP_MACOSX
  *iter++ = 0x2022;
#else
  *iter++ = 0x25CF;
#endif
}

Comment 36

[Kathleen :Brade]

Attachment: simplified patch

bz (comment 33) -- I don't know anything about the font / character selection code in mozilla.  Stuart suggested that this ancient patch might work.  In my testing it seems to.  The need for the ifdef is that 25CF is laughably huge in the Macintosh fonts I've tested.  It looks like Safari uses 2022.  On Windows and Linux 25CF looks best and 2022 looks too small.

smfr (comment 34) -- thanks; fixed.

neil (comment 35) -- I removed the local variable and put the #ifdef in the loop.  An iterator could be considered as part of a separate bug; I'd like to keep this patch very small.

Comment 37

[Simon Fraser [no longer active]]

Comment on attachment 213587 [details] [diff] [review]
simplified patch

r=me but I suggest you test some simple & traditional Chinese, and korean pages too.

Comment 38

[Kathleen :Brade]

also tested these sites on Mac:
  http://www.csc.edu.cn/gb/
  http://kr.yahoo.com/

I do see a difference between DeerPark and Safari for kr.yahoo.com.  Safari's password characters seem close together while DeerPark's have gaps between them (see attachment).

Comment 39

[Kathleen :Brade]

Attachment: kr.yahoo.com.png: safari on left; deerpark on right

Comment 40

[Boris Zbarsky [:bzbarsky]]

> The need for the ifdef is that 25CF is laughably huge in the Macintosh fonts
> I've tested.  It looks like Safari uses 2022.  On Windows and Linux 25CF looks
> best and 2022 looks too small.

My point is that this depends on the fonts installed, not on the operating system.  For example, on my Linux system (FC4) here, in the adobe-times font and in whatever XFT thinks "Serif" is, 0x25CF is huge and 0x2022 is too small.  So frankly, neither one would work very well; if I had to choose one it would definitely be 0x2022.

And that's just with the default font.  The problem with fonts is that web pages can and do control them.  See the testcase in the URL field for a simple example.  I'll attach a screenshot of how it renders on the system here.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: Rendering in Firefox

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Screenshot on same system with different default monospace font

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213587 [details] [diff] [review]
simplified patch

Just based on those two screenshots, this is no good.

Frankly, I doubt we can make this work with any characters which are not _very_ uniform across fonts.  Compare what our bullet-drawing code for lists has to do for disc bullets...

Comment 44

[Kathleen :Brade]

For future reference:
  http://www.fileformat.info/info/unicode/char/25cf/browsertest.htm
  http://www.fileformat.info/info/unicode/char/2022/browsertest.htm

Comment 45

[timeless]

bz: is there some way we could make the code actually use listitem:bullets instead of characters?

it seems like if something has already been written to solve that problem, we shouldn't try to reinvent it :)

Comment 46

[Boris Zbarsky [:bzbarsky]]

brade, do you want to see the screenshots from those two pages?

timeless: not easily....  would need some surgery somewhere.

Comment 47

[Simon Fraser [no longer active]]

What if we style password inputs in forms.css to use a known font?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Pages can override unless it's !important.  And even then there's no guarantee the font is installed on the user's system.

Comment 49

[Jungshik Shin]

(In reply to comment #43)
> (From update of attachment 213587 [details] [diff] [review] [edit])
> Just based on those two screenshots, this is no good.
> 
> Frankly, I doubt we can make this work with any characters which are not _very_
> uniform across fonts.  Compare what our bullet-drawing code for lists has to do
> for disc bullets...

 I fully agree with bz and I doubt we can do anything here other than some surgery mentioned in comment #46. Various screenshots of various pages in different encodings attached here don't tell much let alone guranteeting anything. 

Comment 50

[Mark Smith [:mcs]]

It seems to me that if the issue is a matter of variation among fonts, we can do what Simon suggested in comment #47 as long as we ensure that the font we pick is always available on the user's system.  For Windows and MacOS at least there are some fonts that are always installed, right?

Another way to look at this bug is that at least two vendors (Apple with Safari and Microsoft with IE) have solved this issue, albeit possibly in a platform-specific way.  Given that, it is disappointing that we are not able to do something too (even if only for certain platforms or when certain fonts are used).

Comment 51

[Boris Zbarsky [:bzbarsky]]

You're assuming that IE and Safari use characters from fonts for their bullets.  For Safari, you could verify this assumption if you want -- the code is open and all.  So we could in fact do whatever they do; just have to look up what they do.

Comment 52

[[e-mail]]

(In reply to comment #48)
>And even then there's no guarantee the font is installed on the user's system.
font-family: -moz-fixed !important;

Comment 53

[Boris Zbarsky [:bzbarsky]]

That's not a specific font, Neil.

Comment 54

[Simon Montagu :smontagu]

(In reply to comment #51)
> You're assuming that IE and Safari use characters from fonts for their bullets.
>  For Safari, you could verify this assumption if you want -- the code is open
> and all.  So we could in fact do whatever they do; just have to look up what
> they do.

If (as I assume) Safari uses native widgets, it doesn't help us.

Comment 55

[[e-mail]]

Joke pref: editor.password_mask (default "*") that users can set to a bullet or other string of their choice (e.g. "password"). Makes typing passwords fun!

Comment 56

[J Luh]

(In reply to comment #51)
> You're assuming that IE and Safari use characters from fonts for their bullets.
>  For Safari, you could verify this assumption if you want -- the code is open
> and all.  So we could in fact do whatever they do; just have to look up what
> they do.
> 

This page

<http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGUserInput/chapter_11_section_5.html#//apple_ref/doc/uid/TP30000361-TPXREF63>

makes me think that on Mac OS X you can just use a bullet character. I think the best approach is to fix this on Mac and leave other platforms for the future.

Comment 57

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Implement this in nsILookAndFeel

This really belongs in LookAndFeel.  GTK+ has a setting for this which applies to all GTK+ entry widgets in password mode.  This makes gecko match the native GTK+ setting.  Maybe other toolkits have similar ways to get this information out.

Comment 58

[[e-mail]]

(In reply to comment #57)
>Maybe other toolkits have similar ways to get this information out.
In Windows you would probably base it on the OS version (L'*' if <= 5.0).

Comment 59

[Boris Zbarsky [:bzbarsky]]

Chris, how does this address the issues I mention in comment 40 and following?

Note that GTK, unlike us, doesn't have to deal with arbitrary font styles...

Comment 60

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #59)
> Chris, how does this address the issues I mention in comment 40 and following?
> 
> Note that GTK, unlike us, doesn't have to deal with arbitrary font styles...
> 

Boris, presumably your GTK+ setup would not default to characters that aren't guaranteed on the system.  For example, the default in GTK+ head is the traditional '*' but in FC5 and later, we guarantee fonts are installed which will provide U+2022 and it is uniform across most of the fonts installed in the distribution, thus Fedora's GTK+ version uses U+2022 as the default character.  Of course, users can install their own fonts which don't have a glyph for U+2022, and webpages can otherwise do things which look like crap, but that can't be prevented.

Comment 61

[Christopher Aillon (sabbatical, not receiving bugmail)]

Also, it might be worth noting that perhaps we should restrict websites from changing the font face of password fields.

Comment 62

[Boris Zbarsky [:bzbarsky]]

OK, I think I buy both comment 60 and comment 61.  Especially comment 61.

Comment 63

[Christopher Aillon (sabbatical, not receiving bugmail)]

Okay, bug 357258 filed for comment 61.

Comment 64

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 242663 [details] [diff] [review]
Implement this in nsILookAndFeel

Seeking reviews.

Comment 65

[Kathleen :Brade]

Comment on attachment 242663 [details] [diff] [review]
Implement this in nsILookAndFeel

I like this approach.  However, I don't like the use of "invisible" in nsLookAndFeel* files.  The bullet character is not invisible, it is obscured.  I prefer GetPasswordCharacter or sPasswordSymbol or similar.

For someone who is developing on Windows, there is EM_GETPASSWORDCHAR.  I'm not sure if we can use that or not.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Chris, I'm pretty totally swamped on reviews; I can't sr this in a reasonable timeframe....  Try roc maybe?  Or one of the other srs?

Comment 67

[[e-mail]]

From MSDN:

Windows XP: If an edit control is from user32.dll, an asterisk is the default character for the ES_PASSWORD style. However, if an edit control is from comctl32.dll version 6, a black circle is the default character for the ES_PASSWORD style. Note that comctl32.dll version 6 is not redistributable but is included with Microsoft Windows XP or later. To use comctl32.dll version 6, specify it in a manifest.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should just get the character in EchoInsertionToPWBuff. It is not worth doing it for every edit control. Don't bother caching it either, I think.

Comment 69

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #68)
> I think you should just get the character in EchoInsertionToPWBuff. It is not
> worth doing it for every edit control. Don't bother caching it either, I think.

Um, I _am_ getting it in EchoInsertionToPWBuff.  The cache is minimal -- no need to create and destroy a new GtkEntry each time we have a password control to edit.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're caching the service in nsTextEditRules. I think you shouldn't, it's usually not needed.

Comment 71

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #70)
> You're caching the service in nsTextEditRules. I think you shouldn't, it's
> usually not needed.

Oh the service!  Your prior comment made it sound like you objected to caching the character.  Sure, I can get make that change.  Anything else?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with Brade's naming comment #65. The rest looks fine.

Comment 73

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Once More With nsILookAndFeel-ing (checked in)

Changes the method name and no longer caches the service.  I kept the static variable name in the GTK+2 implementation the same though, to match GTK+ naming.

Comment 74

[[e-mail]]

Comment on attachment 242892 [details] [diff] [review]
Once More With nsILookAndFeel-ing (checked in)

>   PRInt32 length = aOutString->Length();
>   PRInt32 i;
>   aOutString->Truncate();
>   for (i=0; i<length; i++)
>   {
>-    aOutString->Append(PRUnichar('*'));
>+    aOutString->Append(passwordChar);
>   }
I can't help thinking there must be an easier way to do this, but I'm no string API guru so the best I could come up with was something like this:
nsCharTraits<PRUnichar>::assign(aOutString.BeginWriting(), aOutSTring->Length(), passwordChar);

Comment 75

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 242892 [details] [diff] [review]
Once More With nsILookAndFeel-ing (checked in)

Patch committed.  I'll let others decide what to do with other toolkits.

Comment 76

[Håkan Waara]

Attachment: Mac fix (checked in)

This makes mac use bullets instead of asterisks.

Comment 77

[Håkan Waara]

Attachment: Screenshot with above patch (mac)

Comment 78

[[e-mail]]

Attachment: Possible Windows fix

Comment 79

[Håkan Waara]

Comment on attachment 243257 [details] [diff] [review]
Mac fix (checked in)

Checked in.

Comment 80

[Oliver Saier]

On Linux I don't see any bullet or asterisk.

Comment 81

[Sergei Dolgov]

SeaMonkey BeOS from April 26, 2006 trunk
Unknown char and little bullet in first line after "non-monospace text"
Big bullet and little buller in second line.

Should I change something in our nsLookAndFeel ?

Comment 82

[:Gavin Sharp [email: [e-mail]]]

*** Bug 361958 has been marked as a duplicate of this bug. ***

Comment 83

[Mikko Rantalainen]

Linux nightly build fails to display bullets inside password input fields if the input is styled. For example, go to http://wordpress.com/wp-login.php and write anything to password field: the cursor moves but no bullets appear. If you select View - Style - No Style the bullets reappear as expected. Firefox 1.5.x correctly displays asterisks no matter which style is used.
Tested with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070205 Minefield/3.0a2pre

Comment 84

[[e-mail]]

(In reply to comment #81)
> SeaMonkey BeOS from April 26, 2006 trunk
> Unknown char and little bullet in first line after "non-monospace text"
> Big bullet and little buller in second line.
> 
> Should I change something in our nsLookAndFeel ?
Unless you changed it, it should default to asterisks, no?

Comment 85

[Arthur]

From reading the source code I don't understand why, but on Ubuntu Feisty, using the Minefield nightly 2007052004 I get placeholder boxes saying "25CE" in html password fields and http authentication prompts (meaning that it wants to display a unicode 0x25CE bullseye but none is available). Huh?  widget/src/gtk2/nsLookAndFeel.cpp says
PRUnichar nsLookAndFeel::sInvisibleCharacter = PRUnichar('*');
so I don't see where this comes from.

Comment 86

[Arthur]

Oops. It's actually Ox25CF (Unicode Black Circle or rather a bullet). Other apps like Gnumeric or Kate display the same character (Copy and pasted from Minefield) without problems. So there's a problem in how fonts are chosen.

Back to the problem at hand: GDB says that /home/arthur/bigmedia62GB/a/mozilla/mozilla/widget/src/gtk2/nsLookAndFeel.cpp
 671 {
 672     return sInvisibleCharacter;
 673 }
returns 9679 (0x25CF)

sInvisibleCharacter gets initially set to
PRUnichar nsLookAndFeel::sInvisibleCharacter = PRUnichar('*');
and but is then looked up here:
 660     // invisible character styles
 661     GtkWidget *entry = gtk_entry_new();
 662     guint value;
 663     g_object_get (entry, "invisible-char", &value, NULL);
 664     sInvisibleCharacter = PRUnichar(value);
 665     gtk_widget_destroy(entry);
But I couldn't get GDB to honor a breakpoint I've set there so I don't know when (and actually whether) that code get's called.

Comment 87

[Adam Guthrie]

Arthur, could you file a separate bug on that issue and make it block this bug? Thanks.

Comment 88

[Dão Gottwald [:dao]]

Why isn't this bug resolved yet? What's left to do here?

Comment 89

[Kathleen :Brade]

Is there a fix for Linux?  Was the Windows patch ever landed?

Comment 90

[Simon Montagu :smontagu]

Neil landed the Windows patch on 2006-10-26. The fix for Linux is in attachment 242892 [details] [diff] [review], also landed, and it WFM on all 3 platforms (If I wanted to bikeshed I would say that the bullet characters are still too small on OS X, but they're the same as Safari).