1376352 - stylo: Attempting to apply ::before/::after to a replaced element will lead to reframes on every restyle

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

There won't be a ::before/::after frame in this case, nor anonymous content generated for them, I believe.

So in compute_style_difference we get None from self.existing_style_for_restyle_damage and then if the old/new display is not "none", and the old/new content is not "none" we will do a reframe...

Seems like we could just check whether "content" or "display" changed from "none" to "not none" and if not, do nothing.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I wrote something for this.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1376352: properly handle ::before/::after rules applying to replaced elements.

Review commit: https://reviewboard.mozilla.org/r/153950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153950/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1376352: Test.

Review commit: https://reviewboard.mozilla.org/r/153952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153952/

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8882990 [details]
Bug 1376352: properly handle ::before/::after rules applying to replaced elements.

https://reviewboard.mozilla.org/r/153950/#review159086

::: servo/components/style/matching.rs:1591
(Diff revision 1)
> -                // The pseudo-element will remain undisplayed, so just avoid
> +            // The pseudo-element will remain undisplayed, so just avoid
> -                // triggering any change.
> +            // triggering any change.

This comment needs updating, now that we can get here when we keep an effective ::before or ::after.  Well, at least for Servo.  For stylo, I guess the is_existing_before_or_after check in accumulate_damage will prevent us from getting in here, and instead it is the matches_different_pseudos check in match_pseudos that handles this (as bz's old comment mentions), right?  And does that mean that old_style_generates_no_pseudo will always be true in here if cfg(feature = "gecko")?  Probably worth commenting here about how we really handle going from "some gen con" to "no gen con" elsewhere for stylo.

I do wonder if we have any existing tests for going between effective and ineffective content properties, and couldn't see one with a quick search.  Could you add one if you don't know of one?

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8882991 [details]
Bug 1376352: Test.

https://reviewboard.mozilla.org/r/153952/#review159092

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Comment on attachment 8882990 [details]
> Bug 1376352: properly handle ::before/::after rules applying to replaced
> elements.
> 
> https://reviewboard.mozilla.org/r/153950/#review159086
> 
> ::: servo/components/style/matching.rs:1591
> (Diff revision 1)
> > -                // The pseudo-element will remain undisplayed, so just avoid
> > +            // The pseudo-element will remain undisplayed, so just avoid
> > -                // triggering any change.
> > +            // triggering any change.
> 
> This comment needs updating, now that we can get here when we keep an
> effective ::before or ::after.  Well, at least for Servo.

This path is stylo-only anyway, servo always uses old_values.

> For stylo, I guess the is_existing_before_or_after check in accumulate_damage will
> prevent us from getting in here, and instead it is the
> matches_different_pseudos check in match_pseudos that handles this (as bz's
> old comment mentions), right?  And does that mean that
> old_style_generates_no_pseudo will always be true in here if cfg(feature =
> "gecko")?

Why would that be the case? You're right that, for existing pseudos, we just delegate the damage calculation to them, but there's no reason old_style_generates_no_pseudo will always be true. Indeed, in the test-case I added, old_style_generates_no_pseudo would be false (because the pseudo isn't display: none, and has an effective content property).

> Probably worth commenting here about how we really handle going
> from "some gen con" to "no gen con" elsewhere for stylo.

But will do this part anyway :)

> I do wonder if we have any existing tests for going between effective and
> ineffective content properties, and couldn't see one with a quick search. 
> Could you add one if you don't know of one?

Sure.

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> This path is stylo-only anyway, servo always uses old_values.

Oh, OK.  Could you comment to that effect too? :-)

> > For stylo, I guess the is_existing_before_or_after check in accumulate_damage will
> > prevent us from getting in here, and instead it is the
> > matches_different_pseudos check in match_pseudos that handles this (as bz's
> > old comment mentions), right?  And does that mean that
> > old_style_generates_no_pseudo will always be true in here if cfg(feature =
> > "gecko")?
> 
> Why would that be the case? You're right that, for existing pseudos, we just
> delegate the damage calculation to them, but there's no reason
> old_style_generates_no_pseudo will always be true. Indeed, in the test-case
> I added, old_style_generates_no_pseudo would be false (because the pseudo
> isn't display: none, and has an effective content property).

Hmm, I was thinking that the is_existing_before_or_after check in accumulate_damage prevents us from calling accumulate_damage_for (and thus compute_style_difference) for ::before/::after if its frame already exists.  Maybe I'm misreading something?  (Or it's the jetlag?  I didn't debug to test.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Hmm, I was thinking that the is_existing_before_or_after check in
> accumulate_damage prevents us from calling accumulate_damage_for (and thus
> compute_style_difference) for ::before/::after if its frame already exists. 
> Maybe I'm misreading something?  (Or it's the jetlag?  I didn't debug to
> test.)

Right, the key here is that even though the style of a pseudo may make us think that there's a pseudo, that pseudo may not exist (for replaced elements they're not generated, like the test-case).

I guess that we could only check new_style_generates_no_pseudo, since the pseudo-removal case is handled by the pseudo-element itself, and otherwise it wouldn't matter.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> (In reply to Cameron McCormack (:heycam) from comment #7)
> > Hmm, I was thinking that the is_existing_before_or_after check in
> > accumulate_damage prevents us from calling accumulate_damage_for (and thus
> > compute_style_difference) for ::before/::after if its frame already exists. 
> > Maybe I'm misreading something?  (Or it's the jetlag?  I didn't debug to
> > test.)
> 
> Right, the key here is that even though the style of a pseudo may make us
> think that there's a pseudo, that pseudo may not exist (for replaced
> elements they're not generated, like the test-case).
> 
> I guess that we could only check new_style_generates_no_pseudo, since the
> pseudo-removal case is handled by the pseudo-element itself, and otherwise
> it wouldn't matter.

Err, I meant !new_style_generates_no_pseudo, but that will make us reframe for replaced elements, so nope.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Right, the key here is that even though the style of a pseudo may make us
> think that there's a pseudo, that pseudo may not exist (for replaced
> elements they're not generated, like the test-case).

Got it.  Thanks for the forthcoming comment in here that mentions something about this. ;-)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8882991 [details]
Bug 1376352: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153952/diff/1-2/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/17593

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3916ff00f371
Test. r=heycam

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3916ff00f371