Closed
Bug 126072
Opened 22 years ago
Closed 21 years ago
[FIXr][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.3beta
People
(Reporter: paul, Assigned: bzbarsky)
References
()
Details
(Keywords: css2, regression, testcase)
Attachments
(5 files, 4 obsolete files)
503 bytes,
text/html
|
Details | |
394 bytes,
text/html
|
Details | |
64 bytes,
text/css
|
Details | |
542 bytes,
text/html
|
Details | |
39.26 KB,
patch
|
dbaron
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.8) Gecko/20020204 BuildID: 2002020406 When an alternate style sheet is selected from the menu the :before and :after psuedo classes aren't rendered. The problem also persists if the sheet is switched back to the prefered style sheet. Reproducible: Always Steps to Reproduce: 1. load up page 2. select alternate style sheet from Mozilla's menu Actual Results: No psuedo classes were rendered Expected Results: Text should be rendered before and after the test paragraph.
![]() |
Assignee | |
Comment 1•22 years ago
|
||
seeing this on Linux 2002-02-17-06 too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
![]() |
Assignee | |
Comment 2•22 years ago
|
||
Interestingly enough, this works at http://web/bzbarsky/www/index.shtml (switch to the "Debug Look" stylesheet). Is this an issue when the first sheet has no generated content?
Comment 3•22 years ago
|
||
ReResolveStyleContext presumably needs to ProbePseudoStyleContextFor and cause a framechange hint if it gets one where there wasn't one before.
Summary: :before and :after psuedo classes are not rendered when using alternate style sheets → [RR]:before and :after psuedo classes are not rendered when using alternate style sheets
![]() |
Assignee | |
Comment 4•22 years ago
|
||
Oh, this is a regression, btw. It works in a build from Dec 13, 2001. It fails with a build from Jan 7, 2002 (the next one I have after the Dec 13). I have to correct comment #2, I was testing that with 0.9.6 or 0.9.7. With a current build on my page the :after pseudo-elements on the <a> tags fail to render. Making the <a> elts display:block fixes that; not sure whether that's related to the problem at hand.
Keywords: regression
Comment 5•22 years ago
|
||
hyatt changed things on 12-17 so switching stylesheets just causes a reresolve instead of a frame reconstruct. That's probably what did it.
![]() |
Assignee | |
Comment 6•22 years ago
|
||
Hmm... ProbePseudoStyleContextFor seems to want a pseudo.. so would we want to separately probe for nsCSSAtoms::beforePseudo and nsCSSAtoms::afterPseudo, I guess?
Comment 7•22 years ago
|
||
yep.
![]() |
Assignee | |
Comment 8•22 years ago
|
||
Ok.. I can "fix" this by just causing a framechange anytime there are pseudos on the new style context. This seems like it would be very broken. The problem I run into is that I can't tell whether the old style context had pseudos involved or not... I added the following code to ReResolveStyleContext: nsCOMPtr<nsIStyleContext> pseudoContext; aPresContext->ProbePseudoStyleContextFor(content, nsCSSAtoms::beforePseudo, oldContext, PR_FALSE, getter_AddRefs(pseudoContext)); if (pseudoContext) { fprintf(stderr, "Old style context is: %p\n", oldContext); pseudoContext->List(stderr, 0); } I get the following output on sheet switch (going from sheet one to sheet two): frame: Inline(p)(1) (0x8780e88) style: 0x8780e00 :before {} Wrong parent style context: style: 0x8780ccc {} should be using: style: 0x8781610 {} frame: Inline(p)(1) (0x8781068) style: 0x8780fe0 :after {} Wrong parent style context: style: 0x8780ccc {} should be using: style: 0x8781610 {} Old style context is: 0x8781610 0x87815dc(1) parent=0x8781610 :before { p:before weight: 1 { content: ""[before two] "" } } So it looks like there are some issues with style context parenting here to start with... The key problem, however is that the "old" style context already seems to have the new pseudo context as a descendant.... the "new" context also has it as a descendant (that's hardly surprising). The upshot of all this is that there is no way to tell that there used to be no pseudo, as far as I can see....
Comment 9•22 years ago
|
||
I'm getting this in Mozilla 0.9.8 too (specifically the package compiled for Debian GNU/Linux's 'unstable' branch, version 2:0.9.8-2), and things look even more screwy here. At http://menagerie.tf/~jrhunter/mozoddity/ (it's a copy of the front page of my personal website, which I'm currently working on), if you switch to "nocode.css", there should be little colored 'degree' characters after every link. As per this bug, they don't show up in most of the document body, but they DO show up in the Table Of Contents. I'm not sure how/if this helps, but it strikes me as being one of those "things that make you go 'hmmm'".
Comment 10•22 years ago
|
||
bz -- I think you may need to reframe in any case. There are three scenarios: Persistent stylesheet has content - Switching stylesheets doesn't change content. Preferred stylesheet has content - Switching stylesheets causes content to disappear. Only alternate stylesheet has content - Switching stylesheets doesn't create content. Also, Mozilla crashes sometimes if there are images involved.
Comment 11•22 years ago
|
||
Comment 12•22 years ago
|
||
![]() |
Assignee | |
Comment 13•22 years ago
|
||
Well... do we use ReResolveStyleContext for anything other than stylesheet switching?
Comment 14•22 years ago
|
||
We use ReResolveStyleContext for similar things to sheet changes such as rule removal / insertion and selector modification (althougb before hyatt's re-enabling of theme switching at least some of these caused full reconstructs), and also for things like attribute changes and content state changes (:hover, :active) that affect which selectors match a content node.
![]() |
Assignee | |
Comment 15•22 years ago
|
||
I suspected something like that... It's :hover that bothers me... If I have: p:before { content: "three" } it seems like reframing the <p> on every :hover just because it has a pseudo would be a bad idea....
Comment 16•22 years ago
|
||
Only the pseudos need to be reframed--the rest of the content only needs a reflow, and that only if size-related properties have changed.
Comment 17•22 years ago
|
||
We don't need to reframe unnecessarily (you know what I mean, anyway). See RemoveGeneratedContentFrameSiblings for how to tell if they exist -- and we can then do a ProbePseudoStyleContextFor during ReResolve to see if they should continue to exist (if they don't, in which case we should get a ReResolve). Making them disappear might be a tiny bit tricky, but I think ResolvePseudoStyleContextFor has a useful return value in that case...
Comment 18•22 years ago
|
||
If you need another testcase, I believe this is one (from the debugs): http://news.bbc.co.uk/hi/english/sci/tech/newsid_1880000/1880566.stm
Comment 19•22 years ago
|
||
Check out my humor page which uses :before and :after to insert images various places in the document. It loads with all the :before :after content showing. After switching the stylesheet and then going back to "Added Extras", the images don't show up anymore. Have to reload the page to get them back. http://www.visi.com/~hoju/humor.html Jake
Comment 20•22 years ago
|
||
I think my bug 133465 may be a manifestation of this bug as well - although in this case it is print media styles/stylesheet with generated content activated on print or print preview.
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
Comment 21•22 years ago
|
||
See also bug 57226, Changing attributes does not reframe :before/:after generated content [GC]. Is this a dup?
Comment 22•22 years ago
|
||
I marked bug 57226 as depending on this one. See bug 57226 comment 8 for why. (The testcases here are simpler, and I'm more convinced it's only one issue.)
Comment 23•22 years ago
|
||
cc'ing myself
Updated•22 years ago
|
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Updated•22 years ago
|
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Comment 24•22 years ago
|
||
Note that this is somewhat related to bug 145419, which is about :first-letter and :first-line, although that bug probably also depends on bug 23604.
Severity: normal → major
Target Milestone: mozilla1.2alpha → Future
Comment 25•22 years ago
|
||
The :before pseudo-element called from an external style sheeted linked via <link> tag does not work. But if the style is included internally within the <style> tag it works fine. see attached test case
Comment 26•22 years ago
|
||
Comment 27•22 years ago
|
||
Comment 28•22 years ago
|
||
my Bad...... please ignore comment 25, comment 26, and comment 27. They were supposed to be included in bug 171324 :-)
![]() |
Assignee | |
Comment 29•21 years ago
|
||
I have a not-too-bad solution for the case when :before and :after need to be added.... Still working on something that's not horribly slow for when they need to be removed.
![]() |
Assignee | |
Comment 30•21 years ago
|
||
This should work... two issues: 1) The usage of nsIInspectorCSSUtils is complete crap... I just can't get nsRuleNode::IsRoot callable from layout otherwise... we need some sort of glue to do that. :( 2) This still triggers warnings from VerifyStyleTree when it should be tearing down the pseudo frames... perhaps they are not even being torn down.
![]() |
Assignee | |
Comment 31•21 years ago
|
||
Comment on attachment 108686 [details] [diff] [review] proof-of-concept This is not ready for prime-time, of course; just putting it up in case people have any bright ideas....
Attachment #108686 -
Attachment is obsolete: true
Comment 32•21 years ago
|
||
Comment on attachment 108686 [details] [diff] [review] proof-of-concept >Index: layout/html/base/src/nsFrameManager.cpp > aPresContext->ResolvePseudoStyleContextFor(pseudoContent, pseudoTag, > parentContext, > &newContext); >+ if (newContext) { >+ // Check whether this is sitting at the root rulenode; if it >+ // is, we don't actually have style for this pseudo anymore >+ // and should kill this frame >+ nsRuleNode* ruleNode = nsnull; >+ newContext->GetRuleNode(&ruleNode); >+ if (ruleNode) { >+ nsCOMPtr<nsIInspectorCSSUtils> cssUtils = >+ do_GetService(kInspectorCSSUtilsCID); >+ PRBool root = PR_FALSE; >+ cssUtils->IsRuleNodeRoot(ruleNode, &root); >+ if (root) { >+ NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame); >+ aChangeList.AppendChange(aFrame, pseudoContent, >+ nsChangeHint_ReconstructFrame); >+ } >+ } >+ } Can't you just ProbePseudoStyleContextFor instead, and append a framechange if you get null? I think there's another bug somewhere on the same topic where I suggested having bits on the style context if they have child style contexts for :before or :after. Any thoughts on that idea, or can this approach fix the whole problem?
![]() |
Assignee | |
Comment 33•21 years ago
|
||
Hmm... I guess I could just probe and that gives me exactly the same style context _except_ when I need to reframe anyway... Lemme try that. Yeah, having bits on the parent would allow at least avoiding the two probes for new pseudos (we'd still need to resolve the context for the pseudo frame when it already exists, since we could just want to keep it exactly as-is).
![]() |
Assignee | |
Comment 34•21 years ago
|
||
I still get the warnings from VerifyFrameTree in FrameManager::ComputeStyleChangeFor, but I get them for _all_ frames that are being reframed (<head>, the <div>s whose "position" changed, etc on http://web/mit.edu/bzbarsky/www/index.shtml). The problem is that ReResolveStyleContext leaves the old context when a frame will be reframed. This means that the context has the wrong parent, of course, if its parent frame got a new context. We should try to make that warning system a little smarter, somehow... :(
![]() |
Assignee | |
Comment 35•21 years ago
|
||
Attachment #108690 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 36•21 years ago
|
||
I should just take this...
Assignee: dbaron → bzbarsky
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.3beta
![]() |
Assignee | |
Comment 37•21 years ago
|
||
Summary of changes: 1) Move some functions from being statics in nsPresShell.cpp to living in a new utility class -- nsLayoutUtils. 2) Detect when we need to create new :before or :after kids and reframe the parent frame 3) Detect when a :before or :after frame needs to go away and reframe it 4) Fix ComputeStyleChangeFor not to call VerifyStyleTree since the style context and frame trees are still in an inconsistent state there (pending reframing of some stuff) 5) Make callers of ComputeStyleChangeFor call DebugVerifyStyleTree as needed 6) Build system changes, blah blah Changes #4 and #5 make the bogus warnings I was seeing go away. I _do_ still see some warnings when switching back to the "Screen Look" on my site, but those look correct to me (some table pseudo-frame issues that I think we may be messing up).
Attachment #108691 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•21 years ago
|
Summary: [RR]:before and :after psuedo classes are not rendered when using alternate style sheets → [FIX][RR]:before and :after psuedo classes are not rendered when using alternate style sheets
![]() |
Assignee | |
Comment 38•21 years ago
|
||
Comment on attachment 108828 [details] [diff] [review] Ok, this one I'm happy with David? kin? Would you review? This only fixes the ::before and ::after pseudos; I need to read up a bit more on how we handle things like ::first-line and ::first-letter before trying to do anything useful with them...
Attachment #108828 -
Flags: superreview?(kin)
Attachment #108828 -
Flags: review?(dbaron)
![]() |
Assignee | |
Updated•21 years ago
|
Summary: [FIX][RR]:before and :after psuedo classes are not rendered when using alternate style sheets → [FIX][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
I think nsLayoutUtils::IsGeneratedContentFrame and nsLayoutUtils::IsPseudoFrame should be methods in nsIFrame (concrete inline methods, not virtual ones).
![]() |
Assignee | |
Comment 40•21 years ago
|
||
Attachment #108828 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•21 years ago
|
Attachment #108828 -
Flags: superreview?(kin)
Attachment #108828 -
Flags: review?(dbaron)
![]() |
Assignee | |
Updated•21 years ago
|
Attachment #108918 -
Flags: superreview?(roc+moz)
Attachment #108918 -
Flags: review?(dbaron)
Comment on attachment 108918 [details] [diff] [review] Ooh. Good idea. sr=roc+moz
Attachment #108918 -
Flags: superreview?(roc+moz) → superreview+
Comment 42•21 years ago
|
||
any chance this can get commited someone else in Boris' absence? Jake
![]() |
Assignee | |
Comment 43•21 years ago
|
||
I can commit it myself, but _after_ it has review.
Comment 44•21 years ago
|
||
Comment on attachment 108918 [details] [diff] [review] Ooh. Good idea. r=dbaron
Attachment #108918 -
Flags: review?(dbaron) → review+
![]() |
Assignee | |
Updated•21 years ago
|
Summary: [FIX][RR]:before and :after pseudo elements are not rendered when using alternate style sheets → [FIXr][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
![]() |
Assignee | |
Comment 45•21 years ago
|
||
fix checked in
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•