Mounir Lamouri (:mounir) Assignee
	Description • 12 years ago

That should fix a few issues we currently have with the placeholder, hopefully. We would also be able to standardize that because Webkit is using a pseudo-element.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 12 years ago

So there are a few pieces to this.

First of all the input code will need to create a style context for the placeholder.  This should not be too bad.

We probably need to restrict the set of properties that apply to the placeholder.  The set of things that apply to first-line might be a reasonable set of restrictions, right?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 2 • 12 years ago

I don't follow why we want to switch.  Could you explain?

	Mounir Lamouri (:mounir) Assignee
	Comment 3 • 12 years ago

There was some work on bug 673873 and we realized that using a pseudo-class will create unwanted behavior (look at bug 673873 comment 19 and below). A way to fix that would be to use a pseudo-element. That would also solve the issue in bug 737273. That would also allow us reducing the opacity of the placeholder text instead of setting the color to GrayText. That would improve a11y, see bug 556145, and will also help web authors to have a correct placeholder color even if they change the input text color.

It seems that using a pseudo-element would improve a few stuff while using a pseudo-class does help us.

	Frank Yan (:fryn)
	Comment 4 • 12 years ago

Marking this as blocking bug 673873, since this blocks having a proper fix for that bug.

I might give this bug a try when I have time.

	Frank Yan (:fryn)
	Comment 5 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #1)
> So there are a few pieces to this.
> 
> First of all the input code will need to create a style context for the
> placeholder.  This should not be too bad.
> 
> We probably need to restrict the set of properties that apply to the
> placeholder.  The set of things that apply to first-line might be a
> reasonable set of restrictions, right?

I'll give this a shot, but I'll need some hand-holding.
bz, could you point to the files needed for each piece and, if one exists, an already fixed bug that required similar work, e.g. implementing a pseudo-element or converting a pseudo-class to a pseudo-element?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

The file for the first piece is nsTextControlFrame.cpp.

For the restriction stuff, you probably want to look at how first-line and first-letter are handled in nsStyleSet.cpp.

I don't think we have any past pseudo-class to pseudo-element conversions.  As far as implementing, they're all pretty idiosyncratic; you want to model on first-line and first-letter for the style system end, as I said.

	Mounir Lamouri (:mounir) Assignee
	Comment 7 • 12 years ago

Frank, FYI, there is some work happening in bug 716875 and those two bugs might conflict. It's likely no big deal because rebasing for both might not be that hard.

	Mounir Lamouri (:mounir) Assignee
	Comment 8 • 12 years ago

Raphael might work on that if he finds time.

	Frank Yan (:fryn)
	Comment 9 • 12 years ago

If Raphael finds time, he can take over the bug.
This will at most be a side-project for me.

	Simon Oberhammer
	Comment 10 • 12 years ago

As a pseudo-class, placeholder makes it harder to write autoheight JavaScript for textareas; assuming :-moz-placeholder is styled to have different font-size. As a pseudo-element, all existing autoheight code would continue to Just Work.

	Mounir Lamouri (:mounir) Assignee
	Comment 11 • 12 years ago

I've been working on that this evening. Got something working. I will attach patches for review later (likely tonight).

	Mounir Lamouri (:mounir) Assignee
	Comment 12 • 12 years ago

We will not be able to use the current system to show/hide the placeholder frame. This patch doesn't draw the frame when the placeholder should be hidden instead of setting a 'visibility: hidden;' rule via a CSS class.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 12 years ago

Comment on attachment 670580 [details] [diff] [review]
Part 1 - Show/hide placeholder using display list instead of class

You won't need to set the class attr once you've moved to pseudo-element here, for what it's worth.  Of course you'll need to move the rules we have now into the ::-moz-placeholder styles.

>+   * The implementation of this method is equivalent as:

  The implementation of this method is the same as

r=me

	Mounir Lamouri (:mounir) Assignee
	Comment 14 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #13)
> Comment on attachment 670580 [details] [diff] [review]
> Part 1 - Show/hide placeholder using display list instead of class
> 
> You won't need to set the class attr once you've moved to pseudo-element
> here, for what it's worth.  Of course you'll need to move the rules we have
> now into the ::-moz-placeholder styles.

Yes. Next patches do that. I just wanted to have this patch standalone.

	Mounir Lamouri (:mounir) Assignee
	Comment 18 • 12 years ago

I removed some rules from forms.css (overflow: hidden and pointer-events: none) because they didn't seem useful. I understand why the later is no longer useful. I am not sure if the former was needed before. It doesn't seem needed anymore.

I will add other patches (part 3) to restrict the rules that can apply to the pseudo-element.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 12 years ago

Comment on attachment 670761 [details] [diff] [review]
Part 2a - Remove :-moz-placeholder (pseudo-class)

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 12 years ago

Comment on attachment 670763 [details] [diff] [review]
Part 2b - Create ::-moz-placeholder (pseudo-element)

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 12 years ago

But note that this does not seem to implement any sort of styling restriction on placeholders.  That seems odd to me.  What happens when a page styles it as display:inline, say?

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 12 years ago

Comment on attachment 670764 [details] [diff] [review]
Part 2c - Switch all :-moz-placeholder to ::-moz-placeholder (and pass tests)

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 12 years ago

The overflow:hidden was there because of bug 457800 comment 6 item 1.  It's possible that not using nsStackFrame for the text control fixed that.  But please do test, if we don't have a testcase for it already.

That said, what happens if someone gives a placeholder with long text some padding?

	Mounir Lamouri (:mounir) Assignee
	Comment 24 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #23)
> The overflow:hidden was there because of bug 457800 comment 6 item 1.  It's
> possible that not using nsStackFrame for the text control fixed that.  But
> please do test, if we don't have a testcase for it already.

placeholder-6.html is testing that.

> That said, what happens if someone gives a placeholder with long text some
> padding?

Good catch. Putting enough padding put the placeholder outside of the text field. However, "overflow: hidden;" doesn't fix it...

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 12 years ago

> Putting enough padding put the placeholder outside of the text field.

Hmm....  I was more worried about what would happen with the text overflowing the placeholder.  Is the placeholder block sticking out of the textfield, or is the text sticking out of the block?

	Mounir Lamouri (:mounir) Assignee
	Comment 26 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #25)
> > Putting enough padding put the placeholder outside of the text field.
> 
> Hmm....  I was more worried about what would happen with the text
> overflowing the placeholder.  Is the placeholder block sticking out of the
> textfield, or is the text sticking out of the block?

