457800 - Implement placeholder attribute for text input fields

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Andy Lyttle]

We should implement the "placeholder" attribute for text input fields. Safari has already implemented it, and WHATWG intends to adopt it as part of Web Forms.

The HTML looks like this:  <input name="zip" placeholder="Zip Code">

The desired behavior is for the placeholder text to appear in the field with a gray color when the field is not focused and the value is empty.

See my post at the attached URL for reasons why this is a good idea, and lots of examples of sites reinventing the wheel poorly.

Comment 1

[Andy Lyttle]

It's official now.
http://dev.w3.org/html5/spec/Overview.html#the-placeholder-attribute

Comment 2

[Rimas Kudelis [use needinfo to ping]]

This property should also be supported for textarea elements.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: First patch

This patch is adding the placeholder attribute for input and textarea elements.
I still did not write the tests (I have to be familiar with mozilla automatic tests).

Let me know if this patch is correct.

Comment 4

[Rimas Kudelis [use needinfo to ping]]

Mounir: I think you should ask for a review. Not sure whose though. Perhaps :bz's...

Comment 5

[Boris Zbarsky [:bzbarsky]]

:ehsan may be a good reviewer here too (or maybe both Ehsan and me).

Some comments from a brief skim:

1) There's no need for the "hidden-overflow" class as far as I can see.
2) Is there a reason why showing/hiding the placeholder on focus is done via
   changing the DOM text instead of just changing the class to trigger a
   visibility change or something?
3) It'd probably be a good idea to take out the random whitespace changes.
4) Changing the value of a currently-focused control to empty looks like it will
   show the placeholder when it shouldn't (see item 2 in the list).
5) ShowPlaceholder with no arguments (or with PR_TRUE), and HidePlaceholder,
   both look like they can destroy |this|.  There should be weakframe checks
   after those calls.
6) I'd somewhat prefer not having that default argument; just pass PR_TRUE in
   the callers as needed.

I'll have to take a careful look at the spec to see whether the placeholder text should start a new bidi embedding level or not (similar to the unicode-bidi thing going on for the editor div); Ehsan, can you comment on what would make sense there?

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Second patch

1) There's no need for the "hidden-overflow" class as far as I can see.

It is needed. Otherwise, the textarea element is resized to fit the placeholder size. And when too long, placeholder for input text can go outside the element.

2) Is there a reason why showing/hiding the placeholder on focus is done via
   changing the DOM text instead of just changing the class to trigger a
   visibility change or something?

Fixed : using classes.

3) It'd probably be a good idea to take out the random whitespace changes.

Fixed

4) Changing the value of a currently-focused control to empty looks like it
will
   show the placeholder when it shouldn't (see item 2 in the list).

Looks like I forgot this case. Fixed.

5) ShowPlaceholder with no arguments (or with PR_TRUE), and HidePlaceholder,
   both look like they can destroy |this|.  There should be weakframe checks
   after those calls.

Fixed. Now, UpdatePlaceholderText is checking for weakFrame.

6) I'd somewhat prefer not having that default argument; just pass PR_TRUE in
   the callers as needed.

I was also wondering if it was a convention to not use default arguments. I've removed default arguments.

>I'll have to take a careful look at the spec to see whether the placeholder
>text should start a new bidi embedding level or not (similar to the
>unicode-bidi thing going on for the editor div); Ehsan, can you comment on what
>would make sense there?

The question is probably stupid but what is a bidi ?

May you review the new patch ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426348 [details] [diff] [review]
Second patch

> It is needed.

The _style_ is needed.  Why is the _class_ needed?

> The question is probably stupid but what is a bidi ?

http://en.wikipedia.org/wiki/Bi-directional_text

Ehsan said he'll take a look first and have me review after.

Comment 8

[(no longer active)]

Comment on attachment 426348 [details] [diff] [review]
Second patch

I'll review this more thoroughly tomorrow, but here are some general comments for now:

1. The HTML5 spec says that newline (\r and \n) characters should be stripped form this attribute.  Your patch needs to handle it.

2. Please do not rename the mAnonymousDiv member and anonymous-div class.  They are unnecessary and make the patch harder to read, and larger than necessary.

3. You will need a set of reftests for this patch, and probably a mochitest as well.

The reftests should reside in this directory: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/, and they need to verify the basic presentation of the placeholder value, and also ensure that things like focusing the control, setting or changing the attribute/property on the node lead to expected results.  The mochitest will probably need to ensure that the attribute/property can be set/retrieved, and setting/changing the value property/attribute does not change it.

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> (From update of attachment 426348 [details] [diff] [review])
> > It is needed.
> 
> The _style_ is needed.  Why is the _class_ needed?

Oh, thanks for the hint. I've removed the class and kept the style.


