888731 - Bugzilla login field should use "placeholder" HTML5 attribute instead of JavaScript

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Frank Wein [:mcsmurf]]

The Bugzilla login form at the bottom of the main bugzilla page seems to use the init_mini_login_form script to pre-file the input form to display "login" (on bugzilla trunk) as gray text in the background. This is bad as this breaks Firefox autocomplete of saved passwords, at least in recent nightly builds (see Bug 886720 Comment 16). Firefox devs are not sure why this worked fine so far. Bugzilla could use the "placeholder" HTML5 attribute there to display the gray text as placeholder in the text field. Then FF autocomplete should work fine.

Comment 1

[Frédéric Buclin]

Bugzilla uses HTML 4.01, not HTML 5. Also, if this is only broken in nightly builds, then this looks like a regression in Firefox.

Comment 2

[Frank Wein [:mcsmurf]]

Maybe it was more like an accidental timing issue that it worked before. See Bug 886720 Comment 16 on that, a Firefox dev commented there on the bugzilla issue.

Comment 3

[Teemu Mannermaa (:wicked)]

Seems to be working fine on Chrome so this looks more and more like a Firefox regression. 

Using HTML5 attributes is not the way to go, as LpSolit already said. I bet adding invalid attributes will only make things worse for other browsers, assuming they ignore invalid attributes correctly in the first place..

There's some special JS for Gecko (and IE and Opera) browsers in the login field handling so maybe that needs to tweaked somehow, or Fx has changed/regressed onDOMReady event?

Comment 4

[Frank Wein [:mcsmurf]]

Do you know how the onDOMReady event is implemented for Gecko? Does it listen for DOMContentLoaded?

Comment 5

[Frank Wein [:mcsmurf]]

Now, if I see this correctly in Bug 839961, the Firefox autocomplete feature listens for the DOMContentLoaded event to start autocomplete. But the autocomplete feature already used DOMContentLoaded before to start autocomplete. Only the code got refactored. So maybe(?) because of the code refactoring the website script now runs before the script via chrome.

Comment 6

[Frank Wein [:mcsmurf]]

Justin: Can you confirm/deny my Comment 5 (Firefox autocomplete used DOMContentLoaded before and after the patch from Bug 839961 to start autocomplete of username/password fields)?

Comment 7

[:glob ✱]

as far as i can tell bugzilla uses timing sensitive code to fake a placeholder, and i don't think it's reasonable to state that it's firefox's fault when this timing changes.  see the lengthy comment in http://bzr.mozilla.org/bugzilla/trunk/annotate/head:/template/en/default/account/auth/login-small.html.tmpl#L65

placeholder has excellent support (http://caniuse.com/#search=placeholder) and worst case will result in an empty field on browsers which don't support it (IE8 and 9).

for bmo we'll be switching to placeholder, via bug 888806.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Frédéric Buclin from comment #1)
> Bugzilla uses HTML 4.01, not HTML 5.

Wow. That's the most ridiculous stop-energy I've heard in quite some time, even by Bugzilla standards.

(In reply to Byron Jones ‹:glob› from comment #7)

> for bmo we'll be switching to placeholder, via bug 888806.

Thanks, you guys rock as always.


It's vaguely possible there's an actual Firefox bug here, but seeing as the Bugzilla code is already questionable and timing-sensitive, placeholder is pretty clearly the right solution in any case.

Comment 9

[Frédéric Buclin]

Attachment: patch, v1

Comment 10

[Frédéric Buclin]

Attachment: patch, v1.1

Better patch. We don't need JS at all thanks to the 'required' attribute which does the validation for us.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8358928 [details] [diff] [review]
patch, v1.1

Review of attachment 8358928 [details] [diff] [review]:
-----------------------------------------------------------------

nice! r=dkl

Comment 12

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #1)
> Bugzilla uses HTML 4.01, not HTML 5.

Not anymore. ;)

Comment 13

[Dave Miller [:justdave]]

Here's the docs for reference: http://www.w3.org/TR/html/forms.html#the-placeholder-attribute

Note the "Note:" in the green box there.  We're not really accounting for that are we...

Comment 14

[Frédéric Buclin]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #13)
> Note the "Note:" in the green box there.  We're not really accounting for
> that are we...

We use placeholder independently of labels. When the label is present and is explicit enough, there is no need for placeholder. When no label is present for some reason, using placeholder is a good alternative.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified js/global.js
modified skins/standard/global.css
modified template/en/default/account/auth/login-small.html.tmpl
Committed revision 8864.