547224 - Remove the custom emptyText implementation, implement textbox.placeholder using the input field's native placeholder facility

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Mounir Lamouri (:mounir)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6) Gecko/20100114 Gentoo Firefox/3.5.6
Build Identifier: 

The XUL emptyText attribute looks to behave the same as HTML 5 placeholder attribute. Maybe the placeholder attribute should be used instead of emptyText.

Reproducible: Always

Comment 1

[(no longer active)]

I think this is a reasonable thing to do, unless there is something more to emptytext than what meets the eye.

Confirming as an enhancement for now.

Comment 2

[Dão Gottwald [:dao]]

Attachment: untested wip

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Neil Deakin]

Is there a reason to break compatibility here? Why isn't placeholder=emptytext sufficient?

Comment 5

[Dão Gottwald [:dao]]

Calling the property and the attribute "placeholder" makes a lot of sense, since it's the same thing. This will also make it consistent with the -moz-placeholder pseudo class/element.

The patch is backwards compatible, except that setting the attribute later wouldn't have an effect:

+      <property name="emptyText"  onset="this.placeholder = val; return val;"
+                                  onget="return this.placeholder;"/>

       <constructor><![CDATA[
...
+        if (this.hasAttribute("emptytext"))
+          this.placeholder = this.setAttribute("emptytext");

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

updated to tip and fixed some -moz-placeholder selectors

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9ee18dec071b

Comment 8

[Dão Gottwald [:dao]]

Apparently this resulted in a Txul decrease.

Comment 9

[Robert Kaiser]

This needs followups in SeaMonkey, Thunderbird and probably calendar as well, right?

Comment 10

[Dão Gottwald [:dao]]

Not necessarily, no. emptyText/emptytext should still work.

Comment 11

[Robert Kaiser]

OK, good. But it still might be a good idea to migrate over the other apps as well - at lease when targeting 1.9.3, right?

Comment 12

[Dão Gottwald [:dao]]

Yes, placeholder is the new primary property / attribute, emptyText exists only for backwards compatibility.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

All sorts of Ts Shutdown improvements got tied to this bug, if that can be believed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So the -moz-placeholder pseudoclass this patch uses doesn't actually exist....

Comment 15

[Dão Gottwald [:dao]]

Yes, bug 457801 is in the dependency list.

Comment 16

[[e-mail]]

So placeholder text can't be themed until that bug is fixed?

Comment 17

[Dão Gottwald [:dao]]

Correct.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Yes, bug 457801 is in the dependency list.

Of course it's not clear that bug 457801 will add this pseudo-class...

Comment 19

[Dão Gottwald [:dao]]

Sure, but that's not a showstopper for this bug: The CSS can be changed if bug 457801 takes a different route. It can be commented out if that bug remains unfixed. But we wouldn't stick to the previous emptyText implementation just for being able to style it, especially since it proved to be a significant performance factor.

Comment 20

[Boris Zbarsky [:bzbarsky]]

That's fine; I'm just questioning the need to inflict slowdowns due to the CSS error reporting and error console noise on everyone in the meantime.  Seems to me like it would be better to add the placeholder styling after bug 457801 lands (likely to take several weeks).  I have no issues with any of the rest of this patch!

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> That's fine; I'm just questioning the need to inflict slowdowns due to the CSS
> error reporting and error console noise on everyone in the meantime.  Seems to
> me like it would be better to add the placeholder styling after bug 457801
> lands (likely to take several weeks).

I've commented it out:

http://hg.mozilla.org/mozilla-central/rev/56bf1604481a

Comment 22

[[e-mail]]

Comment on attachment 429971 [details] [diff] [review]
patch

>+      <property name="value"      onset="this.inputField.value = val; return val;"
>+                                  onget="return this.inputField.value;"/>
>       <property name="defaultValue" onset="this.inputField.defaultValue = val; return val;"
>                                   onget="return this.inputField.defaultValue;"/>
>       <property name="label"      onset="this.setAttribute('label', val); return val;"
>                                   onget="return this.getAttribute('label') ||
>                                                 (this.labelElement ? this.labelElement.value :
>-                                                 this.emptyText);"/>
>-      <property name="emptyText"  onget="return this.getAttribute('emptytext') || '';"
>-                                  onset="this.setAttribute('emptytext', val);
>-                                         this._updateVisibleText(); return val;" />
>+                                                 this.placeholder);"/>
>+      <property name="placeholder" onset="this.inputField.placeholder = val; return val;"
>+                                  onget="return this.inputField.placeholder;"/>
>+      <property name="emptyText"  onset="this.placeholder = val; return val;"
>+                                  onget="return this.placeholder;"/>
>       <property name="type"       onset="if (val) this.setAttribute('type', val);
>                                          else this.removeAttribute('type'); return val;"
>                                   onget="return this.getAttribute('type');"/>
>       <property name="maxLength"  onset="this.inputField.maxLength = val; return val;"
>                                   onget="return this.inputField.maxLength;"/>
>       <property name="disabled"   onset="this.inputField.disabled = val;
>                                          if (val) this.setAttribute('disabled', 'true');
>                                          else this.removeAttribute('disabled'); return val;"
[Confusingly some of the properties map to attributes, some directly poke the input field, and some do both...]

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With placeholder in place on 1.9.3 now, we do not longer expose the placeholder value to the value property of the textbox if its value is empty? Is that correct?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> we do not longer expose the placeholder
> value to the value property of the textbox if its value is empty?

textbox.value never exposed the emptytext, but textbox.inputField.value did.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Do we have an automated test which checks that the placeholder text is shown in an input field if there is no value set?

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to comment #25)
> Do we have an automated test which checks that the placeholder text is shown in
> an input field if there is no value set?

Yes, layout/reftests/forms/placeholder/ contains some reftests and placeholder-1-text.html is checking if the placeholder shows correctly when the value is empty.

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to comment #25)
> > Do we have an automated test which checks that the placeholder text is shown in
> > an input field if there is no value set?
> 
> Yes, layout/reftests/forms/placeholder/ contains some reftests and
> placeholder-1-text.html is checking if the placeholder shows correctly when the
> value is empty.

This is actually for XUL textboxes.  Do we have reftests for those as well?  If not, we could use some tests there, I guess.

Comment 28

[Eric Shepherd [:sheppy]]

Documented; these are updated to refer to the new name:

https://developer.mozilla.org/en/XUL/Attribute/emptytext
https://developer.mozilla.org/en/XUL/Property/emptyText

placeholder documented here:

https://developer.mozilla.org/en/XUL/Property/placeholder
https://developer.mozilla.org/en/XUL/Attribute/placeholder

And listed here:

https://developer.mozilla.org/en/XUL/Attribute
https://developer.mozilla.org/en/XUL/Property

Amd noted here:

https://developer.mozilla.org/en/Firefox_4_for_developers#Miscellaneous_XUL_changes