(In reply to comment #8)
> (From update of attachment 426348 [details] [diff] [review])
> I'll review this more thoroughly tomorrow, but here are some general comments
> for now:
> 
> 1. The HTML5 spec says that newline (\r and \n) characters should be stripped
> form this attribute.  Your patch needs to handle it.

Actually, text wrapping is already making \r and \n useless. I suppose it's not enough so I've added a call to explicitly strip line breaks.
It makes me realized the wrapping is collapsing sequences of whitespace and '\r' or '\n' are only stripped, not converted to whitespaces.
Let me know if this behavior sounds correct (it is not mentionned in the specs).

> 2. Please do not rename the mAnonymousDiv member and anonymous-div class.  They
> are unnecessary and make the patch harder to read, and larger than necessary.

I think this change is needed because 'anonymousDiv' isn't an appropriate name anymore. However, I understand it makes the patch harder to read so I will try to extract this change to another patch.

> 3. You will need a set of reftests for this patch, and probably a mochitest as
> well.
> 
> The reftests should reside in this directory:
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/, and they
> need to verify the basic presentation of the placeholder value, and also ensure
> that things like focusing the control, setting or changing the
> attribute/property on the node lead to expected results.  The mochitest will
> probably need to ensure that the attribute/property can be set/retrieved, and
> setting/changing the value property/attribute does not change it.

Will do that.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Add placeholder attribute and behavior

This patch is taking in accounts previous comment.
'anonymousDiv' renaming has been removed from this patch.
Tests will come later.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Rename mAnonymousDiv and anonymous-div

mAnonymousDiv is renamed to mEditorDiv
anonymous-div is renamed to editor-div

Previous names are inappropriate because of the placeholder div/class added.

Comment 12

[(no longer active)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 426348 [details] [diff] [review] [details])
> > I'll review this more thoroughly tomorrow, but here are some general comments
> > for now:
> > 
> > 1. The HTML5 spec says that newline (\r and \n) characters should be stripped
> > form this attribute.  Your patch needs to handle it.
> 
> Actually, text wrapping is already making \r and \n useless. I suppose it's not
> enough so I've added a call to explicitly strip line breaks.
> It makes me realized the wrapping is collapsing sequences of whitespace and
> '\r' or '\n' are only stripped, not converted to whitespaces.
> Let me know if this behavior sounds correct (it is not mentionned in the
> specs).

Well, the spec just says that the UA should remove \r and \n from the string before displaying it to the user.  So, "a\nb" should appear as "ab", not "a b", therefore you need to explicitly strip the string from those characters.

Comment 13

[(no longer active)]

Comment on attachment 426488 [details] [diff] [review]
Add placeholder attribute and behavior

>diff -r 5e174f186d31 dom/interfaces/html/nsIDOMNSHTMLInputElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLInputElement.idl	Fri Jan 29 08:42:00 2010 +0000
>+++ b/dom/interfaces/html/nsIDOMNSHTMLInputElement.idl	Thu Feb 11 12:52:45 2010 +0100
>@@ -53,16 +53,18 @@ interface nsIDOMNSHTMLInputElement : nsI
>            attribute long             selectionEnd;
> 
> 	readonly attribute nsIDOMFileList   files;
> 
>            attribute boolean          indeterminate;
> 
>            attribute boolean          multiple;
> 
>+           attribute DOMString        placeholder;
>+

You need to change the interface's IID as well.

>diff -r 5e174f186d31 dom/interfaces/html/nsIDOMNSHTMLTextAreaElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLTextAreaElement.idl	Fri Jan 29 08:42:00 2010 +0000
>+++ b/dom/interfaces/html/nsIDOMNSHTMLTextAreaElement.idl	Thu Feb 11 12:52:45 2010 +0100
>@@ -46,12 +46,13 @@ interface nsIControllers;
> interface nsIDOMNSHTMLTextAreaElement : nsISupports
> {
>   readonly attribute nsIControllers   controllers;
> 
>   readonly attribute long    textLength;
>   attribute long             selectionStart;
>   attribute long             selectionEnd;
>   attribute long             maxLength;
>+  attribute DOMString        placeholder;

Ditto.

>diff -r 5e174f186d31 layout/forms/nsTextControlFrame.cpp
>--- a/layout/forms/nsTextControlFrame.cpp	Fri Jan 29 08:42:00 2010 +0000
>+++ b/layout/forms/nsTextControlFrame.cpp	Thu Feb 11 13:23:59 2010 +0100
>@@ -1095,16 +1095,18 @@ nsTextControlFrame::DestroyFrom(nsIFrame
> {
>   if (mInSecureKeyboardInputMode) {
>     MaybeEndSecureKeyboardInput();
>   }
>   if (!mDidPreDestroy) {
>     PreDestroy();
>   }
>   nsContentUtils::DestroyAnonymousContent(&mAnonymousDiv);
>+  nsContentUtils::DestroyAnonymousContent(&mPlaceholderDiv);
>+  nsContentUtils::DestroyAnonymousContent(&mPlaceholderText);

mPlaceholderText is not registered as anonymous content, so it shouldn't be destroyed in this way here.

>   nsBoxFrame::DestroyFrom(aDestructRoot);
> }
> 
> nsIAtom*
> nsTextControlFrame::GetType() const 
> { 
>   return nsGkAtoms::textInputFrame;
> } 
>@@ -1258,17 +1260,17 @@ nsTextControlFrame::CalcIntrinsicSize(ns
>     if (PresContext()->CompatibilityMode() == eCompatibility_FullStandards) {
>       aIntrinsicSize.width += 1;
>     }
> 
>     // Also add in the padding of our anonymous div child.  Note that it hasn't
>     // been reflowed yet, so we can't get its used padding, but it shouldn't be
>     // using percentage padding anyway.
>     nsMargin childPadding;
>-    if (GetFirstChild(nsnull)->GetStylePadding()->GetPadding(childPadding)) {
>+    if (GetLastChild(nsnull)->GetStylePadding()->GetPadding(childPadding)) {

Is there any specific reason why you chose the placeholder div to be the first child?  I think if you add it after mAnonymousDiv, then all these s/FirstChild/LastChild/ changes can be avoided.

Also, I'm not sure how this works here, really.  Setting visibility to hidden still makes the placeholder div to take the same vertical space as when it's visible, right?

I would expect that you need some CSS trickery to make sure that both div's have the same |top|, and placeholder overlays the other one using z-index.

>       aIntrinsicSize.width += childPadding.LeftRight();
>     } else {
>       NS_ERROR("Percentage padding on anonymous div?");
>     }
>   }
> 
>   // Increment width with cols * letter-spacing.
>   {
>@@ -2379,16 +2391,26 @@ nsTextControlFrame::AttributeChanged(PRI
>     }
>     else 
>     { // unset disabled
>       flags &= ~(nsIPlaintextEditor::eEditorDisabledMask);
>       mSelCon->SetDisplaySelection(nsISelectionController::SELECTION_HIDDEN);
>     }    
>     mEditor->SetFlags(flags);
>   }
>+  else if (nsGkAtoms::placeholder == aAttribute)
>+  {
>+    nsAutoString placeholderValue;
>+
>+    mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholderValue);
>+    RemoveNewlines(placeholderValue);
>+    mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholderValue, PR_FALSE);

The spec only mentions that newline characters should be stripped from the placeholder string before displaying them.  So you shouldn't modify the attribute's value here; you should just use the modified placeholder inside UpdatePlaceholderText.

>+    UpdatePlaceholderText(PR_TRUE);
>+  }
>   // Allow the base class to handle common attributes supported
>   // by all form elements... 
>   else {
>     rv = nsBoxFrame::AttributeChanged(aNameSpaceID, aAttribute, aModType);
>   }
> 
>   return rv;
> }

>+nsresult
>+nsTextControlFrame::UpdatePlaceholderText(PRBool aNotify)
>+{
>+  nsWeakFrame weakFrame(this);
>+
>+  nsAutoString placeholderValue;
>+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholderValue);

You probably need to call RemoveNewLines here instead of the other place.

>+  mPlaceholderText->SetText(placeholderValue, aNotify);
>+
>+  NS_ENSURE_STATE(weakFrame.IsAlive());
>+
>+  return NS_OK;
>+}
>diff -r 5e174f186d31 layout/forms/nsTextControlFrame.h
>--- a/layout/forms/nsTextControlFrame.h	Fri Jan 29 08:42:00 2010 +0000
>+++ b/layout/forms/nsTextControlFrame.h	Thu Feb 11 12:52:45 2010 +0100
>@@ -310,19 +310,28 @@ protected:
>                              nsSize&              aIntrinsicSize);
> 
> private:
>   //helper methods
>   nsresult SetSelectionInternal(nsIDOMNode *aStartNode, PRInt32 aStartOffset,
>                                 nsIDOMNode *aEndNode, PRInt32 aEndOffset);
>   nsresult SelectAllContents();
>   nsresult SetSelectionEndPoints(PRInt32 aSelStart, PRInt32 aSelEnd);
>+
>+  // placeholder methods
>+  nsresult CreatePlaceholderDiv(nsTArray<nsIContent*>& aElements, nsNodeInfoManager* pNodeInfoManager);
>+  nsresult ShowPlaceholder();
>+  nsresult HidePlaceholder();
>+  nsresult SetPlaceholderClass(PRBool aVisible, PRBool aNotify);
>+  nsresult UpdatePlaceholderText(PRBool aNotify);
>   
> private:
>   nsCOMPtr<nsIContent> mAnonymousDiv;
>+  nsCOMPtr<nsIContent> mPlaceholderDiv;
>+  nsCOMPtr<nsIContent> mPlaceholderText;

