Closed Bug 737786 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 3 obsolete files)

15.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.52 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
27.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.44 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.60 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
25.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
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 don't follow why we want to switch. Could you explain?
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.
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.
Blocks: 673873
Blocks: 556145
Keywords: student-project
Whiteboard: [mentor=mounir][lang=c++]
(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?
Assignee: nobody → fryn
Status: NEW → ASSIGNED
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.
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.
Blocks: 765646
No longer blocks: 765646
Depends on: 769405
Raphael might work on that if he finds time.
If Raphael finds time, he can take over the bug. This will at most be a side-project for me.
Depends on: 768737
No longer depends on: 769405
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.
I've been working on that this evening. Got something working. I will attach patches for review later (likely tonight).
Assignee: fryn → mounir
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.
Attachment #670580 - Flags: review?(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
Attachment #670580 - Flags: review?(bzbarsky) → review+
(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.
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 on attachment 670761 [details] [diff] [review] Part 2a - Remove :-moz-placeholder (pseudo-class) r=me
Attachment #670761 - Flags: review?(bzbarsky) → review+
Comment on attachment 670763 [details] [diff] [review] Part 2b - Create ::-moz-placeholder (pseudo-element) r=me
Attachment #670763 - Flags: review?(bzbarsky) → review+
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 on attachment 670764 [details] [diff] [review] Part 2c - Switch all :-moz-placeholder to ::-moz-placeholder (and pass tests) r=me
Attachment #670764 - Flags: review?(bzbarsky) → review+
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?
(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...
> 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?
(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).
> 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?
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...
Attachment #671803 - Flags: review?(bzbarsky)
This is just to move the boilerplate out of the real patch.
Attachment #671863 - Flags: review?(bzbarsky)
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...
Attachment #671866 - Flags: review?(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.
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.
Can we deal with the reframes that changing overflow and white-space triggers?
(Note: I'm 99% sure the answer is "no, they assert and then bad things happen".)
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.
Attached patch Part 4 - TestsSplinter Review
Attachment #671888 - Flags: review?(bzbarsky)
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...
... and improves the test. This is fixing the resize handler not being usable when the placeholder is showing.
Attachment #671891 - Flags: review?(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 on attachment 671803 [details] [diff] [review] Part 2d - Fix tests in layout/reftests/css-placeholder/ r=me
Attachment #671803 - Flags: review?(bzbarsky) → review+
Comment on attachment 671863 [details] [diff] [review] Part 3a - Hook up to a restriction model r=me
Attachment #671863 - Flags: review?(bzbarsky) → review+
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. ;)
Attachment #671866 - Flags: review?(bzbarsky) → review-
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.
Attachment #671888 - Flags: review?(bzbarsky) → review+
Comment on attachment 671891 [details] [diff] [review] Part 5 - Add pointer-events. r=me
Attachment #671891 - Flags: review?(bzbarsky) → review+
Blocks: 769405
(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 :)
> resize: both; on input::-moz-placeholder has no effect. Why not? In any case, I would prefer we played it safe here.
Attached patch Interdiff for part 3b (obsolete) — Splinter Review
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 ;)
Attachment #671866 - Attachment is obsolete: true
Attachment #672784 - Flags: review?(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?
And also, what's the point of disallowing border but allowing margin and padding?
Comment on attachment 672784 [details] [diff] [review] Part 3b - Restrictions set for ::-moz-placeholder r- pending those bits being hashed out.
Attachment #672784 - Flags: review?(bzbarsky) → review-
Sorry, I lost the interdiff... I could generate it if you want to.
Attachment #672782 - Attachment is obsolete: true
Attachment #672784 - Attachment is obsolete: true
Attachment #675572 - Flags: review?(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.
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...
I meant "silently show the wrong thing".
It seems pretty much correct.
Comment on attachment 675572 [details] [diff] [review] Part 3b - Restrictions set for ::-moz-placeholder r=me
Attachment #675572 - Flags: review?(bzbarsky) → review+
Target Milestone: --- → mozilla19
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)
(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
And this problem isn't related to my homescreen not showing up (I've managed to isolate this to something in gaia).
Er, yes. That bit there is not valid CSS... Mounir, please fix?
(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.
No, you actually couldn't...
Keywords: student-project
(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?
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?
Depends on: 810796
(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.
Depends on: 811821
Blocks: 811915
Depends on: 813550
Depends on: 819871
Depends on: 823444
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
Depends on: 847850
Blocks: 1044139
(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.
Depends on: 1115623
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: