This will fix many Alexa top site reftest failures.

Elevate to P1 as it's a key fix to enable stylo dogfooding.

CCing emilio. Emilio + bz, was there anything special we needed to do for moz-placeholder, or is it just a normal lazy pseudo?

Placeholder should work already AFAICT. We have bugs with placeholder removal (and with removal of all lazy element-backed pseudos in general). I plan to address those after bug 1364412. I can investigate why some placeholder reftests are failing, but it's definitely working in autoland.

::placeholder works, ::-moz-placeholder not.

WIP. Compare the difference between nsStyleSet & ServoStylSet

update test case

Is there a good reason for doing this instead of rewriting `::-moz-placeholder` as `::-placeholder` when parsing selectors? I think that's what we do for most of the properties that are aliases, and that's arguably simpler.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17) > Is there a good reason for doing this instead of rewriting > `::-moz-placeholder` as `::-placeholder` when parsing selectors? I think > that's what we do for most of the properties that are aliases, and that's > arguably simpler. Like this one? https://reviewboard.mozilla.org/r/140698/ It will make serialization test case failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=786cd5b91962b365b575225a75358afcacd41d64&selectedJob=100156424 layout/style/test/test_selectors.html | selector 'input::-moz-placeholder' should serialize to 'input::-moz-placeholder'. - got "input::placeholder", expected "input::-moz-placeholder" Input "::-moz-placeholder" should get "::-moz-placeholder" output, instead of "::placeholder". I agree with you, do the change in parser is much simpler. (I personally think this failure is ignoreable, since ::-moz-placeholder is moz-specific alias)

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review144868 That bit below is wrong, so clearing review flag for now. I'd ideally do it at the parser level though. The serialization of `::-moz-placeholder` is definitely intentional reading bug 1069012... Let's ask bz about whether he thinks it's sensible to stop serializing as `-moz-placeholder`. ::: servo/components/style/matching.rs:1017 (Diff revision 3) > + // If pseudo is ::placeholder, we need to match both ::placeholder > + // and ::moz-placeholder, which is an moz-specific alias of > + // ::placeholder. > + if *pseudo == PseudoElement::Placeholder && > + stylist.has_moz_placeholder_selector() { > + stylist.push_applicable_declarations(self, This bit is wrong, and the resulting rules won't be properly ordered in presence of `important` rules. We should change instead the `add_rule_to_map` function, and add to the `Placeholder` map both with `Placeholder` and `MozPlaceholder` pseudos instead. That would allow you to remove this hack and the `has_moz_placeholder_selector`.

Comment on attachment 8869692 [details] Bug 1348490 - Part 2. Enable reftests of ::moz-placeholder. https://reviewboard.mozilla.org/r/141264/#review144870 This is obviously fine, once the previous patch is sorted out :)

Hi Boris, So bug 1069012 added tests to ensure serialization of `::-moz-placeholder` was preserved as `::-moz-placeholder`. Implementing the alias in Stylo, it seems quite easier to just do it at the parser level, the same way we do for CSS properties, but given it seemed pretty intentional, I'd love to know why. It's not terribly hard to make it work the same way as in Gecko, but it's just another special-case we need to think about once we're in style code. I'm not opposed to implement it the same way, but I'd like to know the reasoning after it, and whether it would be sane to drop that bit or not.

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review144868 Yes, do the change at parser level is simpler. (Update current solution base on your suggestion) I will go back to https://reviewboard.mozilla.org/r/140698/ if we all aggree to ignore serialization test failure of "::-moz-placeholer" in stylo.

