Last Comment Bug 457801 - Implement -moz-placeholder pseudo-class
: Implement -moz-placeholder pseudo-class
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 enhancement with 4 votes (vote)
: mozilla2.0b5
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://phroggy.com/placeholder.html
Depends on: 590418 609162 657861
Blocks: 457800 547224 551545 566348
  Show dependency treegraph
 
Reported: 2008-09-29 19:34 PDT by Andy Lyttle
Modified: 2013-04-23 09:32 PDT (History)
29 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Patch - WIP (7.47 KB, patch)
2010-04-06 08:59 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (29.85 KB, patch)
2010-08-06 10:23 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (27.52 KB, patch)
2010-08-16 13:50 PDT, Mounir Lamouri (:mounir)
dbaron: review-
Details | Diff | Splinter Review
Patch v2.1 (9.27 KB, patch)
2010-08-22 19:37 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.2 (33.60 KB, patch)
2010-08-22 19:38 PDT, Mounir Lamouri (:mounir)
dbaron: review+
Details | Diff | Splinter Review
Patch v2.3 (34.44 KB, patch)
2010-08-22 23:43 PDT, Mounir Lamouri (:mounir)
mounir: review+
Details | Diff | Splinter Review
Part 1 - Implement the pseudo-class (34.45 KB, patch)
2010-08-24 11:57 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Fix XUL textbox default placeholder style (2.96 KB, patch)
2010-08-24 12:01 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 (v1.1) - Fix XUL textbox default placeholder style (2.96 KB, patch)
2010-08-24 12:18 PDT, Mounir Lamouri (:mounir)
dao+bmo: review+
Details | Diff | Splinter Review
Fix :-moz-placeholder reftests. (1.33 KB, patch)
2010-08-24 16:58 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Andy Lyttle 2008-09-29 19:34:01 PDT
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
Comment 1 Boris Zbarsky [:bz] 2009-02-17 08:41:13 PST
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.
Comment 2 Mounir Lamouri (:mounir) 2010-02-12 09:03:33 PST
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.
Comment 3 d 2010-02-19 07:31:48 PST
I guess we would keep the same syntax as the one implemented in Webkit now?
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-02-26 11:40:33 PST
Nominating for blocking1.9.3, since given that we've added support for placeholder, we should probably have a mechanism for styling it.
Comment 5 Dão Gottwald [:dao] 2010-02-27 05:26:32 PST
Why would this be a pseudo element rather than a pseudo class?
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-02-27 10:17:09 PST
There was a long thread on pseudo-element vs. pseudo-class here:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-February/thread.html#25190

I summarized my opinions in http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-February/025209.html
Comment 7 Mounir Lamouri (:mounir) 2010-02-27 11:00:10 PST
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 ?
Comment 8 d 2010-03-03 10:34:38 PST
Looks like we already have a psuedo-class called ":-moz-placeholder" (see bug 11011).
Comment 9 Boris Zbarsky [:bz] 2010-03-03 10:40:08 PST
We can rename that if needed; it's purely internal.
Comment 10 Boris Zbarsky [:bz] 2010-03-03 10:41:00 PST
I don't have a good answer to comment 7, really.  David's opinion is linked to from comment 6.
Comment 11 Robert Kaiser 2010-03-08 04:44:01 PST
(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.
Comment 12 Mounir Lamouri (:mounir) 2010-03-08 06:57:07 PST
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.
Comment 13 Alfred Kayser 2010-03-30 02:05:33 PDT
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.
Comment 14 Mounir Lamouri (:mounir) 2010-04-06 08:59:26 PDT
Created attachment 437309 [details] [diff] [review]
Patch - WIP

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...
Comment 15 Boris Zbarsky [:bz] 2010-04-06 09:04:51 PDT
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.
Comment 16 Mounir Lamouri (:mounir) 2010-04-06 10:11:06 PDT
(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.
Comment 17 Boris Zbarsky [:bz] 2010-04-06 10:20:58 PDT
> 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.
Comment 18 Mounir Lamouri (:mounir) 2010-04-07 05:29:37 PDT
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
Comment 19 Boris Zbarsky [:bz] 2010-04-07 05:37:10 PDT
> 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?
Comment 20 Mounir Lamouri (:mounir) 2010-04-07 05:53:57 PDT
(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 ?
Comment 21 Boris Zbarsky [:bz] 2010-04-07 06:35:55 PDT
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>...
Comment 22 Mounir Lamouri (:mounir) 2010-04-08 17:03:09 PDT
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... ?
Comment 23 Boris Zbarsky [:bz] 2010-04-08 19:16:11 PDT
> 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.
Comment 24 Mounir Lamouri (:mounir) 2010-04-12 07:47:20 PDT
(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();
  }
Comment 25 Tantek Çelik 2010-06-03 11:12:30 PDT
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.
Comment 26 Mounir Lamouri (:mounir) 2010-06-03 11:16:09 PDT
Tantek, do you think the -moz-placeholder pseudo-element should be limited to some properties like first-line ?
Comment 27 Tantek Çelik 2010-06-03 11:48:20 PDT
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).
Comment 28 Damon Sicore (:damons) 2010-06-18 14:24:09 PDT
Bumping this to beta2 per beta re-triage with dbaron.  If this should indeed block beta1, please re-nom.
Comment 29 Robert Kaiser 2010-06-18 16:05:43 PDT
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.
Comment 30 Mike Beltzner [:beltzner, not reading bugmail] 2010-07-19 16:05:58 PDT
Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.
Comment 31 Mounir Lamouri (:mounir) 2010-08-05 04:31:48 PDT
(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.
Comment 32 Tantek Çelik 2010-08-05 16:44:33 PDT
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.
Comment 33 Mounir Lamouri (:mounir) 2010-08-06 10:23:03 PDT
Created attachment 463588 [details] [diff] [review]
Patch v1

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.
Comment 34 Anthony Ricaud (:rik) 2010-08-06 14:01:58 PDT
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.
Comment 35 Boris Zbarsky [:bz] 2010-08-12 00:03:46 PDT
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?
Comment 36 Boris Zbarsky [:bz] 2010-08-12 00:06:16 PDT
Do we actually call Blur or Focus when the user clicks the control?
Comment 37 Mounir Lamouri (:mounir) 2010-08-16 13:50:59 PDT
Created attachment 466417 [details] [diff] [review]
Patch v2

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?).
Comment 38 Mounir Lamouri (:mounir) 2010-08-17 13:06:30 PDT
(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.
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 22:55:02 PDT
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.
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 23:03:50 PDT
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?)
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 23:10:46 PDT
Yeah, I think you also just need to send ContentStatesChanged when the placeholder attribute changes.
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 23:41:45 PDT
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.
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 23:47:47 PDT
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.
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-21 23:53:23 PDT
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.)
Comment 45 Mounir Lamouri (:mounir) 2010-08-22 19:37:35 PDT
Created attachment 468205 [details] [diff] [review]
Patch v2.1

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...
Comment 46 Mounir Lamouri (:mounir) 2010-08-22 19:38:21 PDT
Created attachment 468206 [details] [diff] [review]
Patch v2.2

Damn, wrong file... :/
Comment 47 Mounir Lamouri (:mounir) 2010-08-22 19:40:37 PDT
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.
Comment 48 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-22 23:17:37 PDT
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
Comment 49 Mounir Lamouri (:mounir) 2010-08-22 23:31:25 PDT
(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 :)
Comment 50 Mounir Lamouri (:mounir) 2010-08-22 23:43:37 PDT
Created attachment 468247 [details] [diff] [review]
Patch v2.3

r=dbaron
Comment 51 Mounir Lamouri (:mounir) 2010-08-23 23:23:12 PDT
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.
Comment 52 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-23 23:29:07 PDT
(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?)
Comment 53 Mounir Lamouri (:mounir) 2010-08-23 23:39:02 PDT
(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.. :/
Comment 54 Boris Zbarsky [:bz] 2010-08-23 23:59:22 PDT
> :-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.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-24 00:22:47 PDT
(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 .
Comment 56 Robert Kaiser 2010-08-24 06:25:58 PDT
(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 57 Mounir Lamouri (:mounir) 2010-08-24 11:57:53 PDT
Created attachment 468752 [details] [diff] [review]
Part 1 - Implement the pseudo-class
Comment 58 Mounir Lamouri (:mounir) 2010-08-24 11:58:33 PDT
Comment on attachment 468752 [details] [diff] [review]
Part 1 - Implement the pseudo-class

r=dbaron
Comment 59 Mounir Lamouri (:mounir) 2010-08-24 12:01:34 PDT
Created attachment 468754 [details] [diff] [review]
Part 2 - Fix XUL textbox default placeholder style
Comment 60 Mounir Lamouri (:mounir) 2010-08-24 12:18:37 PDT
Created attachment 468756 [details] [diff] [review]
Part 2 (v1.1) - Fix XUL textbox default placeholder style

Oups there were a typo... Thanks to aja on IRC :)
Comment 61 Dão Gottwald [:dao] 2010-08-24 13:47:17 PDT
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
Comment 62 Mounir Lamouri (:mounir) 2010-08-24 13:48:36 PDT
(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.
Comment 63 Mounir Lamouri (:mounir) 2010-08-24 16:58:53 PDT
Created attachment 468862 [details] [diff] [review]
Fix :-moz-placeholder reftests.

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.
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-24 20:19:57 PDT
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.
Comment 65 Mounir Lamouri (:mounir) 2010-08-24 21:19:28 PDT
(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.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-24 21:44:04 PDT
Sounds like a bug.
Comment 68 Eric Shepherd [:sheppy] 2010-08-27 11:09:42 PDT
Is this replacing the existing :-moz-placeholder pseudo-class? Or did that get renamed? Need to clarify that point before I write this up.
Comment 69 Mounir Lamouri (:mounir) 2010-08-27 11:18:00 PDT
(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?
Comment 70 Boris Zbarsky [:bz] 2010-08-27 11:21:46 PDT
There was no existing moz-placeholder pseudo-class.
Comment 71 Mounir Lamouri (:mounir) 2010-08-27 11:23:32 PDT
(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
Comment 72 Boris Zbarsky [:bz] 2010-08-27 11:33:44 PDT
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.
Comment 73 Eric Shepherd [:sheppy] 2010-08-27 12:15:36 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.