Open
Bug 501848
Opened 15 years ago
Updated 2 years ago
quirk.css leads to too much restyling due to :-moz-first-node selectors
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
NEW
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | - |
People
(Reporter: bzbarsky, Unassigned)
References
(Depends on 1 open bug, )
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
|
17.61 KB,
patch
|
Details | Diff | Splinter Review |
quirk.css has selectors like "body > p:-moz-first-node" in it. This marks any node with a <p> kid with the edge selector flag (because we do the marking in SelectorMatches). So after that on any append to such a node we end up restyling its last kid... Ideally we wouldn't add the edge selector flag here unless the node is actually a <body>. The url field has a (zipped) test page where we hit this a bunch, at least if bug 501847 is worked around.
|
Reporter | |
Comment 1•15 years ago
|
||
This doesn't help for slow selectors that are not part of the rightmost part of the selector, but it does help with ones that are... I'm still thinking about a sane way to make this work in general without making SelectorMatchesTree recursive or something.
Attachment #386424 -
Flags: superreview?(dbaron)
Attachment #386424 -
Flags: review?(dbaron)
Comment on attachment 386424 [details] [diff] [review] A possible approach >- const PRBool setNodeFlags = aForStyling && aStateMask == 0 && !aAttribute; >+ const PRBool setNodeFlags = aForStyling && aStateMask == 0 && !aAttribute && 0; This seems a little suspicious to me.
|
|
Reporter | |
Comment 3•15 years ago
|
||
> This seems a little suspicious to me.
Oops. That was left over from a test I was doing. It shouldn't be there.|
|
Reporter | |
Comment 4•15 years ago
|
||
Comment on attachment 386424 [details] [diff] [review] A possible approach On the other hand, even without that we fail tests. I need to look into why.
Attachment #386424 -
Flags: superreview?(dbaron)
Attachment #386424 -
Flags: superreview-
Attachment #386424 -
Flags: review?(dbaron)
Attachment #386424 -
Flags: review-
|
|
Reporter | |
Comment 5•15 years ago
|
||
I'm still trying to figure out a cleaner solution to this.
Attachment #386424 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 6•15 years ago
|
||
So the problem with making this work in general in SelectorMatchesTree is that we'd need to keep track of all the nodes that might need to have flags set. And then depending on whether we finally match we either set the flags or not. Or something. If the function were recursive, we could just keep track of this on the stack; since it's iterative we'd have to maintain a separate stack for this. I'm not sure it's worth it.... I'm also not that happy with the branches and complexity this adds, honestly. I'd considered just keeping track of the flags in the data struct, but that still leaves the issue of when to actually move them to the nodes.
|
|
Reporter | |
Comment 7•15 years ago
|
||
I've been seeing this come up a good bit recently when looking at old "big pages"....
blocking2.0: --- → ?
Keywords: perf
blocking2.0: ? → -
|
|
Reporter | |
Updated•14 years ago
|
Assignee: nobody → bzbarsky
|
|
Reporter | |
Updated•14 years ago
|
Priority: -- → P1
|
|
Reporter | |
Comment 8•6 years ago
|
||
Emilio, is the problem this bug describes still a problem with stylo?
Flags: needinfo?(emilio)
|
||
Comment 9•6 years ago
|
||
Yeah, I fixed some of these cases in bug 1427625, so maybe not so big of an issue for page loads, but Stylo still has the same mechanism for invalidating due to DOM insertion and such. This is basically bug 1443066.
Flags: needinfo?(emilio)
|
||
Comment 10•6 years ago
|
||
Moving to p3 because no activity for at least 24 weeks. See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P1 → P3
|
||
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•