Closed Bug 1352487 Opened 8 years ago Closed 7 years ago

stylo: handling of pseudos that should not be there is a performance footgun

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1366144
Tracking Flags:
Tracking Status
firefox55 --- affected

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 1 open bug)

Details

This is about the same code as bug 1290189 but a somewhat more specific situation. Consider this testcase: <!DOCTYPE html> <style> body::before { color: purple; } body.clicked::before { color: yellow; } </style> <body onclick="this.className = 'clicked'"> Click me. </body> The correct behavior for this testcase when a click happens is to do abolutely nothing. What stylo does is reframe the body. That happens because we land in compute_restyle_damage in servo/components/style/matching.rs for the ::before pseudo, discover that it has no frame, and do this bit: // Something else. Be conservative for now. RestyleDamage::reconstruct() This generally applies to any time the style of a pseudo-element that doesn't actually exist for the relevant element is changed. I see reframes for this reason (specifically for pseudo-elements) all over the place when browsing the web. For example, loading https://twitter.com/bz_moz ends up with dozens of such reframes.
Right, I believe we should be able to handle this correctly easily, by looking at the content property of the old and new style to determine whether it's ok to do nothing, pretty much the same way we look at the display property.
"content" would work for ::before and ::after, but not for ::-moz-placeholder or whatnot. That said, I looked into which pseudo-elements trigger the reframes on twitter and it's all ::before/::after. So maybe we should fix those and file a separate bug for the others, if we don't have a principled solution that will work for all of them.
Depends on: 1352565
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2) > "content" would work for ::before and ::after, but not for > ::-moz-placeholder or whatnot. Does Gecko have any handling to avoid the reconstructs in the ::moz-placeholder case? > That said, I looked into which pseudo-elements trigger the reframes on > twitter and it's all ::before/::after. So maybe we should fix those and > file a separate bug for the others, if we don't have a principled solution > that will work for all of them. Bug 1352565 should handle the ::before and ::after stuff. Emilio wrote up [1] which solves this bug, but assuming I can make it work I think I'd like to do bug 1352565 instead, since it's a bit more general. Really sorry for the collision emilio, I wish I could have landed this stuff sooner. [1] https://github.com/servo/servo/pull/16207
> Does Gecko have any handling to avoid the reconstructs in the ::moz-placeholder case? The question is backwards. The way this works in Gecko is that changes to pseudo-element stuff don't cause a reconstruct unless something explicitly goes out of its way to cause it. For the case of ::-moz-placeholder, Gecko doesn't have anything in place that would reconstructs in the ::-moz-placeholder case, so there aren't any. I'm not saying Gecko's setup is perfect, by the way. It leads to bug 8253 and its various duplicates/dependencies. Realistically, I think we want to separate pseudo-elements into two kinds. The first kind (::before, ::after, ::first-line, ::first-letter, what else?) can cause frames to be created by their presence plus their styles in the case of ::before/::after. If these things appear or disappear we should reframe as needed. The hard part is defining "as needed" in a sane way; Gecko had a lot of trouble with that because it was defined in terms of the frame tree and then we needed all sorts of complications to avoid always reframing replaced elements with ::before styles and so forth. I can't judge how stylo behaves here without either a lot more code reading or a link to some docs explaining the setup. The second kind of pseudo-element (::placeholder, ::selection, etc) exists or doesn't exist as a frame independent of styles and should never trigger reframing). We can use pseudo-element flags to tell the two kinds of pseudo-elements apart and ensure we never reframe for the second kind.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #4) > > Does Gecko have any handling to avoid the reconstructs in the ::moz-placeholder case? > > The question is backwards. The way this works in Gecko is that changes to > pseudo-element stuff don't cause a reconstruct unless something explicitly > goes out of its way to cause it. > > For the case of ::-moz-placeholder, Gecko doesn't have anything in place > that would reconstructs in the ::-moz-placeholder case, so there aren't any. > > I'm not saying Gecko's setup is perfect, by the way. It leads to bug 8253 > and its various duplicates/dependencies. > > Realistically, I think we want to separate pseudo-elements into two kinds. > The first kind (::before, ::after, ::first-line, ::first-letter, what else?) > can cause frames to be created by their presence plus their styles in the > case of ::before/::after. If these things appear or disappear we should > reframe as needed. The hard part is defining "as needed" in a sane way; > Gecko had a lot of trouble with that because it was defined in terms of the > frame tree and then we needed all sorts of complications to avoid always > reframing replaced elements with ::before styles and so forth. I can't > judge how stylo behaves here without either a lot more code reading or a > link to some docs explaining the setup. > > The second kind of pseudo-element (::placeholder, ::selection, etc) exists > or doesn't exist as a frame independent of styles and should never trigger > reframing). > > We can use pseudo-element flags to tell the two kinds of pseudo-elements > apart and ensure we never reframe for the second kind. Does this correspond to the difference between eagerly-resolved and lazily-resolved pseudo-elements [1]? I think it does, because (somewhat orthogonally to what the docs say), the main reason to have a pseudo be eager is that it's existence will cause frames to be generated. If the frames are already generated, then _they_ can be the ones to decide whether to lazily-resolve a pseudo or not. At present, only ::before and ::after are eager in stylo. We'll presumably want ::first-line and ::first-letter along with those. I'm not sure if we need anything else. [1] http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/servo/components/style/selector_parser.rs#84 [2] http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/servo/components/style/gecko/selector_parser.rs#401
> Does this correspond to the difference between eagerly-resolved and lazily-resolved pseudo-elements [1]? I guess it depends on how you redefine "eagerly resolved", as you note. The thing with ::first-line and ::first-letter is that they only apply to things that have blockframes. So eagerly computing them for everything might be a waste of time... I thought that was why they were lazy.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6) > > Does this correspond to the difference between eagerly-resolved and lazily-resolved pseudo-elements [1]? > > I guess it depends on how you redefine "eagerly resolved", as you note. > > The thing with ::first-line and ::first-letter is that they only apply to > things that have blockframes. So eagerly computing them for everything > might be a waste of time... I thought that was why they were lazy. Oh, you're right. I thought we just didn't support them yet, but it looks like we do and they're lazy, and from what you said I guess that's sensible. If they _are_ lazy though, then there's nothing servo can do about generating or not generating change hints for them (IIUC that's the suggestion at the bottom of comment 4). It will have to be handled manually by the block frame during restyle. So I guess my handling for ::before/::after in bug 1352565 should cover this.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7) > If they _are_ lazy though, then there's nothing servo can do about > generating or not generating change hints for them (IIUC that's the > suggestion at the bottom of comment 4). It will have to be handled manually > by the block frame during restyle. How would this handle new elements (already with a frame) matching ::first-line/::first-letter? We'd need to resolve that style all the time for all the blocks IIUC. I believe we need to find a more elegant way to solve this.
Matching them for the first time, I mean.
(Let's move the first-letter discussion into bug 1324618).
Assignee: nobody → bobbyholley
Priority: -- → P1
Assignee: bobbyholley → emilio+bugs
This was fixed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.