Closed Bug 457801 Opened 16 years ago Closed 14 years ago

Implement -moz-placeholder pseudo-class

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mozilla7, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 7 obsolete files)

Gray is a good default when the background-color is white and text color is black, but when that's not the case, it may be appropriate to specify a color for the placeholder text.

I've also submitted this here:
https://bugs.webkit.org/show_bug.cgi?id=21227
Depends on: 457800
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
What would the CSS property do when applied to other elements?

I'm not sure we want a whole bunch of per-element properties; a pseudo-element may make more sense here in some ways.
Indeed, I think we should do a pseudo element, like webkit.
However, I will write a patch with a pseudo element named -moz-placeholder for input and textarea.
I guess we would keep the same syntax as the one implemented in Webkit now?
Nominating for blocking1.9.3, since given that we've added support for placeholder, we should probably have a mechanism for styling it.
Blocks: 457800
No longer depends on: 457800
Why would this be a pseudo element rather than a pseudo class?
Blocks: 547224
My personal feeling is that the pseudo-class is not needed. I don't think there is a real need to style inputs regarding their placeholder attribute. Otherwise, we want to style the placeholder text.

Anyway, it would be better to move to a consensus before implementing something ?

Boris and David, as you took part of the discussion, what is your opinion ? Should I ask the www-style ml ?
Looks like we already have a psuedo-class called ":-moz-placeholder" (see bug 11011).
We can rename that if needed; it's purely internal.
I don't have a good answer to comment 7, really.  David's opinion is linked to from comment 6.
Summary: Implement placeholder-color CSS property (for placeholder text on form inputs) → Implement -moz-placeholder pseudo-class/element (for placeholder text styling on input fields)
(In reply to comment #7)
> My personal feeling is that the pseudo-class is not needed. I don't think there
> is a real need to style inputs regarding their placeholder attribute.
> Otherwise, we want to style the placeholder text.

There surely is a need for styling the placeholder text - esp. its color, as the textbox can get a different background color as well through CSS, so the color of the text rendered on it need to be styleable as well.
I've launch a discussion about it on www-style, see: http://lists.w3.org/Archives/Public/www-style/2010Mar/0095.html
If no consensus (or answers) come from this discussion, I will probably implement it via a pseudo-element. I found it better and it corresponds to the way Webkit implemented it.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Blocks: 551545
Note, Bug 547224 - Remove the custom emptyText implementation, implement textbox.placeholder using the input field's native placeholder facility
is depending on this one, and actually uses -moz-placeholder (but later commented out). Without -moz-placeholder implemented, it is no longer possible to style the placeholder text (previously emptyText) in themes.
This is a kind of regression that should be fixed.
Attached patch Patch - WIP (obsolete) — Splinter Review
This patch is a first try for -moz-placeholder pseudo-element but I got a lot of issues and I don't have any clue of why:
- default placeholder style is never used even if not overwritten by the pseudo-element ;
- placeholder text is never hidden ;
(I would say the classes are not applied)
- sometimes the placeholder text forgets the -moz-placeholder styling (when I unfocus) and it comes back when the mouse is hover ;
- this assertion pops up:
###!!! ASSERTION: scrollbars should not have been created: 'result.mHorizontal != NS_STYLE_OVERFLOW_VISIBLE && result.mHorizontal != NS_STYLE_OVERFLOW_CLIP && result.mVertical != NS_STYLE_OVERFLOW_VISIBLE && result.mVertical != NS_STYLE_OVERFLOW_CLIP', file /home/volkmar/projects/mozilla/mozilla-central/src/layout/generic/nsGfxScrollFrame.cpp, line 1936

As that's my first CSS patch, I'm a bit lost in the code base so if someone has any hint...
Hmm.  The issue with the approach you took is that you're clobbering the "correct" style context (the one that has the rules from forms.css, etc) with the pseudo-element style context.

If we want to make this a pseudo-element and keep using the anonymous-content infrastructure for it (where there's actually a real element), then we need to sort out how we want the pseudo-element rules to interact with the rules applied to the content itself.  How do we want those to work?

The other option is to have anonymous content support a pseudo-element tag when resolving the style and then we can just move all the rules in forms.css into the pseudo-element block... except then the restriction rule will kill us.
(In reply to comment #15)
> Hmm.  The issue with the approach you took is that you're clobbering the
> "correct" style context (the one that has the rules from forms.css, etc) with
> the pseudo-element style context.
> 
> If we want to make this a pseudo-element and keep using the anonymous-content
> infrastructure for it (where there's actually a real element), then we need to
> sort out how we want the pseudo-element rules to interact with the rules
> applied to the content itself.  How do we want those to work?

If I get correctly your question, I would say, the pseudo-element rules should override the content rules because the pseudo-element rules are going to be restricted enough. But I'm not sure that's what you were asking.
Anyway, I've no idea how I can have both kind of rules used at the same time.

> The other option is to have anonymous content support a pseudo-element tag when
> resolving the style and then we can just move all the rules in forms.css into
> the pseudo-element block... except then the restriction rule will kill us.

Indeed, at the moment only 1 properties out of 6 will be accepted.
> Anyway, I've no idea how I can have both kind of rules used at the same time.

You could resolve style for the pseudo-element, then grab those rules and use them to modify the style for the anonymous content.  Not sure how to make the restriction rule thing work in that situation, though.
I don't know how I can get those rules. I think I can add rules to the style context with |ResolveStyleByAddingRules| but how do I find the rules to add ? I can recreate them like like |BuildStyleRule| from nsStyleAnimation [1] but I do not think that is the best idea.
In addition, |ResolveStyleByAddingRules| will not be really helpful to hide the placeholder because I will have to remove a rule.

Some help would be really appreciated !

[1]http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#928
Summary: Implement -moz-placeholder pseudo-class/element (for placeholder text styling on input fields) → Implement -moz-placeholder pseudo-element (for placeholder text styling)
> I don't know how I can get those rules.

Resolve a style context for the pseudo-element; then the relevant rules can be gotten off the style context's rulenode and its parents.

> In addition, |ResolveStyleByAddingRules| will not be really helpful to hide
> the placeholder because I will have to remove a rule.

Yeah, that's an issue.

> Some help would be really appreciated !

Basically, as long as we want to have both pseudo-element styles _and_ normal styles applying to the same node we're out of the "style system is designed to do this" field and into the woods.  Help would involve style system design changes (which means pinning down exactly what you want to do) or finding another way to do this.

Here's a question.  Does the pseudo-element style need to apply to the div in this case, or could it apply to a kid of the div?
(In reply to comment #19)
> > I don't know how I can get those rules.
> 
> Resolve a style context for the pseudo-element; then the relevant rules can be
> gotten off the style context's rulenode and its parents.

I can try that.

> > Some help would be really appreciated !
> 
> Basically, as long as we want to have both pseudo-element styles _and_ normal
> styles applying to the same node we're out of the "style system is designed to
> do this" field and into the woods.  Help would involve style system design
> changes (which means pinning down exactly what you want to do) or finding
> another way to do this.

I'm not looking to changes in the style system. Just to find a way to make that pseudo-element working.

> Here's a question.  Does the pseudo-element style need to apply to the div in
> this case, or could it apply to a kid of the div?

The placeholder div has a kid which is a text node. It would be possible to apply the pseudo-element styling to the text node only ?
Not without breaking more style system invariants, but it _would_ be possible to set up a DOM like:

  <div><span>text</span></div>

and apply the pseudo-element styling to the <span>...
If I do:
<div><span>text</span></div> or even <div><div>text</div></div>

Whatever is the style I try to apply to the span, it is not working. If I try to apply the pseudo element to the div and the content style to the span, I see the pseudo element style. The opposite shows the content style.

In addition, whit these changes, I got those asserts:
###!!! ASSERTION: pseudo type mismatch: 'aOldStyleContext->HasPseudoElementData() == aNewStyleContext->HasPseudoElementData()', file /home/volkmar/projects/mozilla/mozilla-central/src/layout/style/nsTransitionManager.cpp, line 401
###!!! ASSERTION: Some pres arena objects were not freed: 'mPresArenaAllocCount == 0', file /home/volkmar/projects/mozilla/mozilla-central/src/layout/base/nsPresShell.cpp, line 1533

If I do not try to set a style to the span, I do not get the asserts.

Another time, if you have an idea... ?
> Whatever is the style I try to apply to the span, it is not working.

Did you also reparent the style context for the textnode?

The assert is expected given the "set the style context later" approach and harmless for us (only affects CSS transitions); in practice we'd need to somehow flag the pseudo-element on the anonymous content we hand over to the frame constructor.
(In reply to comment #23)
> > Whatever is the style I try to apply to the span, it is not working.
> 
> Did you also reparent the style context for the textnode?

I don't know how I should do that. I tried to set the parent pointer by getting it from GetParent() but it didn't changed anything.
I'm probably doing it the wrong way.

I'm also trying the solution you mentioned in comment 19 because it is probably better but when the page is loaded I got the content style and when I put the cursor on the text field, the style is changing for the pseudo element one. So, I still can't get both style working together.
I don't know why the style is changing when the cursor is on the text field. I got this warning when the style is changing:
###!!! ASSERTION: scrollbars should not have been created: 'result.mHorizontal
!= NS_STYLE_OVERFLOW_VISIBLE && result.mHorizontal != NS_STYLE_OVERFLOW_CLIP &&
result.mVertical != NS_STYLE_OVERFLOW_VISIBLE && result.mVertical !=
NS_STYLE_OVERFLOW_CLIP', file
/home/volkmar/projects/mozilla/mozilla-central/src/layout/generic/nsGfxScrollFrame.cpp,
line 1936
It comes from a reflow but I don't know if the reflow is a cause or a consequence of the style change.

By the way, I'm changing the style like this (in nsTextControlFrame::SetInitialChildList):
  nsIFrame* phFrame = GetLastChild(nsnull);

  nsCSSPseudoElements::Type pseudoType = nsCSSPseudoElements::ePseudo_mozPlaceholder;
  nsRefPtr<nsStyleContext> newStyleContext;
  nsStyleContext* curStyleContext;

  newStyleContext = phFrame->PresContext()->PresShell()->StyleSet()->
    ResolvePseudoElementStyle(phFrame->GetContent(), pseudoType, phFrame->GetParent()->GetStyleContext());

  curStyleContext = phFrame->GetStyleContext();
  nsRuleNode* ruleNode = curStyleContext->GetRuleNode();
  nsCOMArray<nsIStyleRule> rules;

  for (;ruleNode;) {
    nsCOMPtr<nsICSSStyleRule> cssRule = do_QueryInterface(ruleNode->GetRule());
    if (cssRule) {
      nsAutoString cssText;
      cssRule->GetCssText(cssText);
      printf(" === Adding a rule: %s\n", NS_ConvertUTF16toUTF8(cssText).get());
    } else {
      printf(" === Adding non-css rule\n");
    }
    rules.AppendObject(ruleNode->GetRule());
    ruleNode = ruleNode->GetParent();
  }

  newStyleContext = phFrame->PresContext()->PresShell()->StyleSet()->
    ResolveStyleByAddingRules(newStyleContext, rules);

  if (newStyleContext) {
    phFrame->SetStyleContext(newStyleContext);
    newStyleContext->AddRef();
  }
blocking2.0: ? → beta1+
Priority: -- → P1
While I do think both a pseudo-class for indicating an input element in the state of having nothing but a placeholder, and a pseudo-element for selecting the placholder text/background itself is useful, I tend to agree with Tab's summary:

http://lists.w3.org/Archives/Public/www-style/2010Mar/0158.html

Thus I think a pseudo-element has more well defined use cases and thus if we are only implementing one thing for this, we should implement:

::-moz-placeholder 

using the Selectors convention of double-colons for pseudo-elements.

Which could be styled like

input::-moz-placeholder {color:#999; background-color:#fff}

However, Tab also makes the important comparison to the ::value pseudo-element.

[1] http://www.w3.org/TR/css3-ui/#pseudo-value

::value provides a more generally applicable solution across form elements (and use cases), and thus we could instead implement the already defined ::value pseudo-element *and* a new :-moz-placeholder pseudo-class.

For the use case of styling placeholder text then, that could styled as follows:

input:-moz-placeholder::value {color:#999; background-color:#fff}

This would also nicely address all the use cases raised on this thread so far.

Frankly I think this is better path forward because ::value will be useful on many more input/form elements than ::-moz-placeholder and I'd rather have the general ::value pseudo than have to create a bunch of new pseudo-elements for each input element type and attribute that we get from HTML5.
Tantek, do you think the -moz-placeholder pseudo-element should be limited to some properties like first-line ?
Yes absolutely. ::first-line is a good starting point for figuring out the small subset of properties that should apply to ::value (or ::-moz-placeholder).
Bumping this to beta2 per beta re-triage with dbaron.  If this should indeed block beta1, please re-nom.
blocking2.0: beta1+ → beta2+
I hope work continues and we get it for final, currently, textboxes with placeholder look rather ugly or even unreadable in some custom themes, possibly even some system themes that have different color schemes.
Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.
blocking2.0: beta2+ → betaN+
(In reply to comment #25)
> While I do think both a pseudo-class for indicating an input element in the
> state of having nothing but a placeholder, and a pseudo-element for selecting
> the placholder text/background itself is useful, I tend to agree with Tab's
> summary:
> 
> http://lists.w3.org/Archives/Public/www-style/2010Mar/0158.html
> 
> Thus I think a pseudo-element has more well defined use cases and thus if we
> are only implementing one thing for this, we should implement:
> 
> ::-moz-placeholder 
> 
> using the Selectors convention of double-colons for pseudo-elements.
> 
> Which could be styled like
> 
> input::-moz-placeholder {color:#999; background-color:#fff}
> 
> However, Tab also makes the important comparison to the ::value pseudo-element.
> 
> [1] http://www.w3.org/TR/css3-ui/#pseudo-value
> 
> ::value provides a more generally applicable solution across form elements (and
> use cases), and thus we could instead implement the already defined ::value
> pseudo-element *and* a new :-moz-placeholder pseudo-class.
> 
> For the use case of styling placeholder text then, that could styled as
> follows:
> 
> input:-moz-placeholder::value {color:#999; background-color:#fff}
> 
> This would also nicely address all the use cases raised on this thread so far.
> 
> Frankly I think this is better path forward because ::value will be useful on
> many more input/form elements than ::-moz-placeholder and I'd rather have the
> general ::value pseudo than have to create a bunch of new pseudo-elements for
> each input element type and attribute that we get from HTML5.

I misread your comment the first time. I thought you agreed with ::placeholder but actually, it sounds you are more behind :placeholder.
I have to comments regarding your suggestion:
- I think I like the idea of the pseudo-class actually because it would let the authors style the input when the placeholder is shown. Which could be used to have a (very) light gray background when the placeholder is shown ala Aero for example.
- Do you have more documentation about ::value? I can't really get the exact meaning with http://www.w3.org/TR/css3-ui/#pseudo-value . I mean, I don't really get why we would need that in addition of the pseudo-class?

Given that David also prefer the pseudo-class solution maybe we could do that? I think it could be quite easier to implement too.
Assignee: mounir.lamouri → nobody
Component: Layout: Form Controls → Style System (CSS)
QA Contact: layout.form-controls → style-system
Version: unspecified → Trunk
Yes, let's implement just the ::-moz-placeholder pseudo-class for these use cases, see how it works in practice and if there is any evidence of an additional need for a ::value pseudo-element or not.
Summary: Implement -moz-placeholder pseudo-element (for placeholder text styling) → Implement -moz-placeholder pseudo-class
Attached patch Patch v1 (obsolete) — Splinter Review
So, :-moz-placeholder is working if @placeholder is set (even if equals to the empty string), the element isn't focused and it's value is the empty string.
Assignee: nobody → mounir.lamouri
Attachment #437309 - Attachment is obsolete: true
Attachment #463588 - Flags: review?(bzbarsky)
Although I like the additional functionality that a pseudo-class permits, I'm not sure the naming is clear. The pseudo-class name should convey a sense of state. The only alternative I can think of right now is :-moz-placeholder-visible.

Having a different name would also help authors differentiate our pseudo-class with the WebKit pseudo-element.
This conflicts with the patch in bug 580575.  Furthermore, to merge them you have to change all the places that use event state to use an unsigned int....  Perhaps ideally even an nsEventState typedef which will be PRUint32 for now and become PRUint64 once we need more states?
Do we actually call Blur or Focus when the user clicks the control?
Attached patch Patch v2 (obsolete) — Splinter Review
Moving the review to dbaron given that Boris in on vacation this week.

For ::Blur and ::Focus issue mentioned by Boris in comment 36, I've just removed the code from there because it was not called when the focus/blur was coming from a click. Actually, IntrinsicState() is called multiple times when a focus is done. So, for the moment all tests pass without specific call to update content states. I don't think it would be easy to know when the element has been focused from nsHTMLInputElement but having that done from nsTextControlFrame should be easy. Let me know if you want me to add that David (or having tests succeeded is enough right now?).

About other Boris comment (comment 35), this bug will probably be landed before bug 580575 so I will probably do the merge in bug 580575. But in any case, I guess it's no big deal. For nsEventState type I agree but I would see that as a follow-up (of bug 580575 maybe?).
Attachment #463588 - Attachment is obsolete: true
Attachment #466417 - Flags: review?(dbaron)
Attachment #463588 - Flags: review?(bzbarsky)
(In reply to comment #37)
> For ::Blur and ::Focus issue mentioned by Boris in comment 36, I've just
> removed the code from there because it was not called when the focus/blur was
> coming from a click. Actually, IntrinsicState() is called multiple times when a
> focus is done. So, for the moment all tests pass without specific call to
> update content states. I don't think it would be easy to know when the element
> has been focused from nsHTMLInputElement but having that done from
> nsTextControlFrame should be easy. Let me know if you want me to add that David
> (or having tests succeeded is enough right now?).

I guess it can be easily done in ::PostHandleEVent too.
I'm a little confused trying to understand this patch:  I see calls to a PlaceholderApplies method, but I don't see that method in the tree or added in the patch.
A few other comments:

Maybe you should handle the ContentStatesChanged notification in SetValueChanged for both elements, both because:
 * it makes the code more consistent
 * it looks like you're missing a codepath (nsHTMLInputElement::Reset) on the input element

It seems like you also need to set ContentStateChanged notifications for the placeholder state when the control's focus changes.  (I'm not sure if there are situations currently where focus changes wouldn't cause restyling... but it still seems wrong, and could break if we optimize event state changes differently.)

I also need to figure out what the right thing to do when the placeholder attribute changes.

Finally, you'd need to send notifications when PlaceholderApplies() changes, whatever that does.  (Were you thinking of factoring out the HasAttr(None, placeholder) calls into a method?)
Yeah, I think you also just need to send ContentStatesChanged when the placeholder attribute changes.
Sounds like PlaceholderApplies is in bug 553097.

That means that in AfterSetAttr, you need to add the placeholder event state to the really long list of event states that can potentially change when the type of a control changes.
Depends on: 553097
It's possible I'm getting some of this wrong and that bz will correct me, but I don't see any magic handling of IntrinsicState changes across attribute changes.  I *think* we still need to send ContentStatesChanged notifications for when attribute changes cause changes in IntrinsicState.
Ah, ok, there is such code in nsGenericElement::SetAttrAndNotify as Mounir points out.  So you can ignore my comments about handling placeholder attribute changes.

That said, now I wonder why we have the big switch for handling of type changes in nsHTMLInputElement::AfterSetAttr.  Maybe it's not needed anymore.  But given that we have it, you should probably add this new state bit to the big constant; after all, it's just a constant.

But the issues with Reset and focus changes are still present.  (Though I wonder if my suggestion would break a case in which aSetValueChanged is false... I think it might.)
Attached patch Patch v2.1 (obsolete) — Splinter Review
I've added the requested ContentStatesChanged calls:
- value change in ::OnValueChanged
- focus/blur in ::PostHandleEvent
- type change in ::AfterSetAttr

And I've added tests for type change and reset() call. All of them were passing without the changes...
Attachment #466417 - Attachment is obsolete: true
Attachment #468205 - Flags: review?(dbaron)
Attached patch Patch v2.2 (obsolete) — Splinter Review
Damn, wrong file... :/
Attachment #468205 - Attachment is obsolete: true
Attachment #468206 - Flags: review?(dbaron)
Attachment #468205 - Flags: review?(dbaron)
And I've introduced PlaceholderApplies in this patch considering bug 553097 will not land for beta5 (and not have to) because it depends on a layout bug that no assignee yet. This removes the dependency.
No longer depends on: 553097
Blocks: 566348
Comment on attachment 468206 [details] [diff] [review]
Patch v2.2

The focus check in PostHandleEvent should check HasAttr in both cases and PlaceholderApplies in the input case.

(Should the OnValueChanged also check PlaceholderApplies, or does it only get called for text controls?)

r=dbaron with that
Attachment #468206 - Flags: review?(dbaron) → review+
(In reply to comment #48)
> Comment on attachment 468206 [details] [diff] [review]
> Patch v2.2
> 
> The focus check in PostHandleEvent should check HasAttr in both cases and
> PlaceholderApplies in the input case.
> 
> (Should the OnValueChanged also check PlaceholderApplies, or does it only get
> called for text controls?)

Gasp. I'm stupid! By the way, OnValueChanged is a TextControlState method but it's called by radio elements for some reasons so, I'm going to add PlaceholderApplies in that.

> r=dbaron with that

Thank you for the review :)
Keywords: dev-doc-needed
Attached patch Patch v2.3 (obsolete) — Splinter Review
r=dbaron
Attachment #468206 - Attachment is obsolete: true
Attachment #468247 - Flags: review+
The try server do not like this patch. Some logs:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282595957.1282596631.24315.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282597903.1282598764.1306.gz

There are two kinds of failures:
- XUL textboxes don't have the default placeholder style. It looks like the state is correctly set (in ::IntrinsicState) for the input/textarea inside the XUL textbox but for I don't know why, the style doesn't apply...
- Some of my reftests are failing but not on my computer and the images I got from the try server doesn't show an obvious problem... I can't get how this patch can lead to that. I will investigate tomorrow.
(In reply to comment #51)
> The try server do not like this patch. Some logs:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282595957.1282596631.24315.gz
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282597903.1282598764.1306.gz
> 
> There are two kinds of failures:
> - XUL textboxes don't have the default placeholder style. It looks like the
> state is correctly set (in ::IntrinsicState) for the input/textarea inside the
> XUL textbox but for I don't know why, the style doesn't apply...

Are you styling :-moz-placeholder on the input inside the textbox or on the textbox itself?

> - Some of my reftests are failing but not on my computer and the images I got
> from the try server doesn't show an obvious problem... I can't get how this
> patch can lead to that. I will investigate tomorrow.

layout/tools/reftest/reftest-analyzer.xhtml may help:  it can load a log or take a part of a log and highlight the location of the differences.  If you paste from a non-raw tinderbox log (i.e., one with "showlog.cgi?log=" not removed) you need to remove the NEXT ERROR stuff, though.

(Are the failures Win7 only?)
(In reply to comment #52)
> (In reply to comment #51)
> > The try server do not like this patch. Some logs:
> > http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282595957.1282596631.24315.gz
> > http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1282597903.1282598764.1306.gz
> > 
> > There are two kinds of failures:
> > - XUL textboxes don't have the default placeholder style. It looks like the
> > state is correctly set (in ::IntrinsicState) for the input/textarea inside the
> > XUL textbox but for I don't know why, the style doesn't apply...
> 
> Are you styling :-moz-placeholder on the input inside the textbox or on the
> textbox itself?

:-moz-placeholder is set in one place: layout/style/forms.css and it's something like: :-moz-any(input,textarea):-moz-placeholder { color: GrayText; }
Whitout this patch, the placeholder was styled directly using the anonymous div: input > .placeholder { color: GrayText; }
Now, an <input> or <textarea> will have a GrayText placeholder but a XUL textbox will have a dark one... I do not understand why XUL textbox are fine having the color set with input > .placeholder but not the pseudo-class.

> > - Some of my reftests are failing but not on my computer and the images I got
> > from the try server doesn't show an obvious problem... I can't get how this
> > patch can lead to that. I will investigate tomorrow.
> 
> layout/tools/reftest/reftest-analyzer.xhtml may help:  it can load a log or
> take a part of a log and highlight the location of the differences.  If you
> paste from a non-raw tinderbox log (i.e., one with "showlog.cgi?log=" not
> removed) you need to remove the NEXT ERROR stuff, though.

Ok, will look at that. Thanks :)

> (Are the failures Win7 only?)

No, all platforms but not reproducible on mine. I'm wondering if it's related to the focus ring actually. It's just an idea but I don't have one and I'm wondering if the Fedora used by the try server have one. I also say that because I've tested on a Mac and one of the difference between both images was the size of the focus ring. But I really don't see the link between this and my patch so.. :/
> :-moz-any(input,textarea):-moz-placeholder { color: GrayText; }

This is in the UA level.

Then there is a rule in the document level, coming from textbox.css, which explicitly sets the color of the textbox and another one which explicitly sets the color of the input inside it.  I expect this overrides your rule; you can test by making your rule !important.

I suggest just changing textbox.css to have the rules you want in it...  Sort of sad that we have to do that.  I wonder why those color styles are there, exactly.  Can we remove them?

The reason this used to work is that you were styling the anonymous div directly, so it no longer inherited the color from the input and the styling on the input no longer mattered.
(In reply to comment #53)
> :-moz-placeholder is set in one place: layout/style/forms.css and it's
> something like: :-moz-any(input,textarea):-moz-placeholder { color: GrayText; }

Also, please don't use :-moz-any() here; use input:-moz-placeholder, textarea:-moz-placeholder so that the rules end up in the tag table.  See https://developer.mozilla.org/en/CSS/Writing_Efficient_CSS .
(In reply to comment #54)
> Then there is a rule in the document level, coming from textbox.css

Actually, I wonder, why isn't a color like that always set just in the theme? I thought that all colors for XUL elements should actually belong in the theme, i.e. textbox.css in this case. I guess for HTML, forms.css is right, though, as we don't have theming for content, only for chrome.
Comment on attachment 468752 [details] [diff] [review]
Part 1 - Implement the pseudo-class

r=dbaron
Attachment #468752 - Attachment description: Part 1 - → Part 1 - Implement the pseudo-class
Attachment #468752 - Attachment is patch: true
Attachment #468752 - Attachment mime type: application/octet-stream → text/plain
Attachment #468247 - Attachment is obsolete: true
Attachment #468754 - Flags: review?(enndeakin)
Attachment #468754 - Attachment description: Fix XUL textbox default placeholder style → Part 2 - Fix XUL textbox default placeholder style
Oups there were a typo... Thanks to aja on IRC :)
Attachment #468754 - Attachment is obsolete: true
Attachment #468756 - Flags: review?(enndeakin)
Attachment #468754 - Flags: review?(enndeakin)
Attachment #468756 - Flags: review?(enndeakin) → review?(dao)
Comment on attachment 468756 [details] [diff] [review]
Part 2 (v1.1) - Fix XUL textbox default placeholder style

There's a bunch of content and theme CSS that needs uncommenting: http://mxr.mozilla.org/mozilla-central/search?string=moz-placeholder
(In reply to comment #61)
> Comment on attachment 468756 [details] [diff] [review]
> Part 2 (v1.1) - Fix XUL textbox default placeholder style
> 
> There's a bunch of content and theme CSS that needs uncommenting:
> http://mxr.mozilla.org/mozilla-central/search?string=moz-placeholder

We can have that done in a follow-up I guess.
Attachment #468756 - Flags: review?(dao) → review+
Looks like moz-appearance and background-color hates each other and for I don't know why, it was breaking my tests. The purpose of these tests isn't to check native widgets but the pseudo-class so I'm disabling that.
When -moz-appearance is a value that is supported by the native theme, CSS borders and backgrounds for the element are completely disabled. This is intentional and lets authors provide a fallback style for cases when that -moz-appearance value is not supported.
(In reply to comment #64)
> When -moz-appearance is a value that is supported by the native theme, CSS
> borders and backgrounds for the element are completely disabled. This is
> intentional and lets authors provide a fallback style for cases when that
> -moz-appearance value is not supported.

Yes, that is fine but if I have two input elements, one with green background and black color as soon as it loads and the other one has that set dynamically (per being subject to :-moz-placeholder pseudo-class), both of them will have the correct style but they will not look exactly the same. I wasn't able to found the reason and my only guess is there is a bug in the layout part when the style is changed. Or maybe this is done async and the reftests "capture" is done before the entire change. Whatever is the cause, because it doesn't sound related with :-moz-placeholder, I've chosen to just set -moz-appearance: none which is fixing my problem.

Roc, let me know if that sounds normal or not so I can open a bug if needed.
Depends on: 590418
Pushed:
http://hg.mozilla.org/mozilla-central/rev/19ada12a980f
http://hg.mozilla.org/mozilla-central/rev/007c3205aba2

Reftests issues in bug 590418.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Is this replacing the existing :-moz-placeholder pseudo-class? Or did that get renamed? Need to clarify that point before I write this up.
(In reply to comment #68)
> Is this replacing the existing :-moz-placeholder pseudo-class? Or did that get
> renamed? Need to clarify that point before I write this up.

I didn't see any trace of this pseudo-class in the current code base. No test and no obvious names in the code. I'm wondering if that is still used? Otherwise, the naming must be everything but logic.

Boris, do you know if that still exist?
There was no existing moz-placeholder pseudo-class.
(In reply to comment #70)
> There was no existing moz-placeholder pseudo-class.

Eh, then our dev doc was lying. Thank you for the quick answer :)

https://developer.mozilla.org/en/CSS/%3A-moz-placeholder
Uh, what the heck?  That and https://developer.mozilla.org/en/CSS/%3A-moz-alt-text are documenting the initial proposal from bug 11011, not what was actually implemented.
Now documented here:

https://developer.mozilla.org/en/CSS/%3a-moz-placeholder

(Replacing the totally bogus doc that was there before)

And listed on Firefox 4 for developers.
Flags: in-testsuite? → in-testsuite+
Depends on: 609162
Depends on: 657861
You need to log in before you can comment on or make changes to this bug.