restore code for :placeholder-shown pseudo-class

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: dbaron, Assigned: Thomas Wisniewski)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla51
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [parity-chrome][parity-safari][parity-webkit][parity-blink])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Keywords: dev-doc-needed

Comment 1

a year ago
Chrome 47 Support http://caniuse.com/#feat=css-placeholder-shown

Comment 2

a year ago
Also supported in Safari 9.0+.
[parity-chrome][parity-webkit] should be added to the Whiteboard field.

Updated

a year ago
Whiteboard: [parity-chrome][parity-safari][parity-webkit][parity-blink]
(Assignee)

Comment 3

10 months ago
Created attachment 8782644 [details] [diff] [review]
1069015-restore-code-for-placeholder-shown-pseudo-class.diff

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

Comment 4

10 months ago
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)
(Assignee)

Comment 5

10 months ago
>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.
(Assignee)

Comment 6

10 months ago
Created attachment 8782944 [details] [diff] [review]
1069015-restore-code-for-placeholder-shown-pseudo-class.diff

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

Comment 7

10 months ago
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+
(Assignee)

Comment 8

10 months ago
Created attachment 8786964 [details] [diff] [review]
1069015-restore-code-for-placeholder-shown-pseudo-class.diff

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

Updated

10 months ago
Keywords: checkin-needed

Comment 9

10 months ago
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

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74eb4c3934f7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

10 months ago
Depends on: 1301900
Doc is up-to-date:
https://developer.mozilla.org/en-US/docs/Web/CSS/:placeholder-shown
and
https://developer.mozilla.org/en-US/Firefox/Releases/51#CSS
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.