Closed Bug 406095 Opened 17 years ago Closed 16 years ago

add emptyText property to textbox

Categories

(Toolkit :: UI Widgets, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: mconnor, Assigned: dao)

References

Details

(Keywords: access, dev-doc-complete)

Attachments

(1 file, 11 obsolete files)

Attached patch add emptyText property (obsolete) — Splinter Review
An ever increasing number of textboxes within UI have instructional/informational grey text when empty.  We've got a whole bunch of UI elements reinventing the wheel here, I think this is kinda dumb.

Attaching an initial patch for review to add this to textbox, will follow up once that's done with a patch to remove the duplicate impls from across browser and toolkit.
Attachment #290795 - Flags: review?(enndeakin)
Flags: wanted1.9+
Comment on attachment 290795 [details] [diff] [review]
add emptyText property

>+      <property name="emptyText"  onget="return this.getAttribute('emptyText');"
>+                                  onset="this.setAttribute('emptyText', val);
>+                                         this._updateVisibleText(); return val;" />

XUL attributes should be all lowercase. (emptytext)

>+      <method name="_updateVisibleText">
>+        <body><![CDATA[
>+          if (this.value == "" || this.hasAttribute("empty")) {
>+            // This section is a wee bit hacky; without the timeout, the CSS
>+            // style corresponding to the "empty" attribute doesn't kick in
>+            // until the text has changed, leading to an unpleasant moment
>+            // where the engine name flashes black before turning gray.
>+            this.setAttribute("empty", true);

Is this because the style just needs to be flushed?

Otherwise, it looks OK. A test would be nice though.
Shouldn't the binding implement nsIDOMXULLabeledControlElement like searchbar-textbox does? Maybe even call this thing label rather than emptyText from the beginning.
(In reply to comment #2)
> Shouldn't the binding implement nsIDOMXULLabeledControlElement like
> searchbar-textbox does?

No, the label for the textbox is a <label> element beside it, and the emptytext attribute can be used for the text inside.


We seem to use this instead of a <label> element for aesthetic reasons. Not making that accessible doesn't sound sane to me. See bug 392890 for instance.
Blocks: 392890
It can be accessible as long as the emptyText is exposed as the label when there is no other label or tooltiptext (The tooltiptext is better in the case of the main search bar -- it gives details like "search using google")

The other option is to change nsIDOMXULLabelConrolElement to expose the empty text property, and then override ::GetName() in nsXULTextfieldAccessible to use the new emptyText when there is no name returned from an upcall to nsAccessible::GetName().
Because of comment 5, my vote is for a new patch the uses one of those approaches and thus fixes bug 392890 at the same time.
I should explain this better:
> It can be accessible as long as the emptyText is exposed as the label ...
In other words, the <property> "label" which we get from nsIDOMXULLabelControlElement should return emptyText if the other possibilities for providing a label weren't used.
Blocks: 396816
As it seems, this patch sets a different content when the textbox is empty and unfocused, which would result in the undo history problems of bug 338168. Maybe there could be a better approach?
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #290795 - Attachment is obsolete: true
Attachment #294561 - Flags: review?(mconnor)
Attachment #290795 - Flags: review?(enndeakin)
Attached patch patch v2 (obsolete) — Splinter Review
without ignoring whitespace this time
Attachment #294561 - Attachment is obsolete: true
Attachment #294562 - Flags: review?(mconnor)
Attachment #294561 - Flags: review?(mconnor)
Blocks: 338168
Attached patch patch v3 (obsolete) — Splinter Review
drag and drop fixed
Attachment #294562 - Attachment is obsolete: true
Attachment #294566 - Flags: review?(mconnor)
Attachment #294562 - Flags: review?(mconnor)
Attached patch patch v3.1 (obsolete) — Splinter Review
found another problem with the search bar
Attachment #294566 - Attachment is obsolete: true
Attachment #294612 - Flags: review?(mconnor)
Attachment #294566 - Flags: review?(mconnor)
Blocks: 400844
Comment on attachment 294612 [details] [diff] [review]
patch v3.1

>Index: toolkit/content/widgets/textbox.xml

>-    <implementation implements="nsIAccessibleProvider, nsIDOMXULTextBoxElement">
>+    <implementation implements="nsIAccessibleProvider, nsIDOMXULTextBoxElement, nsIDOMXULLabeledControlElement">

If you're going to say that a binding implements an interface, it should actually have implementations of the properties/methods the interface contains. In particular, crop, image, command and accessKey.

>+      <property name="label" readonly="true"
>+                                  onget="return this.labelElement ? this.labelElement.value :
>+                                                this.tooltipText || this.emptyText;"/>

Label should have a setter that just returns 'val' if you want it do nothing, not be readonly.
The getter should not check the tooltipText. (given comment 5, accessibility should deal with that instead, or the search bar should override the label getter.)

Also, what labelElement property are you referring to? Did you mean to extend general.xml#basetext?
(In reply to comment #13)
> If you're going to say that a binding implements an interface, it should
> actually have implementations of the properties/methods the interface contains.
> In particular, crop, image, command and accessKey.
> 
> >+      <property name="label" readonly="true"
> >+                                  onget="return this.labelElement ? this.labelElement.value :
> >+                                                this.tooltipText || this.emptyText;"/>
> 
> Label should have a setter that just returns 'val' if you want it do nothing,
> not be readonly.

Both of this is taken from the search bar binding. As far as I know, it didn't cause problems, but maybe Aaron can comment on this again.

Making the label property fake-writable seems confusing to me. IMHO we should either adhere to breaking the interface or really implement it.

> The getter should not check the tooltipText. (given comment 5, accessibility
> should deal with that instead, or the search bar should override the label
> getter.)

How about a label attribute? This way we could support a setter that actually does something.

> Also, what labelElement property are you referring to? Did you mean to extend
> general.xml#basetext?

The labelElement property is being set here: http://mxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/text.xml#107
Attached patch patch v4 (obsolete) — Splinter Review
made label writable and removed the tooltipText > label mapping
Attachment #294612 - Attachment is obsolete: true
Attachment #294658 - Flags: review?(mconnor)
Attachment #294612 - Flags: review?(mconnor)
Blocks: 407330
Blocks: 407317
Blocks: 404024
Blocks: fox3access
Keywords: access, sec508
Attached patch patch v4 (obsolete) — Splinter Review
updated to trunk
Attachment #294658 - Attachment is obsolete: true
Attachment #295602 - Flags: review?(mconnor)
Attachment #294658 - Flags: review?(mconnor)
Flags: blocking1.9?
Comment on attachment 295602 [details] [diff] [review]
patch v4

From IRC, mconnor is overloaded and suggests that Neil would be able to review this.
Attachment #295602 - Flags: review?(mconnor) → review?(enndeakin)
Yes we want this for ff3
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Comment on attachment 295602 [details] [diff] [review]
patch v4

The patch still needs to implement the rest of nsIDOMXULLabeledControlElement

You should have Gavin review the browser parts.
Attachment #295602 - Flags: review?(enndeakin) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #295602 - Attachment is obsolete: true
Attachment #299755 - Flags: review?(gavin.sharp)
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #299755 - Attachment is obsolete: true
Attachment #299756 - Flags: review?(gavin.sharp)
Attachment #299755 - Flags: review?(gavin.sharp)
Comment on attachment 299756 [details] [diff] [review]
patch v4

Neil for toolkit
Attachment #299756 - Flags: review?(enndeakin)
Attachment #299756 - Flags: review?(enndeakin) → review+
Attached patch patch v5 (obsolete) — Splinter Review
Gavin discovered a bug in the value setter (testcase: http://pastebin.mozilla.org/314051), this fixes it for me.
Attachment #299756 - Attachment is obsolete: true
Attachment #300137 - Flags: review?(gavin.sharp)
Attachment #299756 - Flags: review?(gavin.sharp)
Comment on attachment 300137 [details] [diff] [review]
patch v5

>Index: browser/components/search/content/search.xml

>-    <implementation implements="nsIObserver nsIDOMXULLabeledControlElement">
>+    <implementation implements="nsIObserver">

The "implements" attribute isn't inherited, so if I recall correctly you'll need to revert this change to not break search bar accessibility.

>-      <property name="label" readonly="true"
>-                onget="return '&searchItem.title; ' + 
>-                      document.getBindingParent(this).currentEngine.name;"/>

Ew, this was gross! Glad we're getting rid of it.

>Index: browser/themes/gnomestripe/browser/browser.css

>-#searchbar[empty="true"] > .searchbar-textbox {
>-  color: GrayText;
>-  direction: ltr !important;
>-}
>-
>-#searchbar[empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
>-  direction: ltr !important;
>-  text-align: left !important;
>-}
>-
>-#searchbar[chromedir="rtl"][empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
>-  direction: rtl !important;
>-  text-align: right !important;
>-}

Need to make sure not to regress bug 342007... Have to ask Mano about that, I guess, I don't really know anything about it. Maybe rules like these would be useful in the generic styling?

>Index: toolkit/content/widgets/autocomplete.xml

>-      <property name="label" readonly="true" onget="return this.mInputElt.value;"/>

Is this behavior we want to maintain? It's not clear to me why bug 237249 added this code. Should make sure the URL bar label is still useful with this patch, anyhow (could just file a bug and CC Aaron/Marco).

>       <property name="value"
>-                onget="return this.mInputElt.value;">
>+                onget="return this.hasAttribute('empty') ? '' : this.inputField.value;">
>         <setter><![CDATA[
>           this.mIgnoreInput = true;
>-          this.mInputElt.value = val;
>+          if (val) {
>+            this._clearEmptyText();
>+            this.inputField.value = val;
>+          } else {
>+            this.inputField.value = val;
>+            this._updateVisibleText();
>+          }

Add a comment explaining that the ordering is important, lest someone try to factor out the .value setting? Same comment applies to the textbox.xml setter.

>Index: toolkit/content/widgets/textbox.xml

>+      <method name="_updateVisibleText">

>+                try {
>+                  textbox.editor.transactionManager.beginBatch();
>+                } catch (e) {}

>+      <method name="_clearEmptyText">

>+            try {
>+              this.editor.transactionManager.endBatch();
>+            } catch (e) {}

This is a fix for bug 338168, right? It makes me a bit nervous that we won't necessarily match _updateVisibleText/_clearEmptyText calls because of some focus bug, but I guess the worse that can do is mess up the undo stack, and it's probably unlikely.

>+      <handler event="dragover" phase="capturing">
>+        this._clearEmptyText();
>+      </handler>
>+
>+      <handler event="dragexit" phase="capturing">
>+        this._updateVisibleText();
>+      </handler>

Seems kind of weird for the textbox value to change while the drag is in progress... I'd be fine with just removing these and updating on drop, but I guess it doesn't much matter.
It would be nice to get some basic tests for textbox, too. Perhaps you could make at least some simple tests, based on the existing http://lxr.mozilla.org/seamonkey/source/toolkit/content/tests/widgets/test_textbox_number.xul , and check for e.g. the bug in the value setter?
(In reply to comment #24)
> (From update of attachment 300137 [details] [diff] [review])
> >Index: browser/components/search/content/search.xml
> 
> >-    <implementation implements="nsIObserver nsIDOMXULLabeledControlElement">
> >+    <implementation implements="nsIObserver">
> 
> The "implements" attribute isn't inherited, so if I recall correctly you'll
> need to revert this change to not break search bar accessibility.

Strange, that issue has come up several times before. It is however just a rumour. The interfaces implemented are indeed inherited from base bindings.
 
Comment on attachment 300137 [details] [diff] [review]
patch v5


>       <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);"/>

possibly I missed something but what does @label mean for textbox?
Basically it's the accessible name. So emptyText is used to label the textbox (in gray, when there is no text in it), and we can still get the value of emptyText, and the name, even if the user has typed in the textbox.
(In reply to comment #28)
> Basically it's the accessible name. So emptyText is used to label the textbox
> (in gray, when there is no text in it), and we can still get the value of
> emptyText, and the name, even if the user has typed in the textbox.
> 

1) In general I'm not sure label property should return emptyText. Technically emptyText is
value. It is visible until user input some text. Therefore I guess it's worth
to expose it as value too.

On one hand user can't see the emptyText after input but AT will show it. On
another hand if UI has label for textbox then what do we miss: <label/> or
emptyText?

2) So @label is used only to specify accessible name? I swear no one will use it. I believe ARIA has more chances to be used than this because no one will learn there is @label on textbox which nothing makes but accessible name.
No, emptyText is not changeable by the user -- it's not the value. It's a label that is shown in gray inside a textbox when the textbox isn't focused.

The author doesn't need to use @label. The label in this case is the property for the API which exposes the label text. The author can use emptyText or the original <xul:label control="textBoxId"> method.

Either way they are getting other functionality, and a11y comes for free.
Ok, if emptyText is the sort of the label (accessible name) then we should expose it only if is is visible, should we?

Also if author doesn't need to use @label then its support should be removed from the patch. Correct?
No, don't expose it only when it is visible. It's just a cute trick to save space. But the screen reader user may care at any time what the textbox is for, and that has not changed just because the user is typing in it.

I don't know about the @label question but IIRC in XUL that's another way you can label the textbox, which is just easier than <label control>
Blocks: 414857
Assignee: mconnor → dao
So possibly we should expose it as description rather than name? Because emptyText may contain some long instructions and (for example for textarea) and screen reader will announce it everytime on focus if I get it correctly.
I don't that's the normal case, because:
1) Usually a XUL textbox is not multiline
2) If you put long instructions down, you need to keep them where the user can refer to them while they're filling out the field. Since they would disappear it would not be a good way to put down instructions, so I doubt it will be used that way.

It really is a way of labelling the textfield. Look at the search box to the right of the URL bar.
No longer blocks: 414857
No longer blocks: 407330
Attachment #300137 - Flags: review?(gavin.sharp)
Whiteboard: [needs new patch]
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(In reply to comment #24)
> >-#searchbar[empty="true"] > .searchbar-textbox {
> >-  color: GrayText;
> >-  direction: ltr !important;
> >-}
> >-
> >-#searchbar[empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> >-  direction: ltr !important;
> >-  text-align: left !important;
> >-}
> >-
> >-#searchbar[chromedir="rtl"][empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> >-  direction: rtl !important;
> >-  text-align: right !important;
> >-}
> 
> Need to make sure not to regress bug 342007... Have to ask Mano about that, I
> guess, I don't really know anything about it. Maybe rules like these would be
> useful in the generic styling?