You really don't need to store mPlaceholderText as a member, you can always count on it being the first child of mPlaceholderDiv.

>   nsCOMPtr<nsIEditor> mEditor;
> 
>   // these packed bools could instead use the high order bits on mState, saving 4 bytes 
>   PRPackedBool mUseEditor;
>   PRPackedBool mIsProcessing;
>   PRPackedBool mNotifyOnInput;//default this to off to stop any notifications until setup is complete
>   PRPackedBool mDidPreDestroy; // has PreDestroy been called
>diff -r 5e174f186d31 layout/style/forms.css
>--- a/layout/style/forms.css	Fri Jan 29 08:42:00 2010 +0000
>+++ b/layout/style/forms.css	Thu Feb 11 13:08:53 2010 +0100
>@@ -109,16 +109,31 @@ input {
> }
> 
> input > .anonymous-div {
>   word-wrap: normal !important;
>   /* Make the line-height equal to the available height */
>   line-height: -moz-block-height;
> }
> 
>+input > .placeholder,
>+textarea > .placeholder {
>+  color: darkGrey;
>+  overflow: hidden;
>+}

I think you need ui-review here about the appearance of the placeholder text.  Let's try Alex.

>+input > .placeholder.nowrap {
>+  white-space: nowrap;
>+}

I'm not sure why you need the nowrap class.  Couldn't you just use |input > .placeholder| instead, and don't set the nowrap class at all?


And, you still need the tests.

Comment 14

[(no longer active)]

Comment on attachment 426489 [details] [diff] [review]
Rename mAnonymousDiv and anonymous-div

I think a more accurate name here would be "value div" instead of "editor div".

Comment 15

[(no longer active)]