Or, we probably can just remove "::-moz-placeholder" support in gecko, and replace "::-moz-placeholder" in all reftest by "::placeholder"

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review144880 ::: servo/components/style/stylist.rs:500 (Diff revision 4) > + stylesheet: &Stylesheet) > + { > + if let Some(pseudo) = selector.pseudo_element() { > + self.add_rule_to_pseudos_map(pseudo, selector, rule, stylesheet); > + // ::-moz-placeholder is moz-specific alias of ::placeholder. > + if *pseudo == PseudoElement::MozPlaceholder { When will we match against `MozPlaceholder`? We don't have any code that would do that, right? Also, it seems to me that following your approach we should also add regular `Placeholder` rules to the placeholder map, so they are effectively an alias. But given that way the maps would be identical, we can just use one. I was thinking of something like: ```rust if let Some(pseudo) = selector.pseudo_element() { let canonical_pseudo = matches *pseudo { PseudoElement::MozPlaceholder => PseudoElement::Placeholder, _ => pseudo.clone(), }; self.pseudos_map .entry(pseudo) .or_insert_with(..) // ... ``` Also, perhaps it's worth moving that `canonical_pseudo` bit to its own method in `pseudo_element.rs`. That way the selector-matching code can become: ```rust // We don't expect to match against a non-canonical pseudo-element. debug_assert_eq!(implemented_pseudo, implemented_pseudo.canonical()); implemented_pseudo == pseudo.canonical() ``` And we should probably also make `lazy_pseudo_rules` look at the canonical pseudo I think. That would make matching against `MozPlaceholder` free, if we care about `getComputedStyle(elem, "::-moz-placeholder")`, which I don't know if gecko supports.

I think the main reason we did it the way we did was to avoid confusing web developers. If you write a rule like this: ::-moz-placeholder { color: green; } and then you get the cssText and it comes back as "::placeholder { color: green }", that's pretty confusing, especially since in non-Gecko browsers those have different behavior. My gut feeling, if it's easy enough, would be to preserve the Gecko behavior, and then work on bug 1300896 (convert our UA sheets, add use counters, etc). > `getComputedStyle(elem, "::-moz-placeholder")`, which I don't know if gecko supports Should be supported fine. Do we have an existing bug on the :-moz-placeholder pseudoclass, by the way (should get parsed, but never match)?

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #29) > I think the main reason we did it the way we did was to avoid confusing web > developers. If you write a rule like this: > > ::-moz-placeholder { color: green; } > > and then you get the cssText and it comes back as "::placeholder { color: > green }", that's pretty confusing, especially since in non-Gecko browsers > those have different behavior. > > My gut feeling, if it's easy enough, would be to preserve the Gecko > behavior, and then work on bug 1300896 (convert our UA sheets, add use > counters, etc). > > > `getComputedStyle(elem, "::-moz-placeholder")`, which I don't know if gecko supports > > Should be supported fine. Yeah, shouldn't be extremely hard, let's do it that way then. > Do we have an existing bug on the :-moz-placeholder pseudoclass, by the way > (should get parsed, but never match)? From a quick look didn't find anything, filed bug 1366709.

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review144880 > When will we match against `MozPlaceholder`? We don't have any code that would do that, right? > > Also, it seems to me that following your approach we should also add regular `Placeholder` rules to the placeholder map, so they are effectively an alias. > > But given that way the maps would be identical, we can just use one. I was thinking of something like: > > ```rust > if let Some(pseudo) = selector.pseudo_element() { > let canonical_pseudo = matches *pseudo { > PseudoElement::MozPlaceholder => PseudoElement::Placeholder, > _ => pseudo.clone(), > }; > > self.pseudos_map > .entry(pseudo) > .or_insert_with(..) > // ... > ``` > > Also, perhaps it's worth moving that `canonical_pseudo` bit to its own method in `pseudo_element.rs`. That way the selector-matching code can become: > > ```rust > // We don't expect to match against a non-canonical pseudo-element. > debug_assert_eq!(implemented_pseudo, implemented_pseudo.canonical()); > implemented_pseudo == pseudo.canonical() > ``` > > And we should probably also make `lazy_pseudo_rules` look at the canonical pseudo I think. That would make matching against `MozPlaceholder` free, if we care about `getComputedStyle(elem, "::-moz-placeholder")`, which I don't know if gecko supports. Hi Emilio, I do not totally understand that last requirement. I thought the cascade type of both ::moz-placehodler & ::placeholder are lazy, so lazy_pseudo_rules look at both of them. Doesn't it?

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review145204 Sorry for the lag, I had reviewed this yesterday, but hadn't published it... sigh. The last suggestion is doing `let pseudo = pseudo.canonical()` in the `lazy_pseudo_rules` function. That way, we can get there with `MozPlaceholder`, and that way we'll look at the map of the `Placeholder` pseudo. Does that make sense? Though you said over IRC that even without it you got correct rules for `::-moz-placeholder`. That's because we call `GetPseudoType` from http://searchfox.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#656. Given that, we can either add the `let pseudo = pseudo.canonical()` name, or just `debug_assert_eq!(pseudo, pseudo.canonical());`. ::: servo/components/style/gecko/wrapper.rs:1262 (Diff revision 6) > // TODO(emilio): I believe we could assert we are a pseudo-element and > // match the proper pseudo-element, given how we rulehash the stuff > // based on the pseudo. > match self.implemented_pseudo_element() { > - Some(ref pseudo) => pseudo == pseudo_element, > + Some(ref pseudo) => { > + if *pseudo == PseudoElement::Placeholder { These lines can just be `*pseudo == pseudo_element.canonical()` ::: servo/components/style/stylist.rs:486 (Diff revision 6) > { > - let map = if let Some(pseudo) = selector.pseudo_element() { > - self.pseudos_map > - .entry(pseudo.clone()) > + if let Some(pseudo) = selector.pseudo_element() { > + let map = self.pseudos_map > + .entry(pseudo.canonical()) > - .or_insert_with(PerPseudoElementSelectorMap::new) > + .or_insert_with(PerPseudoElementSelectorMap::new) > - .borrow_for_origin(&stylesheet.origin) > + .borrow_for_origin(&stylesheet.origin); Why the refactor here? Not that I'm opposed to it, but seems unnecessary, and not necessarily cleaner?

Comment on attachment 8869691 [details] Bug 1348490 - Part 1. Match both ::placehoder & ::moz-placeholder for placeholder pseudo-elements. https://reviewboard.mozilla.org/r/141262/#review145566 r=me, thanks! ::: servo/components/style/gecko/pseudo_element.rs:101 (Diff revisions 8 - 9) > > /// Covert non-canonical pseudo-element to canonical one, and keep a > /// canonical one as it is. > + #[inline] > pub fn canonical(&self) -> PseudoElement { > return match *self { nit: You can drop the return if you also drop the semicolon.

Thanks, Emilio. Create pull reques: https://github.com/servo/servo/pull/17010

Please press autoland button at [1] after "Part 1" been merged back to autoland from servo, in case I can not response quick enough... [1] https://reviewboard.mozilla.org/r/141264/diff/9#index_header

hg error in cmd: hg rebase -s 9334148de5be -d 4ffd099ef322: abort: can't rebase public changeset 9334148de5be (see 'hg help phases' for details)

I think the test expectations got updated already...

Xidorn had updated reftest.list it in bug 1341102 already.