I don't see why the gray text would by default not be displayed in the chrome direction. These rules make the search bar in rtl mode actually behave quite silly.

> >Index: toolkit/content/widgets/autocomplete.xml
> 
> >-      <property name="label" readonly="true" onget="return this.mInputElt.value;"/>
> 
> Is this behavior we want to maintain? It's not clear to me why bug 237249 added
> this code. Should make sure the URL bar label is still useful with this patch,
> anyhow (could just file a bug and CC Aaron/Marco).

There's a plan to add emptyText, and thus a label, to the Location bar.

> >Index: toolkit/content/widgets/textbox.xml
> 
> >+      <method name="_updateVisibleText">
> 
> >+                try {
> >+                  textbox.editor.transactionManager.beginBatch();
> >+                } catch (e) {}
> 
> >+      <method name="_clearEmptyText">
> 
> >+            try {
> >+              this.editor.transactionManager.endBatch();
> >+            } catch (e) {}
> 
> This is a fix for bug 338168, right?

Yes

> >+      <handler event="dragover" phase="capturing">
> >+        this._clearEmptyText();
> >+      </handler>
> >+
> >+      <handler event="dragexit" phase="capturing">
> >+        this._updateVisibleText();
> >+      </handler>
> 
> Seems kind of weird for the textbox value to change while the drag is in
> progress... I'd be fine with just removing these and updating on drop, but I
> guess it doesn't much matter.

