stylo: handle degenerate ::before and ::after in servo

RESOLVED DUPLICATE of bug 1366144

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED DUPLICATE of bug 1366144
4 months ago
3 days ago

People

(Reporter: bholley, Assigned: emilio)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

I have patches for this for bug 1331047, but given that I've been having trouble finding enough uninterrupted time to get that green and landed, and given that this impacts bug 1352487, I'm going to split them out and land them here.

The basic issue is as follows: Gecko's current setup filters out no-op ::before and ::after in ResolvePseudoElementStyle. This works fine for Gecko (where style context resolution is the entry point for doing style work), but less fine for Servo (where the style work happens much earlier).

So we really want to detect this case during the servo cascade and remove any eagerly-cascaded ::before and ::after styles from the originating element. This solves bug 1352487, and generally makes the gecko-side and servo-side view of the pseudos match, which is helpful for bug 1331047.

The suboptimal thing about my current patches is that still (currently) trigger reconstruction, because we add the pseudo during matching (which causes us to think that the set of pseudos changed), and then remove it again during the cascade (which causes us to think that the set of pseudos changed, again). My plan is to solve this by tracking the original set of pseudos, and then comparing after the cascade to see if there was a net change.
Depends on: 1335708
(Assignee)

Comment 1

4 months ago
So, I have a few questions about how the setup looks like, but I guess I'll wait to see the patches or something.

I also have a few questions about how Gecko deals with restyling of lazy pseudo-elements, ::placeholder and such, and how are we going to deal with them (I think we don't deal with them at all right now). I suspect that Gecko just arrives to the relevant frames restyling them, but that's something we can't do for obvious reasons, and I don't think we handle that at all though.

I have some ideas about how to make that work efficiently I think, perhaps we should discuss that too. I don't know if those pseudos are also included in your work for bug 1331047.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Attachment #8853689 - Attachment is patch: false
Attachment #8853689 - Attachment is obsolete: true
Attachment #8853689 - Attachment is patch: true
Attachment #8853689 - Flags: review?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> So, I have a few questions about how the setup looks like, but I guess I'll
> wait to see the patches or something.

We're chatting on IRC, but for posterity, the general plan is to remove ::before and ::after pseudos immediately after the cascade if we detect that they're degenerate.
 
> I also have a few questions about how Gecko deals with restyling of lazy
> pseudo-elements, ::placeholder and such, and how are we going to deal with
> them (I think we don't deal with them at all right now). I suspect that
> Gecko just arrives to the relevant frames restyling them, but that's
> something we can't do for obvious reasons, and I don't think we handle that
> at all though.

According to bz ::placeholder and ::selection are ok because we'll create a frame for them if needed, and that frame will handle resolving the pseudo style.

::first-letter and ::first-line are harder. We probably need something between eager and non-eager. See bug 1324618.

> 
> I have some ideas about how to make that work efficiently I think, perhaps
> we should discuss that too. I don't know if those pseudos are also included
> in your work for bug 1331047.

Comment 6

4 months ago
> ::placeholder and ::selection are ok because we'll create a frame for them if needed

::placeholder creates a frame if needed.  ::selection never creates a frame; it just gets its style queried directly during text painting.

That said, for ::placeholder we do need to restyle it if the styles change.  Something like:

  <style>
    input::placeholder { color: purple; }
    input.clicked::placeholder { color: yellow }
  </style>
  <input placeholder="Text" onclick="this.className = 'clicked'">
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6)
> > ::placeholder and ::selection are ok because we'll create a frame for them if needed
> 
> ::placeholder creates a frame if needed.  ::selection never creates a frame;
> it just gets its style queried directly during text painting.
> 
> That said, for ::placeholder we do need to restyle it if the styles change. 

Hm, we do need to handle that somehow. I filed bug 1352743 about this, let's continue the discussion there.
(Assignee)

Comment 8

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6)
> > ::placeholder and ::selection are ok because we'll create a frame for them if needed
> 
> ::placeholder creates a frame if needed.  ::selection never creates a frame;
> it just gets its style queried directly during text painting.
> 
> That said, for ::placeholder we do need to restyle it if the styles change. 
> Something like:
> 
>   <style>
>     input::placeholder { color: purple; }
>     input.clicked::placeholder { color: yellow }
>   </style>
>   <input placeholder="Text" onclick="this.className = 'clicked'">

Right, those are the cases I'm concerned about re. the current approach. This is also an issue if the rules start matching for the first time, in which case we need to detect we need to create the frame. But yeah, let's discuss in bug 1352743.
Depends on: 1353960
Blocks: 1243581
Assignee: bobbyholley → emilio+bugs
(Assignee)

Updated

3 days ago
Status: NEW → RESOLVED
Last Resolved: 3 days ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1366144
You need to log in before you can comment on or make changes to this bug.