The placeholder block is translated outside of the text block. I thought putting "overflow: hidden;" on input would fix that but no. IMO, it's no big deal. Actually, I even wonder if that's a bug; I think <progress> behaves the same way (padding for the bar will move it outside of the progress block).
Weirdly Chrome has the same behaviour for input but not textarea (it's hidden for textarea).

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 12 years ago

> The placeholder block is translated outside of the text block.

Hmm.  I guess it wasn't an issue with the pseudo-class because that div could not be styled directly...

> IMO, it's no big deal.

It might be in terms of writing a spec for this.  The spec would either need to require this behavior or disallow it.  Which should it do?

	Mounir Lamouri (:mounir) Assignee
	Comment 28 • 12 years ago

I forgot those tests and fixing them wasn't easy because a pseudo-class and a pseudo-element are very different and there are a ton of stuff we can't actually test easily with this pseudo-element.
Changing the background of ::-moz-placeholder isn't similar as changing the one of <input>. Same goes for -moz-appearance, border, font-style, ... Actually that last one was tricky: if font-style is changed on <input> we change the size of the element to match the new width of the font but if we do that for ::-moz-placeholder, we obviously don't do that. This is a good behaviour but it took me some time to understand why input.ref, input::-moz-placeholder { font-style: italic; } was not outputing the same thing...

	Mounir Lamouri (:mounir) Assignee
	Comment 29 • 12 years ago

This is just to move the boilerplate out of the real patch.

	Mounir Lamouri (:mounir) Assignee
	Comment 30 • 12 years ago

I chose to use the same restrictions than first-letter and allowed a few more properties like:
- opacity,
- display
- resize,
- overflow,
- white-space.

Except 'opacity', all those rules are allowed mostly because the UA stylesheet need them...

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 12 years ago

> Except 'opacity', all those rules are allowed mostly because the UA stylesheet need
> them...

Then you need to make sure the UA sheet always sets them, and does so with !important, right?  Otherwise pages will be able to change those.

	Mounir Lamouri (:mounir) Assignee
	Comment 32 • 12 years ago

I did that for display and resize. Seems not worth it for overflow and white-space. I can do that if you think it would be better.

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 12 years ago

Can we deal with the reframes that changing overflow and white-space triggers?

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 12 years ago

(Note: I'm 99% sure the answer is "no, they assert and then bad things happen".)

	Mounir Lamouri (:mounir) Assignee
	Comment 35 • 12 years ago

Not sure what you mean by that. Setting statically or dynamically "overflow: scroll; white-space: pre;" on a textarea or an input just work as expected without assertions.

Again, I can mark those !important if you feel more comfortable with that.

	Mounir Lamouri (:mounir) Assignee
	Comment 37 • 12 years ago

There is a bug I saw while working on that and might require a follow-up: on textareas, it is no longer possible to resize the element while the placeholder is showing... I wonder if pointer-events: none could fix that actually...

	Mounir Lamouri (:mounir) Assignee
	Comment 38 • 12 years ago

... and improves the test.

This is fixing the resize handler not being usable when the placeholder is showing.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 12 years ago

> Not sure what you mean by that.

If a web page dynamically restyles the _placeholder_, not the input, to change its overflow or white-space style, what happens?

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 12 years ago

Comment on attachment 671803 [details] [diff] [review]
Part 2d - Fix tests in layout/reftests/css-placeholder/

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 12 years ago

Comment on attachment 671863 [details] [diff] [review]
Part 3a - Hook up to a restriction model

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 12 years ago

Comment on attachment 671866 [details] [diff] [review]
Part 3b - Restrictions set for ::-moz-placeholder

Shouldn't "resize: none !important" be set on input::-moz-placeholder too?

Also, don't we want to either exclude "float" or force "float: none"?  I don't understand how we can possibly handle "float: right" on a placeholder sanely.

In general, I think it would make more sense to model placeholder on first-line instead of first-letter.  That would prevent styling of the border and such, but would also simplify the layout issues that arise when the page does that...

Again, all this needs to be raised as spec issues.

Please document on things like "display" in the .h file why you're making them work on placeholder.

Please do an interdiff for this one for the review comments, because I don't want to read that whole header again.  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 12 years ago

Comment on attachment 671888 [details] [diff] [review]
Part 4 - Tests

The "left" in css-restrictions.html makes no sense.  It would be a no-op even if not restricted, no?

r=me with that fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 12 years ago

Comment on attachment 671891 [details] [diff] [review]
Part 5 - Add pointer-events.

r=me

	Mounir Lamouri (:mounir) Assignee
	Comment 45 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #42)
> Comment on attachment 671866 [details] [diff] [review]
> Part 3b - Restrictions set for ::-moz-placeholder
> 
> Shouldn't "resize: none !important" be set on input::-moz-placeholder too?

resize: both; on input::-moz-placeholder has no effect.

> Also, don't we want to either exclude "float" or force "float: none"?  I
> don't understand how we can possibly handle "float: right" on a placeholder
> sanely.

Ok. Will exclude this.

> In general, I think it would make more sense to model placeholder on
> first-line instead of first-letter.  That would prevent styling of the
> border and such, but would also simplify the layout issues that arise when
> the page does that...

Will have a look.

> Again, all this needs to be raised as spec issues.

This is planned.

> Please document on things like "display" in the .h file why you're making
> them work on placeholder.
> 
> Please do an interdiff for this one for the review comments, because I don't
> want to read that whole header again.  ;)

Sure things :)

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 12 years ago

> resize: both; on input::-moz-placeholder has no effect.

Why not?

In any case, I would prefer we played it safe here.

	Mounir Lamouri (:mounir) Assignee
	Comment 48 • 12 years ago

See interdiff.

I've added comments in the header for 'display' and 'resize' properties.
I've restricted all border properties from ::-moz-placeholder which basically makes the pseudo-element behaves like first line with the following differences:
- allows margin and padding,
- allows box-shadow,
- allows resize and display for the given reasons,
- allows opacity, overflow and white-space.

I see that this patch will have build issues. I have to run right now but consider them fixed ;)

	Boris Zbarsky [:bzbarsky]
	Comment 49 • 12 years ago

Comment on attachment 672784 [details] [diff] [review]
Part 3b - Restrictions set for ::-moz-placeholder

Don't you also need to set white-space and overflow with !important in the UA sheet?

	Boris Zbarsky [:bzbarsky]
	Comment 50 • 12 years ago

And also, what's the point of disallowing border but allowing margin and padding?

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 12 years ago

Comment on attachment 672784 [details] [diff] [review]
Part 3b - Restrictions set for ::-moz-placeholder

r- pending those bits being hashed out.

	Mounir Lamouri (:mounir) Assignee
	Comment 52 • 12 years ago

Sorry, I lost the interdiff... I could generate it if you want to.

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 12 years ago

If it's easy to generate one from the last thing I reviewed, that would be great.  If not, don't worry about it.

	Mounir Lamouri (:mounir) Assignee
	Comment 54 • 12 years ago

What about: https://bugzilla.mozilla.org/attachment.cgi?oldid=671866&action=interdiff&newid=675572&headers=1

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 12 years ago

