Closed Bug 1069015 Opened 10 years ago Closed 8 years ago

restore code for :placeholder-shown pseudo-class

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dbaron, Assigned: wisniewskit)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-chrome][parity-safari][parity-webkit][parity-blink])

Attachments

(1 file, 2 obsolete files)

In bug 737786 we removed code for a placeholder pseudo-class and added code for a pseudo-element.

However, the CSS WG resolved to have both, and the names were agreed by the CSSWG in http://krijnhoetmer.nl/irc-logs/css/20130206#l-654 .  Only the pseudo-class is in a spec http://dev.w3.org/csswg/selectors/#placeholder partly because there doesn't appear to be an active spec appropriate for the pseudo-element.
Also supported in Safari 9.0+.
[parity-chrome][parity-webkit] should be added to the Whiteboard field.
Whiteboard: [parity-chrome][parity-safari][parity-webkit][parity-blink]
Here's a patch which rolls back the changes from bug 737786 necessary to get :placeholder working again (as :-moz-placeholder-shown).

Note that I also removed "font: -moz-list" style from one of the tests for the pseudo-element, because it was failing due to that style, and I don't see why it was necessary in the first place. (The failure was that the text in the input was being aligned to the top of the input, rather than the middle).

A try run seems fine (just a few unrelated intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=31e10f380502
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8782644 - Flags: review?(dbaron)
Comment on attachment 8782644 [details] [diff] [review]
1069015-restore-code-for-placeholder-shown-pseudo-class.diff

So other implementations are shipping this unprefixed (right?), and I believe the CSS WG resolved that that was OK.  So we should do this unprefixed.  So could you remove the MOZ_ and -moz- prefixes.  (We really shouldn't have the MOZ_ prefixes internally at all.)  Could you update the patch to do that and then re-request review?

I didn't yet do a careful comparison to the removal in attachment 670761 [details] [diff] [review].  Were there any substantial differences?

I'm a little curious about the -moz-list test changes, though.  And I don't understand this addition:

>+input:-moz-placeholder {
>+  -moz-appearance: none;
>+  color: -moz-FieldText;
>+  background-color: green;
>+}

What's :-moz-placeholder with one colon?
Attachment #8782644 - Flags: review?(dbaron)
>So could you remove the MOZ_ and -moz- prefixes.

Oh definitely, I just thought we wanted to do that in the parent ticket for both this and the pseudo-element, all at once.

>Were there any substantial differences?

No, I picked only the changes from the patches in that set that added back the feature. The only significant changes I recall were adding the -shown suffix, and removing the :moz-list styles. I don't really see a reason for them to be there, unless maybe there are rendering differences without them on a platform that I'm not as familiar with.

>What's :-moz-placeholder with one colon?

Good catch, that's just a copy-paste error (I forgot to add the -shown suffix). Makes me wonder how that test passed.. I'll double-check the tests just in case.
Alright, here's a new version which drops the -moz prefix.

I've also gone over the tests again and they seem fine to me now (the -moz-list change still seems necessary for that test to pass, though).

Try seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e097566a5b5
Attachment #8782644 - Attachment is obsolete: true
Attachment #8782944 - Flags: review?(dbaron)
Comment on attachment 8782944 [details] [diff] [review]
1069015-restore-code-for-placeholder-shown-pseudo-class.diff

>diff --git a/layout/reftests/css-placeholder/css-simple-styling-ref.html b/layout/reftests/css-placeholder/css-simple-styling-ref.html
>--- a/layout/reftests/css-placeholder/css-simple-styling-ref.html
>+++ b/layout/reftests/css-placeholder/css-simple-styling-ref.html
>@@ -1,13 +1,12 @@
> <!DOCTYPE html>
> <html>
>   <style>
>     :-moz-any(input, textarea) {
>-      font: -moz-list;
>       font-family: mono;
>       font-style: italic;
>       color: blue;
>       word-spacing: 5px;
>       text-shadow: 1px 1px 1px red;
>     }
>   </style>
>   <body>
>diff --git a/layout/reftests/css-placeholder/css-simple-styling.html b/layout/reftests/css-placeholder/css-simple-styling.html
>--- a/layout/reftests/css-placeholder/css-simple-styling.html
>+++ b/layout/reftests/css-placeholder/css-simple-styling.html
>@@ -5,24 +5,22 @@
>     applied to ::-moz-placeholder.
>   -->
>   <style>
>     /*
>      * We need to set some properties on the <input> because its size will
>      * depend on its font.
>      */
>     input, textarea {
>-      font: -moz-list;
>       font-family: mono;
>       font-style: italic;
>     }
> 
>     :-moz-any(input, textarea)::-moz-placeholder {
>       opacity: 1.0;
>-      font: -moz-list;
>       font-family: mono;
>       font-style: italic;
>       color: blue;
>       word-spacing: 5px;
>       text-shadow: 1px 1px 1px red;
>     }
>   </style>
>   <body>

Does comment 6 mean that you need to not make this change anymore?

>diff --git a/layout/reftests/css-placeholder/input/style-shown.css b/layout/reftests/css-placeholder/input/style-shown.css
>+input:placeholder-shown {
>+  -moz-appearance: none;
>+  color: -moz-FieldText;
>+  background-color: green;
>+}

Maybe this background should be red, since it seems intended to be overridden.  And maybe the same for the color?

>diff --git a/layout/reftests/css-placeholder/textarea/style-shown.css b/layout/reftests/css-placeholder/textarea/style-shown.css

Same here?

r=dbaron with that
Attachment #8782944 - Flags: review?(dbaron) → review+
>Does comment 6 mean that you need to not make this change anymore?

I suppose so. I only made that change because the reftest fails for me locally with the -moz-list bits left there, but since it isn't directly related to this patch or the :placeholder-shown pseudoclass, I might as well just omit the change.


>Maybe this background should be red, since it seems intended to be overridden.  And maybe the same for the color?

Agreed, done.


A fresh try run with the revised patch still seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=021f5b6dcaa7

Carrying over r+ and requesting check-in.
Attachment #8782944 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74eb4c3934f7
Restore code for :placeholder-shown pseudo-class. r=dbaron
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74eb4c3934f7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1301900
You need to log in before you can comment on or make changes to this bug.