Closed Bug 547224 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: XUL Widgets, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: mounir, Assigned: dao)

References

Details

(Keywords: dev-doc-complete, perf)

Attachments

(1 file, 2 obsolete files)

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
Severity: normal → enhancement
Depends on: 457800
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.
Status: UNCONFIRMED → NEW
Component: XUL → XUL Widgets
Ever confirmed: true
Product: Core → Toolkit
QA Contact: xptoolkit.widgets → xul.widgets
Version: unspecified → Trunk
Attached patch untested wip (obsolete) — Splinter Review
Depends on: 457801
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Attachment #429318 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #429345 - Flags: review?(enndeakin)
Is there a reason to break compatibility here? Why isn't placeholder=emptytext sufficient?
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");
Attached patch patchSplinter Review
updated to tip and fixed some -moz-placeholder selectors
Attachment #429345 - Attachment is obsolete: true
Attachment #429971 - Flags: review?(enndeakin)
Attachment #429345 - Flags: review?(enndeakin)
Blocks: 549823, 471319
Attachment #429971 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/9ee18dec071b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
Apparently this resulted in a Txul decrease.
Keywords: perf
This needs followups in SeaMonkey, Thunderbird and probably calendar as well, right?
Not necessarily, no. emptyText/emptytext should still work.
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?
Yes, placeholder is the new primary property / attribute, emptyText exists only for backwards compatibility.
Blocks: 550186
Summary: HTML 5 'placeholder' attribute should be used instead of 'emptyText' → Remove the custom emptyText implementation, implement textbox.placeholder using the input field's native placeholder facility
All sorts of Ts Shutdown improvements got tied to this bug, if that can be believed.
So the -moz-placeholder pseudoclass this patch uses doesn't actually exist....
Yes, bug 457801 is in the dependency list.
So placeholder text can't be themed until that bug is fixed?
Correct.
Blocks: 550710
No longer blocks: 550710
Depends on: 550710
> Yes, bug 457801 is in the dependency list.

Of course it's not clear that bug 457801 will add this pseudo-class...
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.
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!
(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
Depends on: 551545
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...]
Blocks: 551747
Keywords: dev-doc-needed
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?
(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.
Do we have an automated test which checks that the placeholder text is shown in an input field if there is no value set?
Flags: in-testsuite?
(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.
(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.
You need to log in before you can comment on or make changes to this bug.