737786 - Switch from :-moz-placeholder to ::-moz-placeholder (pseudo-class to pseudo-element)

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[Mounir Lamouri (:mounir)]

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.

Comment 4

[Frank Yan (:fryn)]

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.

Comment 5

[Frank Yan (:fryn)]

(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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Mounir Lamouri (:mounir)]

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.

Comment 8

[Mounir Lamouri (:mounir)]

Raphael might work on that if he finds time.

Comment 9

[Frank Yan (:fryn)]

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

Comment 10

[Simon Oberhammer]

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.

Comment 11

[Mounir Lamouri (:mounir)]

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

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Show/hide placeholder using display list instead of class

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Mounir Lamouri (:mounir)]

(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.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 2a - Remove :-moz-placeholder (pseudo-class)

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Part 2b - Create ::-moz-placeholder (pseudo-element)

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 2c - Switch all :-moz-placeholder to ::-moz-placeholder (and pass tests)

Comment 18

[Mounir Lamouri (:mounir)]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[Mounir Lamouri (:mounir)]

(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...

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 26

[Mounir Lamouri (:mounir)]

(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).

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Part 2d - Fix tests in layout/reftests/css-placeholder/

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...

Comment 29

[Mounir Lamouri (:mounir)]

Attachment: Part 3a - Hook up to a restriction model

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

Comment 30

[Mounir Lamouri (:mounir)]

Attachment: Part 3b - Restrictions set for ::-moz-placeholder

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...

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 32

[Mounir Lamouri (:mounir)]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Mounir Lamouri (:mounir)]

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.

Comment 36

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Tests

Comment 37

[Mounir Lamouri (:mounir)]

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...

Comment 38

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Add pointer-events.

... and improves the test.

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 43

[Boris Zbarsky [:bzbarsky]]

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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 45

[Mounir Lamouri (:mounir)]

(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 :)

Comment 46

[Boris Zbarsky [:bzbarsky]]

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

Why not?

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

Comment 47

[Mounir Lamouri (:mounir)]

Attachment: Interdiff for part 3b

Comment 48

[Mounir Lamouri (:mounir)]

Attachment: Part 3b - Restrictions set for ::-moz-placeholder

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

Comment 49

[Boris Zbarsky [:bzbarsky]]

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?

Comment 50

[Boris Zbarsky [:bzbarsky]]

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

Comment 51

[Boris Zbarsky [:bzbarsky]]

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

r- pending those bits being hashed out.

Comment 52

[Mounir Lamouri (:mounir)]

Attachment: Part 3b - Restrictions set for ::-moz-placeholder

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

Comment 53

[Boris Zbarsky [:bzbarsky]]

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

Comment 54

[Mounir Lamouri (:mounir)]

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

Comment 55

[Boris Zbarsky [:bzbarsky]]

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...

Comment 56

[Boris Zbarsky [:bzbarsky]]

I meant "silently show the wrong thing".

Comment 57

[Mounir Lamouri (:mounir)]

It seems pretty much correct.

Comment 58

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 59

[Mounir Lamouri (:mounir)]

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

Comment 60

[(no longer active)]

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

Comment 61

[Dave Hylands [:dhylands]]

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)

Comment 62

[Frank Yan (:fryn)]

(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

Comment 63

[Dave Hylands [:dhylands]]

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

Comment 64

[Boris Zbarsky [:bzbarsky]]

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

Mounir, please fix?

Comment 65

[Frank Yan (:fryn)]

(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.

Comment 66

[Boris Zbarsky [:bzbarsky]]

No, you actually couldn't...

Comment 67

[Mounir Lamouri (:mounir)]

(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?

Comment 68

[Boris Zbarsky [:bzbarsky]]

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?

Comment 69

[Mounir Lamouri (:mounir)]

(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.

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 71

[dickvl]

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

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.