Does it give the right output?  In my experience, bugzilla interdiff will silently show the right thing a good fraction of the time, so I never trust it...

	Boris Zbarsky [:bzbarsky]
	Comment 56 • 12 years ago

I meant "silently show the wrong thing".

	Mounir Lamouri (:mounir) Assignee
	Comment 57 • 12 years ago

It seems pretty much correct.

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 12 years ago

Comment on attachment 675572 [details] [diff] [review]
Part 3b - Restrictions set for ::-moz-placeholder

r=me

	Mounir Lamouri (:mounir) Assignee
	Comment 59 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c3d66e557a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9ba4944378
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f28fd24bdf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/612d29c19d3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5dffbc5a154

	(no longer active)
	Comment 60 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/b3c3d66e557a
https://hg.mozilla.org/mozilla-central/rev/6b9ba4944378
https://hg.mozilla.org/mozilla-central/rev/0f28fd24bdf3
https://hg.mozilla.org/mozilla-central/rev/612d29c19d3c
https://hg.mozilla.org/mozilla-central/rev/b5dffbc5a154

	Dave Hylands [:dhylands]
	Comment 61 • 12 years ago

I get the following in my logcat:
[JavaScript Warning: "Found trailing token after pseudo-element, which must be the last part of a selector:  '['.  Ruleset ignored due to bad selector." {file: "resource://gre-resources/forms.css" line: 90}]

And my homescreen doesn't load on b2g (not sure if its due to that or not)

	Frank Yan (:fryn)
	Comment 62 • 12 years ago

(In reply to Dave Hylands [:dhylands] from comment #61)
> I get the following in my logcat:
> [JavaScript Warning: "Found trailing token after pseudo-element, which must
> be the last part of a selector:  '['.  Ruleset ignored due to bad selector."
> {file: "resource://gre-resources/forms.css" line: 90}]

It seems that this is the problem:
https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#89

	Dave Hylands [:dhylands]
	Comment 63 • 12 years ago

And this problem isn't related to my homescreen not showing up (I've managed to isolate this to something in gaia).

	Boris Zbarsky [:bzbarsky]
	Comment 64 • 12 years ago

Er, yes.  That bit there is not valid CSS...

Mounir, please fix?

	Frank Yan (:fryn)
	Comment 65 • 12 years ago

(In reply to Frank Yan (:fryn) from comment #62)
> (In reply to Dave Hylands [:dhylands] from comment #61)
> > I get the following in my logcat:
> > [JavaScript Warning: "Found trailing token after pseudo-element, which must
> > be the last part of a selector:  '['.  Ruleset ignored due to bad selector."
> > {file: "resource://gre-resources/forms.css" line: 90}]
> 
> It seems that this is the problem:
> https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#89

(In reply to Boris Zbarsky (:bz) from comment #64)
> Er, yes.  That bit there is not valid CSS...

I suppose we could attach a class to the placeholder div, like we do to the editor div (.anonymous-div), and use that.

	Boris Zbarsky [:bzbarsky]
	Comment 66 • 12 years ago

No, you actually couldn't...

	Mounir Lamouri (:mounir) Assignee
	Comment 67 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #64)
> Er, yes.  That bit there is not valid CSS...
> 
> Mounir, please fix?

The only solution I see is to remove the line. Where you thinking of something else Boris?

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 12 years ago

I didn't have any bright ideas; otherwise I would have suggested them.  :(

Do we need that style, or can we get away without it?

	Mounir Lamouri (:mounir) Assignee
	Comment 69 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #68)
> I didn't have any bright ideas; otherwise I would have suggested them.  :(
> 
> Do we need that style, or can we get away without it?

I opened a follow-up (bug 810796) where I explain what I believe is the behaviour intended by the rule.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 70 • 11 years ago

The working group resolved to have both a pseudo-class and a pseudo-element, :placeholder-shown and ::placeholder : http://krijnhoetmer.nl/irc-logs/css/20130206#l-654

	dickvl
	Comment 71 • 11 years ago

Can someone update the pages on MDN to reflect this change?

https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Mozilla_Extensions

"View Live Example" links to old input:-moz-placeholder example:
https://developer.mozilla.org/en-US/docs/Web/CSS/::-moz-placeholder
https://developer.mozilla.org/samples/cssref/moz-placeholder.html

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 72 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #70)
> The working group resolved to have both a pseudo-class and a pseudo-element,
> :placeholder-shown and ::placeholder :
> http://krijnhoetmer.nl/irc-logs/css/20130206#l-654

I filed bug 1069012 and bug 1069015 for this.