The current behaviour of the Search bar looks wrong to me, because the cursor makes it look like you could drop the text somewhere between the empty text.
Attached patch patch v5.1 (obsolete) — Splinter Review
Attachment #300137 - Attachment is obsolete: true
Attachment #301253 - Flags: review?(gavin.sharp)
Whiteboard: [needs new patch]
Version: unspecified → Trunk
Comment on attachment 301253 [details] [diff] [review]
patch v5.1

>Index: toolkit/content/tests/widgets/test_textbox_emptytext.xul

>+SimpleTest.waitForExplicitFinish();

You shouldn't need the SimpleTest prefix in this file at all, the test functions will be in the global scope.
Attachment #301253 - Flags: review?(gavin.sharp) → review+
(In reply to comment #35)
> (In reply to comment #24)
> > >-#searchbar[empty="true"] > .searchbar-textbox {
> > >-  color: GrayText;
> > >-  direction: ltr !important;
> > >-}
> > >-
> > >-#searchbar[empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> > >-  direction: ltr !important;
> > >-  text-align: left !important;
> > >-}
> > >-
> > >-#searchbar[chromedir="rtl"][empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> > >-  direction: rtl !important;
> > >-  text-align: right !important;
> > >-}
> > 
> > Need to make sure not to regress bug 342007... Have to ask Mano about that, I
> > guess, I don't really know anything about it. Maybe rules like these would be
> > useful in the generic styling?
> 
> I don't see why the gray text would by default not be displayed in the chrome
> direction. These rules make the search bar in rtl mode actually behave quite
> silly.
> 
Huh? These rules made it so the gray text does show up in the _chrome direction_ and not in the might-have-been-switched-to-direction of the textfield (for example, in an en-US build, don't show "Search Google" in RTL if the direction of the textbox was switched to RTL, and in a Hebrew build, don't show "חפש בגוגל" in LTR if the direction of the textbox was switched to LTR).

