stylo: need support for ::-moz-placeholder pseudo element

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: bz, Assigned: cjku)

Tracking

(Blocks: 3 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
Priority: -- → P2
(Assignee)

Updated

3 months ago
Assignee: nobody → cku

Comment 1

2 months ago
This will fix many Alexa top site reftest failures.

Comment 2

2 months ago
Elevate to P1 as it's a key fix to enable stylo dogfooding.
Status: NEW → ASSIGNED
Priority: P2 → P1
CCing emilio. Emilio + bz, was there anything special we needed to do for moz-placeholder, or is it just a normal lazy pseudo?
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(emilio+bugs)
(Reporter)

Updated

2 months ago
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 5

2 months ago
Created attachment 8868672 [details]
input-styling.html

::placeholder works, ::-moz-placeholder not.
(Assignee)

Comment 6

2 months ago
WIP. Compare the difference between nsStyleSet & ServoStylSet
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
Created attachment 8869068 [details]
input-styling.html

update test case
Attachment #8868672 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8869060 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Summary: stylo: need support for :-moz-placeholder → stylo: need support for ::-moz-placeholder pseudo element
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8869691 - Flags: review?(emilio+bugs)
Attachment #8869692 - Flags: review?(emilio+bugs)
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.
Flags: needinfo?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 months ago
(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)
Flags: needinfo?(cku)

Comment 21

2 months ago
mozreview-review
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`.
Attachment #8869691 - Flags: review?(emilio+bugs)

Comment 22

2 months ago
mozreview-review
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 :)
Attachment #8869692 - Flags: review?(emilio+bugs) → review+
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.
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 27

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

Comment 28

2 months ago
mozreview-review
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.
Attachment #8869691 - Flags: review?(emilio+bugs)
(Reporter)

Comment 29

2 months ago
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)?
Flags: needinfo?(bzbarsky)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 months ago
mozreview-review
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?
Attachment #8869691 - Flags: review?(emilio+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

2 months ago
mozreview-review
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.
Attachment #8869691 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 44

2 months ago
Thanks, Emilio.

Create pull reques:
https://github.com/servo/servo/pull/17010
(Assignee)

Updated

2 months ago
Attachment #8869691 - Attachment is obsolete: true
(Assignee)

Comment 45

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

Comment 46

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

Comment 47

2 months ago
I think the test expectations got updated already...
(Assignee)

Comment 48

2 months ago
Xidorn had updated reftest.list it in bug 1341102 already.
(Assignee)

Updated

2 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.