Closed Bug 126072 Opened 21 years ago Closed 21 years ago

[FIXr][RR]:before and :after pseudo elements are not rendered when using alternate style sheets


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






(Reporter: paul, Assigned: bzbarsky)




(Keywords: css2, regression, testcase)


(5 files, 4 obsolete files)

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.
seeing this on Linux 2002-02-17-06 too.
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
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?
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
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
hyatt changed things on 12-17 so switching stylesheets just causes a reresolve
instead of a frame reconstruct.  That's probably what did it.
Hmm... ProbePseudoStyleContextFor seems to want a pseudo.. so would we want to
separately probe for nsCSSAtoms::beforePseudo and nsCSSAtoms::afterPseudo, I guess?
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,
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
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
(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'".
bz -- I think you may need to reframe in any case.

There are three scenarios:

  Persistent stylesheet has content - Switching stylesheets doesn't change
  Preferred stylesheet has content - Switching stylesheets causes content to
  Only alternate stylesheet has content - Switching stylesheets doesn't create

Also, Mozilla crashes sometimes if there are images involved.
Well... do we use ReResolveStyleContext for anything other than stylesheet
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.
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....
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.
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...
If you need another testcase, I believe this is one (from the debugs):
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.

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.
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
See also bug 57226, Changing attributes does not reframe :before/:after
generated content [GC].  Is this a dup?
Blocks: 57226
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.)
cc'ing myself
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Keywords: css2
Keywords: testcase
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
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
Attached file reduced testcase
my Bad...... please ignore comment 25, comment 26, and comment 27.
They were supposed to be included in bug 171324 :-)

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.
Attached patch proof-of-concept (obsolete) — Splinter Review
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
    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.
Comment on attachment 108686 [details] [diff] [review]

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 on attachment 108686 [details] [diff] [review]

>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
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).
Attached patch Pretty much ready (obsolete) — Splinter Review
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/  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... :(
Attached patch oops, I forgot the new files. (obsolete) — Splinter Review
Attachment #108690 - Attachment is obsolete: true
I should just take this...
Assignee: dbaron → bzbarsky
Target Milestone: Future → mozilla1.3beta
Blocks: 184516
Blocks: 158192
Blocks: 141259
Attached patch Ok, this one I'm happy with (obsolete) — Splinter Review
Summary of changes:

1)  Move some functions from being statics in nsPresShell.cpp to living in a
    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
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
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)
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).
Attached patch Ooh. Good idea.Splinter Review
Attachment #108828 - Attachment is obsolete: true
Attachment #108828 - Flags: superreview?(kin)
Attachment #108828 - Flags: review?(dbaron)
Attachment #108918 - Flags: superreview?(roc+moz)
Attachment #108918 - Flags: review?(dbaron)
Comment on attachment 108918 [details] [diff] [review]
Ooh.  Good idea.

Attachment #108918 - Flags: superreview?(roc+moz) → superreview+
any chance this can get commited someone else in Boris' absence?

I can commit it myself, but _after_ it has review.
Blocks: 84582
Comment on attachment 108918 [details] [diff] [review]
Ooh.  Good idea.

Attachment #108918 - Flags: review?(dbaron) → review+
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
fix checked in
Closed: 21 years ago
Resolution: --- → FIXED
Verified in build 2003010508
You need to log in before you can comment on or make changes to this bug.