Closed Bug 1376352 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1376352: Test.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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.
Blocks: 1377197
I wrote something for this.
Assignee: nobody → emilio+bugs
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?
Attachment #8882990 - Flags: review?(cam) → review+
(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.
(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.)
(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.
(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.
(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. ;-)
Attachment #8882990 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3916ff00f371
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.