And yes, these rules should be applied to all textboxes using the emptyText attribute.
(In reply to comment #38)
> for example, in an en-US build, don't show "Search Google" in RTL if
> the direction of the textbox was switched to RTL

Why and when would that happen?

> And yes, these rules should be applied to all textboxes using the emptyText
> attribute.

Could you do me a favor and file a follow-up bug for this? Because I don't understand it. All I know is that the search bar is kinda broken right now in RTL mode, which makes it hard to believe that these rules without any change should be applied to all textboxes with graytext.
(In reply to comment #37)
> (From update of attachment 301253 [details] [diff] [review])
> >Index: toolkit/content/tests/widgets/test_textbox_emptytext.xul
> 
> >+SimpleTest.waitForExplicitFinish();
> 
> You shouldn't need the SimpleTest prefix in this file at all, the test
> functions will be in the global scope.

I see the following in SimpleTest.js:

// Global symbols:
var ok = SimpleTest.ok;
var is = SimpleTest.is;
[...]

but nothing like that for |waitForExplicitFinish| or |finish|.
Keywords: checkin-needed
>> for example, in an en-US build, don't show "Search Google" in RTL if
>> the direction of the textbox was switched to RTL

> Why and when would that happen?

Have bidiui turned on, focus the search field, accel+shift+x, write something, remove it, blur.

Now, i heard this is already kinda broken on trunk, so i'm fine with a follow up.
(In reply to comment #40)
> but nothing like that for |waitForExplicitFinish| or |finish|.

Ah, right. My comment only applies to the ok calls, then. I also noticed that you combine conditions and use ok() where you could use is(). Best to separate out conditions so that it's easier to see exactly what failed when something does.
(In reply to comment #42)
> Ah, right. My comment only applies to the ok calls, then. I also noticed that
> you combine conditions and use ok() where you could use is(). Best to separate
> out conditions so that it's easier to see exactly what failed when something
> does.

I separated the conditions but went with ok() in order to test the type, too.
Attachment #301253 - Attachment is obsolete: true
(In reply to comment #43)
> ok() in order to test the type, too.

Note that because of the missing type check, a build without emptyText support passes all tests except "emptyText exposed as label" when using is(). With ok() and ===, only "emptyText not exposed as value" and "value setter/getter works while emptyText is present" succeed using the same build.
(In reply to comment #41)
> Have bidiui turned on, focus the search field, accel+shift+x, write something,
> remove it, blur.
> 
> Now, i heard this is already kinda broken on trunk, so i'm fine with a follow
> up.

I suppose we can just reopen bug 342007. I actually have an idea how to fix it in a less fragile way.
Depends on: 342007
(In reply to comment #34)
> I don't that's the normal case, because:
> 1) Usually a XUL textbox is not multiline
> 2) If you put long instructions down, you need to keep them where the user can
> refer to them while they're filling out the field. Since they would disappear
> it would not be a good way to put down instructions, so I doubt it will be used
> that way.
> 
> It really is a way of labelling the textfield. Look at the search box to the
> right of the URL bar.
> 

But we miss emptyValue with the current patch if textbox is labeled by xul:label.

I think about description because often @emptyText describes an action (not like usual label), for example, msdn has "Search MSDN with Live Search"
mozilla/browser/components/search/content/search.xml 1.120
mozilla/browser/themes/gnomestripe/browser/browser.css 1.174
mozilla/browser/themes/pinstripe/browser/browser.css 1.122
mozilla/browser/themes/winstripe/browser/browser.css 1.167
mozilla/toolkit/content/tests/widgets/Makefile.in 1.49
mozilla/toolkit/content/widgets/autocomplete.xml 1.112
mozilla/toolkit/content/widgets/textbox.xml 1.52
mozilla/toolkit/themes/gnomestripe/global/textbox.css 1.2
mozilla/toolkit/themes/pinstripe/global/textbox.css 1.8
mozilla/toolkit/themes/winstripe/global/textbox.css 1.6
mozilla/toolkit/content/tests/widgets/test_textbox_emptytext.xul initial revision: 1.1
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/places/organizer.css#121
> #searchFilter[empty="true"] {
>   color: GrayText;
> }
I think that we can delete this. 
No longer blocks: 407317
Depends on: 416013
No longer depends on: 342007
Depends on: 414857
Blocks: 415429
Documented.
Added the emptytext attribute to the documentation. I'm also about to add the label property and attribute.
Depends on: 416467
Flags: in-testsuite+
Depends on: 418874
No longer depends on: 423158
Depends on: 471319
Added a long time ago. Works fine in releases like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0 ID:2008061004

Marking verified fixed.
Status: RESOLVED → VERIFIED
Depends on: 487250
You need to log in before you can comment on or make changes to this bug.