Closed
Bug 1376352
Opened 7 years ago
Closed 7 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)
Tracking
()
RESOLVED
FIXED
mozilla56
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)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8882991 [details]
Bug 1376352: Test.
https://reviewboard.mozilla.org/r/153952/#review159092
Attachment #8882991 -
Flags: review?(cam) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
(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.)
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8882990 -
Attachment is obsolete: true
Assignee | ||
Comment 12•7 years ago
|
||
Comment 13•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3916ff00f371
Test. r=heycam
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•