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

Status: VERIFIED FIXED

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

Description

[Paul Sowden]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

seeing this on Linux 2002-02-17-06 too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[David Baron :dbaron:]

ReResolveStyleContext presumably needs to ProbePseudoStyleContextFor and cause a
framechange hint if it gets one where there wasn't one before.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[David Baron :dbaron:]

hyatt changed things on 12-17 so switching stylesheets just causes a reresolve
instead of a frame reconstruct.  That's probably what did it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm... ProbePseudoStyleContextFor seems to want a pseudo.. so would we want to
separately probe for nsCSSAtoms::beforePseudo and nsCSSAtoms::afterPseudo, I guess?

Comment 7

[David Baron :dbaron:]

yep.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

[Joanne Hunter]

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

[fantasai]

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

[fantasai]

Attachment: Content one as persistent stylesheet

Comment 12

[fantasai]

Attachment: only alternate stylesheet w/ content

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well... do we use ReResolveStyleContext for anything other than stylesheet
switching?

Comment 14

[David Baron :dbaron:]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[fantasai]

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

[David Baron :dbaron:]

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

[Randell Jesup [:jesup] (needinfo me)]

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

[Jacob Kjome]

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

[John P Baker]

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.

Comment 21

[Jesse Ruderman]

See also bug 57226, Changing attributes does not reframe :before/:after
generated content [GC].  Is this a dup?

Comment 22

[David Baron :dbaron:]

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

[Madhur Bhatia]

cc'ing myself

Comment 24

[David Baron :dbaron:]

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.

Comment 25

[Madhur Bhatia]

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

[Madhur Bhatia]

Attachment: style sheet for the reduced testcase

Comment 27

[Madhur Bhatia]

Attachment: reduced testcase

Comment 28

[Madhur Bhatia]

my Bad...... please ignore comment 25, comment 26, and comment 27.
They were supposed to be included in bug 171324 :-)

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: proof-of-concept

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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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....

Comment 32

[David Baron :dbaron:]

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?

Comment 33

[Boris Zbarsky [:bzbarsky]]

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).

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Pretty much ready

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... :(

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: oops, I forgot the new files.

Comment 36

[Boris Zbarsky [:bzbarsky]]

I should just take this...

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Ok, this one I'm happy with

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).

Comment 38

[Boris Zbarsky [:bzbarsky]]

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...

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think nsLayoutUtils::IsGeneratedContentFrame and nsLayoutUtils::IsPseudoFrame
should be methods in nsIFrame (concrete inline methods, not virtual ones).

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Ooh. Good idea.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 108918 [details] [diff] [review]
Ooh.  Good idea.

sr=roc+moz

Comment 42

[Jacob Kjome]

any chance this can get commited someone else in Boris' absence?

Jake

Comment 43

[Boris Zbarsky [:bzbarsky]]

I can commit it myself, but _after_ it has review.

Comment 44

[David Baron :dbaron:]

Comment on attachment 108918 [details] [diff] [review]
Ooh.  Good idea.

r=dbaron

Comment 45

[Boris Zbarsky [:bzbarsky]]

fix checked in

Comment 46

[Bill Mason]

Verified in build 2003010508