Closed
Bug 1069015
Opened 10 years ago
Closed 8 years ago
restore code for :placeholder-shown pseudo-class
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
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.
Updated•10 years ago
|
Keywords: dev-doc-needed
Chrome 47 Support http://caniuse.com/#feat=css-placeholder-shown
Comment 2•9 years ago
|
||
Also supported in Safari 9.0+.
[parity-chrome][parity-webkit] should be added to the Whiteboard field.
Updated•9 years ago
|
Whiteboard: [parity-chrome][parity-safari][parity-webkit][parity-blink]
Assignee | ||
Comment 3•9 years ago
|
||
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
Reporter | ||
Comment 4•9 years 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•9 years 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•8 years ago
|
||
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•8 years 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•8 years ago
|
||
>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•8 years ago
|
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
Comment 10•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 11•8 years ago
|
||
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.
Description
•