You should probably file an accessibility bug for this attribute as well, so that we can figure out if/how we want to expose this stuff through a11y.

CCing David here for now so that he knows what's going to happen here.

Comment 16

[Marco Zehe (:MarcoZ)]

We already have the emptyText attribute for XUL:textbox (for example in the search field of the Add-Ons Manager's "Get Add-Ons" page). What we do in accessibility is convert that emptyText property into the label for the textbox (the accessible name of the textbox accessible). IO think we should do the same here for the placeholder value.

The one thing we need to decide on is this:
What if there is also a label associated? Will we have the placeholder text as the accessible description then and the label as the accessible name? In XUL, this is not an issue because the usecase for emptytext is exactly this that we don't need an extra label. Is the HTML case the same? If so we don't have this conflict and should always use placeholder if present.

Thoughts or expected use cases?

Comment 17

[Rimas Kudelis [use needinfo to ping]]

The standard says it:
The placeholder attribute should not be used as an alternative to a label.

Comment 18

[(no longer active)]

(In reply to comment #5)
> I'll have to take a careful look at the spec to see whether the placeholder
> text should start a new bidi embedding level or not (similar to the
> unicode-bidi thing going on for the editor div); Ehsan, can you comment on what
> would make sense there?

The spec doesn't mention anything in this regard, but since this value is supposed to be displayed independently, I would imagine that we need a similar unicode-biti rule for the placeholder div as well.

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #17)
> The standard says it:
> The placeholder attribute should not be used as an alternative to a label.

I filed bug 545817 for the accessibility-specific discussion and implementation of this attribute and what to derive from the spec.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Tests

Mochitests and reftests for placeholders.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch (v0.3)

This patch is fixing every issue from comment 13.

> Is there any specific reason why you chose the placeholder div to be the first
> child?  I think if you add it after mAnonymousDiv, then all these
> s/FirstChild/LastChild/ changes can be avoided.

I changed to GetLastChild because when the placeholder was the last child of the StackFrame, it was taking every events. Actually, I did that because placeholder was hidden by setting text to "".  Now, because it is hidden by CSS, it can be the last frame of the stack.

Comment 22

[Mounir Lamouri (:mounir)]

Attachment: Patch (v0.4)

Few fixes here :
- a typo
- the placeholder div is using 'anonymous-div' class instead of adding 'placeholder' class to all 'anonymous-div' rules.

Consequently, the renaming should be done only from mAnonymousDiv to mValueDiv. 'anonymous-div' could be kept as a generic class.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Renaming patch

The renaming patch is now renaming mAnonymousDiv to mValueDiv and isn't changing the anonymous-div class name.

Comment 24

[(no longer active)]

Comment on attachment 426678 [details] [diff] [review]
Patch (v0.4)

Nit: please attach patches with context set to 8 for review.

>diff -r 58b0ec3224c0 layout/forms/nsTextControlFrame.cpp
>+  // Now create the placeholder frame

This comment is wrong.  How about: "Now create the placeholder anonymous content"?

>+  CreatePlaceholderDiv(aElements, doc->NodeInfoManager());
>+
>   return NS_OK;
> }
> 

>+nsresult
>+nsTextControlFrame::UpdatePlaceholderText(PRBool aNotify)
>+{
>+  nsWeakFrame weakFrame(this);
>+  nsAutoString placeholderValue;
>+
>+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholderValue);
>+  RemoveNewlines(placeholderValue);
>+  mPlaceholderDiv->GetChildAt(0)->SetText(placeholderValue, aNotify);

Can you check the return value of GetChildAt for being null, and assert it as well?

r=me with those comments fixed.  Forwarding the review to bz.

Comment 25

[(no longer active)]

Comment on attachment 426651 [details] [diff] [review]
Tests

>diff -r e59c49730ebe content/html/content/test/test_bug457800.html
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/html/content/test/test_bug457800.html	Fri Feb 12 12:42:30 2010 +0100
>@@ -0,0 +1,52 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=457800
>+-->
>+<head>
>+  <title>Test for Bug 457800</title>
>+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=457800">Mozilla Bug 457800</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+  <input type="text" id="inputtext"/><br/>
>+  <input type="password"id="inputpassword"/><br/>
>+  <textarea id="textarea"></textarea>
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for Bug 457800 **/
>+
>+function testPlaceholderForElement(element)
>+{
>+  // init
>+  element.value = ''
>+
>+  // check if {g,s}etAttribute are working
>+  element.setAttribute('placeholder', 'placeholder');
>+  is(element.getAttribute('placeholder'), 'placeholder', "element has no placeholder attribute");

Our usual convention for the 3rd argument of |is| is stating what you're testing.  For example, you should probably use "Make sure the placeholder attribute works" here.  Same for the rest of the patc

>+  // check if placeholder is working through DOM interface
>+  is(element.placeholder, 'placeholder', "can't read placeholder in DOM interface");
>+  element.placeholder = 'ph';
>+	is(element.placeholder, 'ph', "can't write placeholder in DOM interface");

Nit: why the indention here?

You would also want this check here as well:

  is(element.getAttribute("placeholder"), "ph", ...);

>+  // check placeholder and value are not interfering
>+  is(element.value, '', "value has changed when placeholder has changed");
>+  element.value = 'value';
>+  is(element.placeholder, 'ph', "placeholder has changed when value has changed");
>+}
>+
>+testPlaceholderForElement(document.getElementById('inputtext'));
>+testPlaceholderForElement(document.getElementById('inputpassword'));
>+testPlaceholderForElement(document.getElementById('textarea'));
>+
>+</script>
>+</pre>
>+</body>
>+</html>

The reftests look great.  I only have two nits:

1. Please set the placeholder color style in a separate css file and use it in all your tests, so that if we change the style in the future, we only have to update one file.
2. No need to put a slash at the end of input tags.

Comment 26

[(no longer active)]

Have you pushed these patches to the try server?  If you don't have commit access, please create an hg bundle of all three patches and attach it here, and I will do that.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426678 [details] [diff] [review]
Patch (v0.4)

ShowPlaceholder, HidePlaceholder, and UpdatePlaceholderText (when true is passed) can all destroy |this|.  There really do need to be weakframe checks in the _callers_ of these methods to bail out if that happens.  SetFocus certainly doesn't have one, nor does AttributeChanged.  

Doesn't the caller of CreatePlaceholderDiv need to check the return value?

The color thing in forms.css is no good because there's no reason the background can't be darkGrey there....  It needs to be at least a system color of some sort.

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to comment #24)
> (From update of attachment 426678 [details] [diff] [review])
> Nit: please attach patches with context set to 8 for review.
> 
> >diff -r 58b0ec3224c0 layout/forms/nsTextControlFrame.cpp
> >+  // Now create the placeholder frame
> 
> This comment is wrong.  How about: "Now create the placeholder anonymous
> content"?

Ok

> >+nsresult
> >+nsTextControlFrame::UpdatePlaceholderText(PRBool aNotify)
> >+{
> >+  nsWeakFrame weakFrame(this);
> >+  nsAutoString placeholderValue;
> >+
> >+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholderValue);
> >+  RemoveNewlines(placeholderValue);
> >+  mPlaceholderDiv->GetChildAt(0)->SetText(placeholderValue, aNotify);
> 
> Can you check the return value of GetChildAt for being null, and assert it as
> well?

Ok


(In reply to comment #25)
> (From update of attachment 426651 [details] [diff] [review])
<snip>
> >+  // check if {g,s}etAttribute are working
> >+  element.setAttribute('placeholder', 'placeholder');
> >+  is(element.getAttribute('placeholder'), 'placeholder', "element has no placeholder attribute");
> 
> Our usual convention for the 3rd argument of |is| is stating what you're
> testing.  For example, you should probably use "Make sure the placeholder
> attribute works" here.  Same for the rest of the patc

MDC says third argument is the error message [1] and tests I've seen are using it as the error message. It has changed recently ?

[1] https://developer.mozilla.org/en/Mochitest

> >+  // check if placeholder is working through DOM interface
> >+  is(element.placeholder, 'placeholder', "can't read placeholder in DOM interface");
> >+  element.placeholder = 'ph';
> >+	is(element.placeholder, 'ph', "can't write placeholder in DOM interface");
> 
> Nit: why the indention here?

Fixed

> You would also want this check here as well:
> 
>   is(element.getAttribute("placeholder"), "ph", ...);

Ok

> The reftests look great.  I only have two nits:
> 
> 1. Please set the placeholder color style in a separate css file and use it in
> all your tests, so that if we change the style in the future, we only have to
> update one file.

Done

> 2. No need to put a slash at the end of input tags.

Not needed but not bad, isn't it ?

Comment 29

[(no longer active)]

(In reply to comment #28)
> (In reply to comment #25)
> > (From update of attachment 426651 [details] [diff] [review] [details])
> <snip>
> > >+  // check if {g,s}etAttribute are working
> > >+  element.setAttribute('placeholder', 'placeholder');
> > >+  is(element.getAttribute('placeholder'), 'placeholder', "element has no placeholder attribute");
> > 
> > Our usual convention for the 3rd argument of |is| is stating what you're
> > testing.  For example, you should probably use "Make sure the placeholder
> > attribute works" here.  Same for the rest of the patc
> 
> MDC says third argument is the error message [1] and tests I've seen are using
> it as the error message. It has changed recently ?
> 
> [1] https://developer.mozilla.org/en/Mochitest

I've seen more tests to actually put the assertion that they're testing in the third argument, but it doesn't matter much, really.

> > 2. No need to put a slash at the end of input tags.
> 
> Not needed but not bad, isn't it ?

Well, <SomeRandomTag> is also not needed, and doesn't appear in many test files in our tree.  ;-)

Seriously, the slash should be there for XHTML, but it actually makes HTML documents invalid, and our HTML parser has to go through the trouble of ignoring it.  The slash has no meaning and is basically invalid markup in HTML.

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to comment #27)
> (From update of attachment 426678 [details] [diff] [review])
> ShowPlaceholder, HidePlaceholder, and UpdatePlaceholderText (when true is
> passed) can all destroy |this|.  There really do need to be weakframe checks in
> the _callers_ of these methods to bail out if that happens.  SetFocus certainly
> doesn't have one, nor does AttributeChanged.  
> 
> Doesn't the caller of CreatePlaceholderDiv need to check the return value?

Ok, should be fixed with my next patches.

> The color thing in forms.css is no good because there's no reason the
> background can't be darkGrey there....  It needs to be at least a system color
> of some sort.

Looking to gtk, i think using mStyle->text[GTK_STATE_INSENSITIVE] may do it. But that means I will have to found something for every platform ?

(In reply to comment #29)
> Well, <SomeRandomTag> is also not needed, and doesn't appear in many test files
> in our tree.  ;-)
> 
> Seriously, the slash should be there for XHTML, but it actually makes HTML
> documents invalid, and our HTML parser has to go through the trouble of
> ignoring it.  The slash has no meaning and is basically invalid markup in HTML.

It's going to be fixed with the next patches too.

Comment 31

[Mounir Lamouri (:mounir)]

Attachment: Tests v2

(previous version of this patch has been reviewed by ehsan)

The tests have been changed to apply ehsan comments.

In addition, I've changed tests related to focus. For those tests, I'm using a readonly input text to prevent blinking caret.
I'm also using reftests-wait class when needed.

Comment 32

[Mounir Lamouri (:mounir)]

Attachment: Patch (v0.5)

(previous version of this patch has been reviewed by ehsan)

I've applied changed requested by ehsan and bz comments.

For the color, I'm using GrayColor which is using a system color.

Comment 33

[Mounir Lamouri (:mounir)]

Attachment: Renaming patch v2

(previous version of this patch has been reviewed by ehsan)

This is an update to rename no use of mAnonymousDiv.

Comment 34

[Mounir Lamouri (:mounir)]

Attachment: hg bundle of the tree patches for the try server

Here is the bundle ehsan requested. At the moment I have no access to the try server (I asked for one).
Thank you for your help ehsan :)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 427585 [details] [diff] [review]
Patch (v0.5)

Looks good to me.  Thanks!

Please ping me if faaborg hasn't gotten to this in the next week?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 427584 [details] [diff] [review]
Tests v2

Might be better to put this under layout/reftests/forms/ instead of layout/reftests/editor.  r=bzbarsky with that.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 427586 [details] [diff] [review]
Renaming patch v2

r=bzbarsky

Comment 38

[(no longer active)]

(In reply to comment #34)
> Created an attachment (id=427589) [details]
> hg bundle of the tree patches for the try server
> 
> Here is the bundle ehsan requested. At the moment I have no access to the try
> server (I asked for one).
> Thank you for your help ehsan :)

Pushed your bundle to try; you can track the progress here: <http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry>.  I'll post a summary when the builds/tests finish.

Comment 39

[(no longer active)]

Three a11y tests failed on Linux, which seems suspicious.  Mac was green (it doesn't include the a11y tests, of course).  Still waiting on Windows.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1266527127.1266536819.10413.gz

Comment 40

[(no longer active)]

The Windows build failed with the exact same a11y test failures.  You need to track these down.  There's a leak in mochitests as well, which I'm pretty sure that I've seen it before, but can't find a random orange bug filed for it...

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1266526426.1266543592.3357.gz

Comment 41

[Mounir Lamouri (:mounir)]

Attachment: Fix some a11y tests

Thank you ehsan.

I was able to found what was happening.
For the test_combobox.xul, the issue is the placeholder is adding a now child so I had to update the test to take it in account. (this is what the patch is doing)
For the nsIAccessibleEditableText.html, the issue is more complex. Actually, "et.insertText(" hello", 10, "mama hello hello");" is failing because " hello" should be added at the end of the string but is added as a placeholder value instead :-/ Help on that issue would be welcomed.

Comment 42

[Mounir Lamouri (:mounir)]

Attachment: Tests v2.1

r=bzbarsky,ehsan

reftests have moved from editor/ to forms/.

Comment 43

[Boris Zbarsky [:bzbarsky]]

That test seems to be modifying the value via the editor directly, right?  ccing some accessibility folks.

Comment 44

[Marco Zehe (:MarcoZ)]

(In reply to comment #41)
> For the test_combobox.xul, the issue is the placeholder is adding a now child
> so I had to update the test to take it in account. (this is what the patch is
> doing)

This is worrysome. Having text leaf nodes as children of textbox accessibles is most certainly not what an assistive technology is expecting.

Did the try builds complete at all? If so, could anyone post the link to the directory where these reside so I can try this? Thanks!

Comment 45

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 427760 [details] [diff] [review]
Fix some a11y tests

>diff -r 332b7215174e accessible/tests/mochitest/tree/test_combobox.xul
>--- a/accessible/tests/mochitest/tree/test_combobox.xul	Thu Feb 18 19:35:18 2010 +0100
>+++ b/accessible/tests/mochitest/tree/test_combobox.xul	Fri Feb 19 15:32:56 2010 +0100

>-            children: []
>+            children: [
>+              {
>+                role: ROLE_TEXT_LEAF // HTML 5 placeholder attribute value
>+              }
>+            ]

>           {
>             role: ROLE_ENTRY,
>-            children: [ ]
>+            children: [
>+              {
>+                role: ROLE_TEXT_LEAF // HTML 5 placeholder attribute value
>+              }
>+            ]

I can see why this is the case given the current patch, and the a11y module's nsXULTextFieldAccessible::CacheChildren (see bug 542824) but like Marco I'm concerned about the implication of this change to our a11y tree and the resultant AT behavior. Try build?

Comment 46

[David Bolter [:davidb] (NeedInfo me for attention)]

Actually this should be okay I think. AT should speak the placeholder when it is available. Would like confirmation though.

Comment 47

[(no longer active)]

(In reply to comment #44)
> (In reply to comment #41)
> > For the test_combobox.xul, the issue is the placeholder is adding a now child
> > so I had to update the test to take it in account. (this is what the patch is
> > doing)
> 
> This is worrysome. Having text leaf nodes as children of textbox accessibles is
> most certainly not what an assistive technology is expecting.

And it is one more reason why a11y should not be walking the native anon content tree in the first place.  ;-)

> Did the try builds complete at all? If so, could anyone post the link to the
> directory where these reside so I can try this? Thanks!

You can find them here: http://build.mozilla.org/tryserver-builds/[e-mail]-try-29ed07bc337d

(In reply to comment #45)
> I can see why this is the case given the current patch, and the a11y module's
> nsXULTextFieldAccessible::CacheChildren (see bug 542824) but like Marco I'm
> concerned about the implication of this change to our a11y tree and the
> resultant AT behavior. Try build?

I think a11y needs to treat the placeholder div differently, and not expose it as children to a11y clients.  Would it be an acceptable behavior?

Comment 48

[(no longer active)]

(In reply to comment #46)
> Actually this should be okay I think. AT should speak the placeholder when it
> is available. Would like confirmation though.

Yes, but it shouldn't be exposing the _div_, only the placeholder value.

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #48)
> (In reply to comment #46)
> > Actually this should be okay I think. AT should speak the placeholder when it
> > is available. Would like confirmation though.
> 
> Yes, but it shouldn't be exposing the _div_, only the placeholder value.

I agree in principle. I need to figure out (or recall) all the reasons we walk anonymous content of xul text fields.

As for the placeholder as long as there is API we can call to get it, that should be enough.

Assuming we need to keep walking children of xul text fields, what is the right way to tell if the child is a placeholder?

Comment 50

[(no longer active)]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #46)
> > > Actually this should be okay I think. AT should speak the placeholder when it
> > > is available. Would like confirmation though.
> > 
> > Yes, but it shouldn't be exposing the _div_, only the placeholder value.
> 
> I agree in principle. I need to figure out (or recall) all the reasons we walk
> anonymous content of xul text fields.

Just a reminder: xul text fields have an html:input as part of their XBL anonymous content...

> As for the placeholder as long as there is API we can call to get it, that
> should be enough.

There is: nsIDOMHTMLInputElement::GetPlaceholder and nsIDOMHTMLTextAreaElement::GetPlaceholder.

> Assuming we need to keep walking children of xul text fields, what is the right
> way to tell if the child is a placeholder?

It is a div with the |placeholder| class.

Comment 51

[Mounir Lamouri (:mounir)]

Daivd, do you think you can write a patch to fix the a11y issues and make sure the placeholder doesn't break a11y normal behavior ?
Otherwise, let me know what I should do exactly.

By the way, we just need to be sure the placeholder is not going to break everything at the moment. Bug 545817 is about using the placeholder for a11y.

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #45)

The placeholder shouldn't change the a11y tree until the placeholder is exposed as accessible value. However I think this patch shouldn't break a11y (but let's wait for Marco's try) and we can figure out the proper fix in bug 545817 unhurriedly. In the case when the placeholder is presented and textfield hasn't value then it should be exposed as a normal value (which is probably not correct but fatal). Otherwise it shouldn't be exposed in a11y tree since it's hidden.

Comment 53

[Marco Zehe (:MarcoZ)]

Here are my results from testing this try server build:

1. NVDA sees the placeholder text as plain text where the textbox is. The text cannot be queried, as is expected, by something like acc_Value. It disappears when the textbox gains focus.

2. JAWS and Window-Eyes completely ignore the placeholder text. They probably don't walk into the text leaf children on textboxes. For them, this behaves like any other textbox.

So I believe it is safe to land this and then work on bug 545817 to properly expose placeholder text.

Comment 54

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #51)
> Daivd, do you think you can write a patch to fix the a11y issues and make sure
> the placeholder doesn't break a11y normal behavior ?
> Otherwise, let me know what I should do exactly.

With respect to accessibility I think you are okay to land thanks.

Comment 55

[(no longer active)]

(In reply to comment #54)
> (In reply to comment #51)
> > Daivd, do you think you can write a patch to fix the a11y issues and make sure
> > the placeholder doesn't break a11y normal behavior ?
> > Otherwise, let me know what I should do exactly.
> 
> With respect to accessibility I think you are okay to land thanks.

Have all the test failures been taken care of?  With attachment 427760 [details] [diff] [review], there is still one failure which need to be fixed.  See comment 40.  Pasting the log here:

3002 INFO Running chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html...
3003 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | before wait for focus -- loaded: loading active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html desired window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html docshell visible: true
3004 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | must wait for load
3005 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | already focused
3006 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | maybe run tests <load:false, focus:true> -- loaded: loading active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html desired window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html docshell visible: true
3007 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html desired window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html docshell visible: true
3008 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | MozAfterPaint event received
3009 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html desired window: ([object XPCNativeWrapper [object Window]]) chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html docshell visible: true
3010 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert hello at 0 to element with ID 'input'
3011 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert ma  at 0 to element with ID 'input'
3012 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert ma at 2 to element with ID 'input'
3013 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert  hello at 10 to element with ID 'input' - got "mama hello", expected "mama hello hello"
3014 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert hello at 0 to element with ID '[object XPCNativeWrapper [object HTMLDocument]]'
3015 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert ma  at 0 to element with ID '[object XPCNativeWrapper [object HTMLDocument]]'
3016 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert ma at 2 to element with ID '[object XPCNativeWrapper [object HTMLDocument]]'
3017 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleEditableText.html | insertText: Can't insert  hello at 10 to element with ID '[object XPCNativeWrapper [object HTMLDocument]]'

Comment 56

[(no longer active)]

Pinging faaborg for the ui-r here.

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 427585 [details] [diff] [review]
Patch (v0.5)

One nit, we need to correctly style this per platform, similar to the self describing text in search-box (bug 388811).  In addition to being GrayText, on Windows Vista and 7 the text needs to be in italics.

Does the spec including when the self describing text is cleared (on focus versus when the user starts typing?)  If not, we need to check that behavior across all of the platforms as well so it will feel native.

Comment 58

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #51)
> > > Daivd, do you think you can write a patch to fix the a11y issues and make sure
> > > the placeholder doesn't break a11y normal behavior ?
> > > Otherwise, let me know what I should do exactly.
> > 
> > With respect to accessibility I think you are okay to land thanks.
> 
> Have all the test failures been taken care of?  With attachment 427760 [details] [diff] [review], there
> is still one failure which need to be fixed.  See comment 40.  Pasting the log
> here:

Oh I didn't see that; of course we need passing tests. That "mama hello hello" failure is very strange given the preceding passing tests.

Comment 59

[Mounir Lamouri (:mounir)]

(In reply to comment #57)
> (From update of attachment 427585 [details] [diff] [review])
> One nit, we need to correctly style this per platform, similar to the self
> describing text in search-box (bug 388811).  In addition to being GrayText, on
> Windows Vista and 7 the text needs to be in italics.

I am wondering how I will be able to test that ? Do you think it can be move to a follow-up bug ?

> Does the spec including when the self describing text is cleared (on focus
> versus when the user starts typing?)  If not, we need to check that behavior
> across all of the platforms as well so it will feel native.

The placeholder has to be shown "when the element's value is the empty string and the control is not focused". So we can't customize the behavior for each platform.

(In reply to comment #58)
> (In reply to comment #55)
> > (In reply to comment #54)
> > > (In reply to comment #51)
> > > > Daivd, do you think you can write a patch to fix the a11y issues and make sure
> > > > the placeholder doesn't break a11y normal behavior ?
> > > > Otherwise, let me know what I should do exactly.
> > > 
> > > With respect to accessibility I think you are okay to land thanks.
> > 
> > Have all the test failures been taken care of?  With attachment 427760 [details] [diff] [review] [details], there
> > is still one failure which need to be fixed.  See comment 40.  Pasting the log
> > here:
> 
> Oh I didn't see that; of course we need passing tests. That "mama hello hello"
> failure is very strange given the preceding passing tests.

It looks like it has been fixed by someone. The test file name has changed and the test passed. I will send a new bundle to the tryserver and see if everything is green this time.

Comment 60

[Mounir Lamouri (:mounir)]

Everything is green with only "Fix some a11y tests" patch. The other issue has been fixed by someone else.

Builds:
https://build.mozilla.org/tryserver-builds/[e-mail]-1267096743

Any recommendation for who I should add for the sr ?

Comment 61

[Alex Faaborg [:faaborg] (Firefox UX)]

Filed spin off bug 548626 for the appearance on modern versions of Windows.

Comment 62

[(no longer active)]

(In reply to comment #59)
> (In reply to comment #57)
> > (From update of attachment 427585 [details] [diff] [review] [details])
> > One nit, we need to correctly style this per platform, similar to the self
> > describing text in search-box (bug 388811).  In addition to being GrayText, on
> > Windows Vista and 7 the text needs to be in italics.
> 
> I am wondering how I will be able to test that ? Do you think it can be move to
> a follow-up bug ?

See bug 548626 comment 1.

Comment 63

[Mounir Lamouri (:mounir)]

Comment on attachment 427760 [details] [diff] [review]
Fix some a11y tests

I don't know if it is really needed because this patch has already be discussed but may you do a review, David ?

Comment 64

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 427760 [details] [diff] [review]
Fix some a11y tests

r=me for the a11y tests as per discussion -- thanks.

Comment 65

[Mounir Lamouri (:mounir)]

(In reply to comment #64)
> (From update of attachment 427760 [details] [diff] [review])
> r=me for the a11y tests as per discussion -- thanks.

Thanks David.
Should I ask roc to sr this patch or someone else would be more appropriate ?

Comment 66

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #65)
> (In reply to comment #64)
> > (From update of attachment 427760 [details] [diff] [review] [details])
> > r=me for the a11y tests as per discussion -- thanks.
> 
> Thanks David.
> Should I ask roc to sr this patch or someone else would be more appropriate ?

I don't think you need sr on this tests-only patch.

Comment 67

[Mounir Lamouri (:mounir)]

Thank you for the review roc :)

Keywording checkin-neeeded.

Comment 68

[(no longer active)]

Landed as http://hg.mozilla.org/mozilla-central/rev/d617a2fced26

Thanks a lot, Mounir, for your patches!

Comment 69

[[e-mail]]

Should it be possible to apply CSS to the placeholder text, in particular overriding the default colour?

Comment 70

[Mounir Lamouri (:mounir)]

(In reply to comment #69)
> Should it be possible to apply CSS to the placeholder text, in particular
> overriding the default colour?

It is the purpose of bug 457801. Actually, there is no specification for that at the moment.

Comment 71

[Eric Shepherd [:sheppy]]

This has been documented:

https://developer.mozilla.org/en/HTML/Element/input