Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Toolkit
XUL Widgets
--
enhancement
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mounir, Assigned: dao)

Tracking

({dev-doc-complete, perf})

Trunk
mozilla1.9.3a3
dev-doc-complete, perf
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

51.35 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
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
(Assignee)

Comment 2

8 years ago
Created attachment 429318 [details] [diff] [review]
untested wip
(Assignee)

Updated

8 years ago
Depends on: 457801
(Assignee)

Comment 3

8 years ago
Created attachment 429345 [details] [diff] [review]
patch
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?
(Assignee)

Comment 5

8 years ago
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");
(Assignee)

Comment 6

8 years ago
Created attachment 429971 [details] [diff] [review]
patch

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)
(Assignee)

Updated

8 years ago
Blocks: 549823, 471319
Attachment #429971 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9ee18dec071b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
(Assignee)

Comment 8

8 years ago
Apparently this resulted in a Txul decrease.
Keywords: perf

Comment 9

8 years ago
This needs followups in SeaMonkey, Thunderbird and probably calendar as well, right?
(Assignee)

Comment 10

8 years ago
Not necessarily, no. emptyText/emptytext should still work.

Comment 11

8 years ago
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?
(Assignee)

Comment 12

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

Updated

8 years ago
Blocks: 550186
(Assignee)

Updated

8 years ago
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.

Comment 14

8 years ago
So the -moz-placeholder pseudoclass this patch uses doesn't actually exist....
(Assignee)

Comment 15

8 years ago
Yes, bug 457801 is in the dependency list.
So placeholder text can't be themed until that bug is fixed?
(Assignee)

Comment 17

8 years ago
Correct.

Updated

8 years ago
Blocks: 550710
(Assignee)

Updated

8 years ago
No longer blocks: 550710
Depends on: 550710

Comment 18

8 years ago
> Yes, bug 457801 is in the dependency list.

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

Comment 19

8 years ago
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

8 years ago
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!
(Assignee)

Comment 21

8 years ago
(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

Updated

8 years ago
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...]

Updated

8 years ago
Blocks: 551747

Updated

7 years ago
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?
(Assignee)

Comment 24

7 years ago
(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?
(Reporter)

Comment 26

7 years ago
(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.
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.