238072 - [GC] support positioned and floated generated content for non-replaced elements

Status: RESOLVED FIXED

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

Description

[Sekundes]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040318 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040318 Firefox/0.8.0+

 

Reproducible: Always
Steps to Reproduce:

Comment 1

[Sekundes]

It seems it's merely related to the code at
http://lxr.mozilla.org/mozilla/source/content/base/src/nsRuleNode.cpp#2678
developer, is it an easy task actually?

I don't have cvs tools to hack Mozilla.
And I'm not a C++ programmer, though.
Anyone who knows and is willing to fix this bug please contribute the patch.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, it's not.  While that code would need to be changed, other code needs to be
changed as well (mostly in nsCSSFrameConstructor).

Comment 3

[Boris Zbarsky [:bzbarsky]]

*** Bug 251850 has been marked as a duplicate of this bug. ***

Comment 4

[Boris Zbarsky [:bzbarsky]]

*** Bug 268062 has been marked as a duplicate of this bug. ***

Comment 5

[Boris Zbarsky [:bzbarsky]]

*** Bug 280459 has been marked as a duplicate of this bug. ***

Comment 6

[Boris Zbarsky [:bzbarsky]]

Notes to self on what's needed to support this:

1) Refactor XUL display types into ConstructrameByDisplayType
2) Add arg to ConstructFrameByDisplayType that will prevent construction of kids
   (and construction of before/after content).
3) Construct the before/after wrapper frames via ConstructFrameByDisplayType
4) Handle pseudo-frames for table display types of the wrapper
5) Verify that ReResolveStyleContext works right
6) Remove the rulenode code pointed to in comment 1

That should let us handle positioned, floated, overflowing, columnated, etc,
etc. before/after content....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note also that it would make it easier to fix bug 159799.

Comment 8

[Frank Wein [:mcsmurf]]

*** Bug 284982 has been marked as a duplicate of this bug. ***

Comment 9

[Anne (:annevk)]

Attachment: testcase

Comment 10

[Matthias Berndt]

*** Bug 296290 has been marked as a duplicate of this bug. ***

Comment 11

[Erik Fabert]

*** Bug 300242 has been marked as a duplicate of this bug. ***

Comment 12

[lightsolphoenix]

You know...IMO, the "fix" to these errors makes it rather obvious what the error
actually is...

It appears that if you set display: block on the replaced content, suddenly
floating and positioning works.  Could the problem be that generated content
isn't automatically converted to block elements, like every other element is
when it is positioned or floated?  After all, there is no behaviour defined for
positioned or floated inline elements, because it's not supposed to happen.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nope.

Comment 14

[Matthias Berndt]

(In reply to comment #12)
> You know...IMO, the "fix" to these errors makes it rather obvious what the error
> actually is...
> 
> It appears that if you set display: block on the replaced content, suddenly
> floating and positioning works.  Could the problem be that generated content
> isn't automatically converted to block elements, like every other element is
> when it is positioned or floated?  After all, there is no behaviour defined for
> positioned or floated inline elements, because it's not supposed to happen.
Could you please attach a test case? Setting display:block doesn't fix the
problem for me (Mozilla 1.7.8).

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This discussion is irrelevant.  See comment 6.

Comment 16

[Erik Fabert]

*** Bug 332487 has been marked as a duplicate of this bug. ***

Comment 17

[Philip Withnall (unavailable)]

*** Bug 332488 has been marked as a duplicate of this bug. ***

Comment 22

[Ralph]

Seeing how Firefox 3 seems to be around the corner (right now in RC1), will this "feature" (I though it was a bug) have to wait for a next version? (I guess v4?).

Just by reading comment #6 I get the idea is not an easy thing, but I have no idea how much of it has been done —and it would definitely be great if this was changed before Fx3 shipped.

So, any news, anyone?

Comment 23

[John Lascurettes]

(In reply to comment #22)
> Just by reading comment #6 I get the idea is not an easy thing, but I have no
> idea how much of it has been done —and it would definitely be great if this
> was changed before Fx3 shipped.

For what it's worth, both WebKit and Presto both render the testcase correctly (https://bugzilla.mozilla.org/attachment.cgi?id=179036).

Comment 24

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

This bug is tested in acid3.

Comment 25

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

(In reply to comment #6)
> 3) Construct the before/after wrapper frames via ConstructFrameByDisplayType

This step is a bit invasive because all the Construct...Frame methods need to know about it. It seems simpler to create an anonymous container element for the anonymous children. Then I think we can also simplify the anonymous content and frame construction by creating an anonymous element for each 'content' value, making them all children of the anonymous container element, and then doing one BindToTree and doing ConstructFrameByDisplayType on the anonymous container to construct the whole frame subtree at once.

Comment 26

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

I have a patch that works, albeit with some bugs which I need to fix. I also need to check ReResolveStyleContext and write a bunch of tests.

Comment 27

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

As usual, outer table frames are causing problems. If we get generated content with display:table, I have a problem, because the topmost generated content frame is going to have to be the outer table frame, but its pseudo-element has to be -moz-table-outer. The inner table frame is the one with the 'after' or 'before' tag. Now, that might actually work OK once I make nsLayoutUtils::IsGeneratedContentFor/GetBeforeFrame/GetAfterFrame grok the new setup, but it's a somewhat risky change.

I haven't looked at how to handle table pseudoframes yet. Right now making generated content display:table-cell just crashes.

Comment 28

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

Attachment: fix

Here it is!

I've already described the general approach. Apart from the main changes, there are supporting changes:

-- don't return USERDISABLED from nsGenConImageContent::IntrinsicState, treat it as SUPPRESSED
-- Make nsAttributeTextNode reflect the attribute of its *grandparent* since that's what we need to do in the new setup
-- bug 374193 has a reftest that seems to assume we don't create generated content for mtable::after. We do now, and I think that's correct, so I just turned it into a crashtest (which seems to have been the original intent).
-- Add mAdditionalStateBits to nsFrameConstructorState so that we can tag
frames created during CreateGeneratedContentFrame as NS_FRAME_GENERATED_CONTENT.
-- Table frame construction should pass PR_TRUE for aCanHaveGeneratedContent to ProcessChildren.
-- Don't treat any generated content as "ignorable whitespace" in tables.
-- ProcessChildren needs to create generated content inside its table-psuedo-frame-state-saving, and take generated content into account when processing table pseudo frames, since generated content can contribute to it now.
-- nsGenConNode needs to store an element and optional pseudo-element-name instead of just a parent nsIFrame. The problem is that when we create quote and counter nodes we don't know what the parent is going to be because we haven't created it yet. It seemed simpler and more robust to break the nsGenConNode's dependency on frames than to try to create the generated content container frame earlier. It might require changing more code this way, but the changes are straightforward. It does mean extra changes to nsCSSFrameConstructor::NotifyDestroyingFrame.
-- nsLayoutUtils::IsGeneratedContentFor has to be a little smarter because the frame whose style context pseudo is ":before" or ":after" is no longer necessarily the outermost generated content frame. In particular for generated tables the outermost generated content frame is the table outer frame which has the table-outer pseudo, and the ":before" or ":after" frame is its table-inner child.
-- The testcase for bug 380842 assumed that display:block generated content in an inline context would be converted to inline (or inline-block?). That no longer happens, so fix the testcase. This issue was actually raised in 380842.
-- a little testsuite for generated content.
  * display-types-01: test generating content of various 'display' types
  * dynamic-attr-01: test that attribute changes are reflected through attr()
  * dynamic-restyle-01: test that some simple style changes propagate into generated content
  * floated-01: test that 'float' generated content works
  * positioned-01: test that positioned generated content works
  * images-01: test that a broken generated content image displays nothing
  * table-ignoring-whitespace-01: test that generated content that evaluates to whitespace is not treated as ignorable whitespace in tables
  * table-parts-01: test creating generated content table parts in various contexts, e.g., that "tbody::before { display:table-cell; content:'X' }" does what it's supposed to do.

The table pseudo-frame stuff actually Just Worked, once I moved ProcessChildren's pseudo-frame processing to the right place and dealt with the ignorable whitespace problem. (There was an issue where some counters start off empty and then counter recalculation sets them to the right value; without the ignorable whitespace fix, we don't create frames for them, and there's no mechanism to track text node changes to recreate those frames, and even if there was it might not work well for generated content.)

Comment 29

[Boris Zbarsky [:bzbarsky]]

roc, it'll probably take me a week and a bit to get to this; moving this Sunday...

Comment 30

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

There's no rush at all.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 325737 [details] [diff] [review]
fix

>+++ mozilla-trunk/content/base/src/nsTextNode.cpp	2008-06-19 23:21:02.000000000 +1200
> nsAttributeTextNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
>+  if (mGrandparent) {
>+    mGrandparent->RemoveMutationObserver(this);
>+    mGrandparent = nsnull;

So aNullParent might not be true here (e.g. we might be unbinding the root of the anon content subtree), right?  But we still want to drop grandparent, no matter what.  Might be worth commenting to that effect.

> nsAttributeTextNode::AttributeChanged(nsIDocument* aDocument,
>     // Since UpdateText notifies, do it asynchronously.  Note that if we get
>     // unbound while the event is up that's ok -- we'll just have no parent
>     // when it fires, and will do nothing.    

s/parent/grandparent/ here.

>+++ mozilla-trunk/layout/base/nsCSSFrameConstructor.cpp	2008-06-19 23:21:02.000000000 +1200
>+  nsFrameState              mAdditionalStateBits; 

Add a comment documenting what will happen with these bits?

> nsCSSFrameConstructor::NotifyDestroyingFrame(nsIFrame* aFrame)
>+  if (!parentFrame) {
>+    // The root frame can't be generated...

Assert that, please.

>+nsCSSFrameConstructor::CreateGeneratedContent(nsIContent*     aParentContent,
>+      // Note that if there are increments or resets for this generated
>+      // content, they will be added *later* when we create the frame subtree

We had this issue before this patch too, right?  Might be worth filing a bug for this.

>+ * children of the XML element. Then we create a frame subtree for
>+ * the XML element as if it was a regular child of aParentFrame/aParentContent,

s/if it was/if it were/

> nsCSSFrameConstructor::CreateGeneratedContentFrame(nsFrameConstructorState& aState,
>+  rv = container->BindToTree(mDocument, aParentContent, container, PR_TRUE);

We've changed the binding parent setup for native anon content since you wrote this, I think.  That third argument should be aParentContent, not container.

>+  ConstructFrameInternal(aState, container, aParentFrame,
>+    elemName, kNameSpaceID_None, pseudoStyleContext, aFrameItems, PR_FALSE);

Probably safer to pass PR_TRUE as the last argument, I would think.

>@@ -10627,24 +10532,25 @@ nsCSSFrameConstructor::CreateContinuingF
>+    newFrame->AddStateBits(aFrame->GetStateBits() & NS_FRAME_GENERATED_CONTENT);

What about oofContFrame?  I'd think we want to pu the bits there, in fact; the placeholder should already be picking this bit up from its parent frame, right?

In fact, shouldn't the nsFrame::Init code be picking up the generated content bit from the prev-in-flow?  It seems like we flub things right now if the outermost generated content frame splits.

>@@ -12769,32 +12666,28 @@ nsCSSFrameConstructor::ProcessInlineChil

So this used to do pseudo-frames after ::after, while ProcessChildren did it before?  Fun times....  Good to see us being consistent now!

>+++ mozilla-trunk/layout/base/nsCounterManager.cpp	2008-06-19 23:21:02.000000000 +1200
>     // Get the content node for aNode's rendering object's *parent*,
>     // since scope includes siblings, so we want a descendant check on
>     // parents.  If aNode is for a pseudo-element, then the parent
>     // rendering object is the frame's content

s/the frame's/aNode's/ perhaps?

>     // element, then the parent rendering object is the frame's
>     // content's parent.

And here.

And maybe this comment should be before GetScopeContent's definition, not here?

>+        pseudo = content->Tag() == nsGkAtoms::mozgeneratedcontentbefore ?
>+            nsCSSPseudoElements::before : nsCSSPseudoElements::after;

In some ways, it would be simpler to just use nsCSSPseudoElements::before/after for the node tagname when we create the node...  I guess that could mess up chrome or user styling if the language being styled actually has <before> or <after> elements, but other than that I don't see a problem with it, and it would simplify some of the places where we go back and forth from one to the other.  What do you think of just doing that?   What about putting <before> or <after> in some Mozilla namespace and using it?

>+++ mozilla-trunk/layout/base/nsCounterManager.h	2008-06-19 23:21:02.000000000 +1200
>+#include "nsIFrame.h"

Why is this needed?  Just putting it in the .cpp should work, no?  Or do we use nsIFrame in this header?

>+    // Destroy nodes for pseudo in any lists, and return whether any
>     // nodes were destroyed.

s/pseudo/the given content and pseudo/ and mention that pseudo may be null?

>+++ mozilla-trunk/layout/base/nsGenConList.cpp	2008-06-19 23:21:02.000000000 +1200
> nsGenConList::NodeAfter(const nsGenConNode* aNode1, const nsGenConNode* aNode2)
>-  // XXX Switch to the frame version of DoCompareTreePosition?

Thing is, the code as currently written doesn't handle XBL well.  So we should have _some_ sort of XXX comment here.

>+++ mozilla-trunk/layout/base/nsGenConList.h	2008-06-19 23:21:02.000000000 +1200
> struct nsGenConNode : public PRCList {
>+  nsIContent* mParentContent;

Please document what this means?  That is, which content node it is in the various cases?

>+  nsGenConNode(nsIContent* aParentContent, nsIAtom* aPseudoType,
>-    NS_ASSERTION(aContentIndex <
>-                   PRInt32(aPseudoFrame->GetStyleContent()->ContentCount()),
>-                 "index out of range");

Why remove this?  I guess because we no longer have the style context available?  Is there a sane way we could keep asserting this?

>-    NS_ASSERTION(aContentIndex < 0 ||
>-                 aPseudoFrame->GetStateBits() & NS_FRAME_GENERATED_CONTENT,
>-                 "not generated content and not counter change");

Similar here.  If we did have the gen con content in some namespace, we could test that here, I guess.

>+++ mozilla-trunk/layout/generic/nsHTMLParts.h	2008-06-19 23:21:02.000000000 +1200
>+// Special Generated Content Frame. It contains text taken from an
>+// attribute of its *grandparent* content node. 

s/Frame/Content Node/ or some such?

>+++ mozilla-trunk/layout/reftests/generated-content/table-parts-01-ref.html	2008-06-19 23:21:02.000000000 +1200

Does this need all the "gen" stuff in <style>?  Seems like it shouldn't.

>+++ mozilla-trunk/layout/reftests/generated-content/table-parts-01.html	2008-06-19 23:21:02.000000000 +1200

Three comments on this test:

1) Please add a test where the parent is display:table, has a single display:tabe-cell kid, and has display:table-cell ::before/::after.  Give the three cells different heights, stick a background on them or something, and make sure they end up all at the same height (since they should all be in a single row).  Repeat this with a display:block parent.

2) Should have a test of display:table-row generated content in a block.  Possibly even a block with an existing display:table-row kid, and make sure that the rows affect each other's sizing.

3) Test that display:block kids of a display:table-row do in fact all end up in that row, and create cells (e.g. have another table-row that has table-cells in it so the columns have to line up) but do not affect each other's height styling (the row ends up at the height of the tallest cell, but the other blocks don't stretch.

>+++ mozilla-trunk/layout/style/nsRuleNode.cpp	2008-06-19 23:21:02.000000000 +1200
>-    // XXXbz For example, the calls to WipeContainingBlock in the
>-    // frame constructor will need to be changedif we allow
>-    // block-level generated content inside inlines.

I just reread the WipeContainingBlock part in question.  See the comments before the two WipeContainingBlock calls in nsCSSFrameConstructor, which have no bearing on reality after this patch.

With this change, it is in fact possible to have a block ::after inside an inline.  The only case when WipeContainingBlock looks at aIsAppend is when we're adding kids to the block of an {ib} split.  If we're doing that, and the ::after is a block, we certainly don't need to reframe.  So the worst-case scenario is that we'll reframe when we don't actually need to.  I guess that's OK, but fix the comments accordingly, and add comments inside WipeContainingBlock pointing out that aAppend may be incorrect when doing ::after stuff?  Or heck, we know at the callsites whether there is a appendAfterFrame/parentAfterFrame (depending on the method we're in),  so we can just make the boolean exact.  I'd prefer that.

I love the cruft removal this patch does!  Sorry it took me so long to get to it

It'd be a good idea to have dbaron look over the GenConList changes, I think.

r+sr=bzbarsky with at least the bindingParent thing fixed.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 325737 [details] [diff] [review]
fix

>diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsCounterManager.cpp mozilla-trunk/layout/base/nsCounterManager.cpp
>diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsCounterManager.h mozilla-trunk/layout/base/nsCounterManager.h
> struct nsCounterChangeNode : public nsCounterNode {
>     PRInt32 mChangeValue; // the numeric value of the increment or reset
> 
>-    // |aPseudoFrame| is not necessarily a pseudo-element's frame, but
>-    // since it is for every other subclass of nsGenConNode, we follow
>-    // the naming convention here.
>     // |aPropIndex| is the index of the value within the list in the
>     // 'counter-increment' or 'counter-reset' property.
>-    nsCounterChangeNode(nsIFrame* aPseudoFrame,
>+    nsCounterChangeNode(nsIContent* aContent, nsIAtom* aPseudo,


I'd rather make this comment about |aPseudo| rather than remove it.

>diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsGenConList.cpp mozilla-trunk/layout/base/nsGenConList.cpp
>diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsGenConList.h mozilla-trunk/layout/base/nsGenConList.h
> struct nsGenConNode : public PRCList {
>-  // The wrapper frame for all of the pseudo-element's content.  This
>-  // frame generally has useful style data and has the
>-  // NS_FRAME_GENERATED_CONTENT bit set (so we use it to track removal),
>-  // but does not necessarily for |nsCounterChangeNode|s.
>-  nsIFrame* const mPseudoFrame;
>+  nsIContent* mParentContent;
>+  nsIAtom*    mPseudoType; // nsGkAtoms::before or nsGkAtoms::after

The comment you added is not true for counter change nodes; you should instead update the comment you removed to reflect what's now going on, but still mention counter change nodes.

>+  nsGenConNode(nsIContent* aParentContent, nsIAtom* aPseudoType,
>+               PRInt32 aContentIndex)
>+    : mParentContent(aParentContent)
>+    , mPseudoType(aPseudoType)
>     , mContentIndex(aContentIndex)
>   {
>-    NS_ASSERTION(aContentIndex <
>-                   PRInt32(aPseudoFrame->GetStyleContent()->ContentCount()),
>-                 "index out of range");

I'm sad to see this assertion go.


I'm curious about the motivation for these changes as well.  Why do you not want to pass frames anymore?  Will the changes at the callers that required these changes lead to quotes or counters being affected by 'display:none' elements, which I believe would be incorrect?  (I haven't really looked at the callers.)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 325737 [details] [diff] [review]
fix

Er, never mind my last paragraph; I found the relevant bit within comment 28.  So r=dbaron.

(It might be worth passing the style context to the constructors instead of the pseudo type just so that you can put that assertion back in...)

Comment 34

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

(In reply to comment #31)
> (From update of attachment 325737 [details] [diff] [review])
> >+++ mozilla-trunk/content/base/src/nsTextNode.cpp	2008-06-19 23:21:02.000000000 +1200
> > nsAttributeTextNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
> >+  if (mGrandparent) {
> >+    mGrandparent->RemoveMutationObserver(this);
> >+    mGrandparent = nsnull;
> 
> So aNullParent might not be true here (e.g. we might be unbinding the root of
> the anon content subtree), right?  But we still want to drop grandparent, no
> matter what.  Might be worth commenting to that effect.

OK.

> > nsAttributeTextNode::AttributeChanged(nsIDocument* aDocument,
> >     // Since UpdateText notifies, do it asynchronously.  Note that if we get
> >     // unbound while the event is up that's ok -- we'll just have no parent
> >     // when it fires, and will do nothing.    
> 
> s/parent/grandparent/ here.

OK

> >+++ mozilla-trunk/layout/base/nsCSSFrameConstructor.cpp	2008-06-19 23:21:02.000000000 +1200
> >+  nsFrameState              mAdditionalStateBits; 
> 
> Add a comment documenting what will happen with these bits?

OK

> > nsCSSFrameConstructor::NotifyDestroyingFrame(nsIFrame* aFrame)
> >+  if (!parentFrame) {
> >+    // The root frame can't be generated...
> 
> Assert that, please.

OK

> >+nsCSSFrameConstructor::CreateGeneratedContent(nsIContent*     aParentContent,
> >+      // Note that if there are increments or resets for this generated
> >+      // content, they will be added *later* when we create the frame subtree
> 
> We had this issue before this patch too, right?  Might be worth filing a bug
> for this.

No, AFAIK we didn't have this issue before the patch because we processed the increments and resets when we created the outermost container frame for the generated content, before we generated the 'use' node.

> >+ * children of the XML element. Then we create a frame subtree for
> >+ * the XML element as if it was a regular child of aParentFrame/aParentContent,
> 
> s/if it was/if it were/

OK

> > nsCSSFrameConstructor::CreateGeneratedContentFrame(nsFrameConstructorState& aState,
> >+  rv = container->BindToTree(mDocument, aParentContent, container, PR_TRUE);
> 
> We've changed the binding parent setup for native anon content since you wrote
> this, I think.  That third argument should be aParentContent, not container.

OK thanks.

> >+  ConstructFrameInternal(aState, container, aParentFrame,
> >+    elemName, kNameSpaceID_None, pseudoStyleContext, aFrameItems, PR_FALSE);
> 
> Probably safer to pass PR_TRUE as the last argument, I would think.

OK.

> >@@ -10627,24 +10532,25 @@ nsCSSFrameConstructor::CreateContinuingF
> >+    newFrame->AddStateBits(aFrame->GetStateBits() & NS_FRAME_GENERATED_CONTENT);
> 
> What about oofContFrame?  I'd think we want to pu the bits there, in fact; the
> placeholder should already be picking this bit up from its parent frame, right?

No, because it can be the outermost generated content frame, e.g. for ::after { float:left; }.

> In fact, shouldn't the nsFrame::Init code be picking up the generated content
> bit from the prev-in-flow?  It seems like we flub things right now if the
> outermost generated content frame splits.

Near the end of nsCSSFrameConstructor::CreateContinuingFrame there's code to take care of this. That also should ensure that the continuation of an out-of-flow frame gets the right bit.

> >@@ -12769,32 +12666,28 @@ nsCSSFrameConstructor::ProcessInlineChil
> 
> So this used to do pseudo-frames after ::after, while ProcessChildren did it
> before?  Fun times....  Good to see us being consistent now!

Yay :-)

> >+++ mozilla-trunk/layout/base/nsCounterManager.cpp	2008-06-19 23:21:02.000000000 +1200
> >     // Get the content node for aNode's rendering object's *parent*,
> >     // since scope includes siblings, so we want a descendant check on
> >     // parents.  If aNode is for a pseudo-element, then the parent
> >     // rendering object is the frame's content
> 
> s/the frame's/aNode's/ perhaps?
> >     // element, then the parent rendering object is the frame's
> >     // content's parent.
> 
> And here.

That comment just needs to be fixed and moved to GetScopeContent. Something like

     // Get the content that determines the scope for aNode. For non-generated
     // content, this is the parent of the element associated with aNode,
     // otherwise it's the mParentContent stored in aNode.

> >+        pseudo = content->Tag() == nsGkAtoms::mozgeneratedcontentbefore ?
> >+            nsCSSPseudoElements::before : nsCSSPseudoElements::after;
> 
> In some ways, it would be simpler to just use nsCSSPseudoElements::before/after
> for the node tagname when we create the node...  I guess that could mess up
> chrome or user styling if the language being styled actually has <before> or
> <after> elements, but other than that I don't see a problem with it, and it
> would simplify some of the places where we go back and forth from one to the
> other.  What do you think of just doing that?   What about putting <before> or
> <after> in some Mozilla namespace and using it?

The text for nsCSSPseudoElements::before/after is ":before" and ":after", are those legitimate tag names? If I can use those as tag names, I'll do as you suggest.

> >+++ mozilla-trunk/layout/base/nsCounterManager.h	2008-06-19 23:21:02.000000000 +1200
> >+#include "nsIFrame.h"
> 
> Why is this needed?  Just putting it in the .cpp should work, no?  Or do we use
> nsIFrame in this header?

I can't remember, I'll try moving it.

> >+    // Destroy nodes for pseudo in any lists, and return whether any
> >     // nodes were destroyed.
> 
> s/pseudo/the given content and pseudo/ and mention that pseudo may be null?

Sure.

> >+++ mozilla-trunk/layout/base/nsGenConList.cpp	2008-06-19 23:21:02.000000000 +1200
> > nsGenConList::NodeAfter(const nsGenConNode* aNode1, const nsGenConNode* aNode2)
> >-  // XXX Switch to the frame version of DoCompareTreePosition?
> 
> Thing is, the code as currently written doesn't handle XBL well.  So we should
> have _some_ sort of XXX comment here.

OK.

> >+++ mozilla-trunk/layout/base/nsGenConList.h	2008-06-19 23:21:02.000000000 +1200
> > struct nsGenConNode : public PRCList {
> >+  nsIContent* mParentContent;
> 
> Please document what this means?  That is, which content node it is in the
> various cases?

Sure.

> >+  nsGenConNode(nsIContent* aParentContent, nsIAtom* aPseudoType,
> >-    NS_ASSERTION(aContentIndex <
> >-                   PRInt32(aPseudoFrame->GetStyleContent()->ContentCount()),
> >-                 "index out of range");
> 
> Why remove this?  I guess because we no longer have the style context
> available?  Is there a sane way we could keep asserting this?

Yes, I can pass in the style context. I'll do that.

> >-    NS_ASSERTION(aContentIndex < 0 ||
> >-                 aPseudoFrame->GetStateBits() & NS_FRAME_GENERATED_CONTENT,
> >-                 "not generated content and not counter change");
> 
> Similar here.  If we did have the gen con content in some namespace, we could
> test that here, I guess.

OK. Should I create a new namespace or use an existing one? If I create a new one I guess I'll need to add it to the magic internal namespaces list.

> >+++ mozilla-trunk/layout/generic/nsHTMLParts.h	2008-06-19 23:21:02.000000000 +1200
> >+// Special Generated Content Frame. It contains text taken from an
> >+// attribute of its *grandparent* content node. 
> 
> s/Frame/Content Node/ or some such?

Er, yeah.

> >+++ mozilla-trunk/layout/reftests/generated-content/table-parts-01-ref.html	2008-06-19 23:21:02.000000000 +1200
> 
> Does this need all the "gen" stuff in <style>?  Seems like it shouldn't.

Right, that should go.

> >+++ mozilla-trunk/layout/reftests/generated-content/table-parts-01.html	2008-06-19 23:21:02.000000000 +1200
> 
> Three comments on this test:
> 
> 1) Please add a test where the parent is display:table, has a single
> display:tabe-cell kid, and has display:table-cell ::before/::after.  Give the
> three cells different heights, stick a background on them or something, and
> make sure they end up all at the same height (since they should all be in a
> single row).  Repeat this with a display:block parent.

OK

> 2) Should have a test of display:table-row generated content in a block. 
> Possibly even a block with an existing display:table-row kid, and make sure
> that the rows affect each other's sizing.

OK

> 3) Test that display:block kids of a display:table-row do in fact all end up
> in that row, and create cells (e.g. have another table-row that has
> table-cells in it so the columns have to line up) but do not affect each
> other's height styling (the row ends up at the height of the tallest cell,
> but the other blocks don't stretch.

OK

> >+++ mozilla-trunk/layout/style/nsRuleNode.cpp	2008-06-19 23:21:02.000000000 +1200
> >-    // XXXbz For example, the calls to WipeContainingBlock in the
> >-    // frame constructor will need to be changedif we allow
> >-    // block-level generated content inside inlines.
> 
> I just reread the WipeContainingBlock part in question.  See the comments
> before the two WipeContainingBlock calls in nsCSSFrameConstructor, which have
> no bearing on reality after this patch.
> 
> With this change, it is in fact possible to have a block ::after inside an
> inline.  The only case when WipeContainingBlock looks at aIsAppend is when
> we're adding kids to the block of an {ib} split.  If we're doing that, and the
> ::after is a block, we certainly don't need to reframe.  So the worst-case
> scenario is that we'll reframe when we don't actually need to.  I guess that's
> OK, but fix the comments accordingly, and add comments inside
> WipeContainingBlock pointing out that aAppend may be incorrect when doing
> ::after stuff?  Or heck, we know at the callsites whether there is a
> appendAfterFrame/parentAfterFrame (depending on the method we're in),  so we
> can just make the boolean exact.  I'd prefer that.

OK, I'll fix that. Great catch.

(In reply to comment #32)
> (From update of attachment 325737 [details] [diff] [review])
> >diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsCounterManager.cpp mozilla-trunk/layout/base/nsCounterManager.cpp
> >diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsCounterManager.h mozilla-trunk/layout/base/nsCounterManager.h
> > struct nsCounterChangeNode : public nsCounterNode {
> >     PRInt32 mChangeValue; // the numeric value of the increment or reset
> > 
> >-    // |aPseudoFrame| is not necessarily a pseudo-element's frame, but
> >-    // since it is for every other subclass of nsGenConNode, we follow
> >-    // the naming convention here.
> >     // |aPropIndex| is the index of the value within the list in the
> >     // 'counter-increment' or 'counter-reset' property.
> >-    nsCounterChangeNode(nsIFrame* aPseudoFrame,
> >+    nsCounterChangeNode(nsIContent* aContent, nsIAtom* aPseudo,
> 
> 
> I'd rather make this comment about |aPseudo| rather than remove it.

OK.

> >diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsGenConList.cpp mozilla-trunk/layout/base/nsGenConList.cpp
> >diff -NrpU12 mozilla-trunk.5034371d2778/layout/base/nsGenConList.h mozilla-trunk/layout/base/nsGenConList.h
> > struct nsGenConNode : public PRCList {
> >-  // The wrapper frame for all of the pseudo-element's content.  This
> >-  // frame generally has useful style data and has the
> >-  // NS_FRAME_GENERATED_CONTENT bit set (so we use it to track removal),
> >-  // but does not necessarily for |nsCounterChangeNode|s.
> >-  nsIFrame* const mPseudoFrame;
> >+  nsIContent* mParentContent;
> >+  nsIAtom*    mPseudoType; // nsGkAtoms::before or nsGkAtoms::after
> 
> The comment you added is not true for counter change nodes; you should instead
> update the comment you removed to reflect what's now going on, but still
> mention counter change nodes.

OK.

> >+  nsGenConNode(nsIContent* aParentContent, nsIAtom* aPseudoType,
> >+               PRInt32 aContentIndex)
> >+    : mParentContent(aParentContent)
> >+    , mPseudoType(aPseudoType)
> >     , mContentIndex(aContentIndex)
> >   {
> >-    NS_ASSERTION(aContentIndex <
> >-                   PRInt32(aPseudoFrame->GetStyleContent()->ContentCount()),
> >-                 "index out of range");
> 
> I'm sad to see this assertion go.

I'll rescue it.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> No, AFAIK we didn't have this issue before the patch because we processed the
> increments and resets when we created the outermost container frame

Then it definitely deserves a followup bug.  That sounds like a reasonably common use case that we want to keep fast.

> No, because it can be the outermost generated content frame, e.g. for ::after {
> float:left; }.

Ah, ok.  Still need the bits on oofContFrame, I would think, in addition to putting them on the placeholder....

> Near the end of nsCSSFrameConstructor::CreateContinuingFrame there's code to
> take care of this.

That sets bits on newFrame, which in this case is the placeholder.  It's not setting those bits on the placeholders out-of-flow, that I can see.

> That comment just needs to be fixed and moved to GetScopeContent

Sounds good.

> The text for nsCSSPseudoElements::before/after is ":before" and ":after"

Hrm...  I suspect it would work, but we'd need to do some code auditing to make sure, and it would be fragile.  Let's stick with the way you did it.  Sad.

> Should I create a new namespace or use an existing one?

This would need a new one, magic as you point out.  It might not be worth the effort just to save this assertion.

Comment 36

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

(In reply to comment #35)
> > No, AFAIK we didn't have this issue before the patch because we processed
> > the increments and resets when we created the outermost container frame
> 
> Then it definitely deserves a followup bug.  That sounds like a reasonably
> common use case that we want to keep fast.

Will do.

> > No, because it can be the outermost generated content frame, e.g. for
> > ::after { float:left; }.
> 
> Ah, ok.  Still need the bits on oofContFrame, I would think, in addition to
> putting them on the placeholder....
> 
> > Near the end of nsCSSFrameConstructor::CreateContinuingFrame there's code to
> > take care of this.
> 
> That sets bits on newFrame, which in this case is the placeholder.  It's not
> setting those bits on the placeholders out-of-flow, that I can see.

NS_FRAME_GENERATED_CONTENT should be set on the first-in-flow out-of-flow frame when it's created, via mAdditionalStateBits. The out-of-flow continuations are created via CreateContinuingFrame which should propagate the bit to those continuations. So I think we're OK. I'll add a comment to clarify this.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Oh, because in the placeholder case we're reusing CreateContinuingFrame to create the continuation for the out-of-flow.  OK, makes sense now.

Comment 38

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

(In reply to comment #34)
> > 3) Test that display:block kids of a display:table-row do in fact all end up
> > in that row, and create cells (e.g. have another table-row that has
> > table-cells in it so the columns have to line up) but do not affect each
> > other's height styling (the row ends up at the height of the tallest cell,
> > but the other blocks don't stretch.

Actually I'm not sure what you mean here. A sequence of display:block children of a display:table-row container all end up in the same cell, even in the absence of generated content. For example:

<div style="display:table-row"><div>Hello</div><div>Kitty</div></div>

Trunk puts "Hello" and "Kitty" on different lines, not the same line.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Hmm....  Indeed.  Nevermind on that, then.

Comment 40

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

Attachment: updated patch

This patch contains all the changes and I think it's ready to land.

Comment 41

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

Pushed dcbcc3d9070c.

Comment 42

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

I eventually had to back this out. Some kind of weird orange happened where there aren't test failures but the tests just stop and the boxes go orange.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Were the builds crashing or something?

Comment 44

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

I don't think so, but it was happening on all platforms so I should be able to reproduce it.

Comment 45

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

It actually seems to have been crashing in layout/generic/crashtests/382396-1.xhtml.

Comment 46

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

The crash seems to be because nsQuoteNode is storing an nsRefPtr<nsStyleContext>. PresShell::Destroy calls nsCSSFrameConstructor::WillDestroyFrameTree and then in nsQuoteNode we crash on releasing the reference and destroying the nsStyleContext; its rulenode seems to have been already deleted (filled with 0xddddddd anyway). Perhaps because nsPresShell::Destroy already called mStyleSet->BeginShutdown? I don't know. It seems unfortunate that the style context can get corrupt even though I'm holding a strong ref to it.

If I emulate what was happening before and store a frame pointer and get the style context via the frame, the problem goes away, because we don't need to release the style context at an inopportune time anymore. It seems odd, though, that the frame tree is still around and full of references to these apparently half-borked style contexts.

I may try to fix this and bug 449538 by dropping the nsGenConNode changes; we can just create anonymous text nodes for counters and quotes in CreateGeneratedContent, but attach a hook so that when the frames for that text are created, *then* we create the actual nsGenConNodes.

Comment 47

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

Attachment: fix v3

OK, here's the new patch along those lines.

This should make 449538 a non-issue, because now the counter node creation order is not changed. I don't see 449541 on my Linux build, hopefully that's just fixed. And of course this fixes the crash and passes reftests.

Basically the nsGenConNode stuff from the last patch has been reverted. There are still changes in that code, to account for the fact that ::before and ::after frames have their own elements now, and especially adding an InitTextFrame method so that nsCSSFrameConstructor can delay complete initialization of the nodes until their text frame(s) have been created. So CreateGeneratedContent creates a counter or quote node, and a DOM text node, and attaches an initialization callback to the DOM text node which will finish initializing the counter or quote node. When we create a textframe for the DOM text node, we activate the initialization callback (and remove it from the DOM node). Note that this requires we create DOM text nodes and textframes for "no-" quotes where we didn't before; I can't see an easy way around this, but it shouldn't matter.

David, Boris is out this week. I know you're on the road but the changes here from the previous iteration are not large and mostly in your code, so I hope this is OK.

Comment 48

[Boris Zbarsky [:bzbarsky]]

That's really weird.  We shouldn't be tearing down the rulenodes until nsStyleContext::Shutdown() is called, which is after frame tree tree teardown.  The style context strong ref thing is certainly unfortunate (and even worse if content is holding those pointers across presshell going away altogether), but it really shouldn't be biting us here!

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 333217 [details] [diff] [review]
fix v3

> I don't know. It
> seems unfortunate that the style context can get corrupt even though I'm
> holding a strong ref to it.

This shouldn't happen unless nsStyleSet::Shutdown has already been
called.  PresShell::Destroy doesn't call that until after all frames
have been destroyed; you should ensure that what you're destroying is
also destroyed before that point.

If that wasn't the case, it might be good to know why it was happening.
Style contexts own their parents, which are immutable.  Rule nodes are
garbage collected, but nsStyleSet::NotifyStyleContextDestroyed does the
mark phase through all style contexts that don't have a parent.


It looks like part of the moving of 374193-1.xhtml to crashtests didn't
get backed out -- make sure when things are relanded that everything gets
relanded properly and the test is enabled.


The name nsCSSFrameConstructor::CreateTextNode might be a little too
general; maybe call it CreateGenConTextNode ?

In nsCounterUseNode::InitTextFrame, could you follow local style and
brace the else with "} else {" all on one line, which I think also makes
it clearer looking at the top of the if that it *has* an else-clause.


r+sr=dbaron.  Sorry for taking so long (in hindsight, a useful interdiff
may well have meant I'd gotten to it during the week).  Boris should
probably take a look as well when he gets back.

Comment 50

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

(In reply to comment #49)
> This shouldn't happen unless nsStyleSet::Shutdown has already been
> called.  PresShell::Destroy doesn't call that until after all frames
> have been destroyed; you should ensure that what you're destroying is
> also destroyed before that point.
> 
> If that wasn't the case, it might be good to know why it was happening.
> Style contexts own their parents, which are immutable.  Rule nodes are
> garbage collected, but nsStyleSet::NotifyStyleContextDestroyed does the
> mark phase through all style contexts that don't have a parent.

nsStyleSet::BeginShutdown calls mRoots.Clear(); would this allow rule nodes to be GCed while we still have style contexts referencing them?

> The name nsCSSFrameConstructor::CreateTextNode might be a little too
> general; maybe call it CreateGenConTextNode ?

Sure OK.

> In nsCounterUseNode::InitTextFrame, could you follow local style and
> brace the else with "} else {" all on one line, which I think also makes
> it clearer looking at the top of the if that it *has* an else-clause.

Sure.

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #50)
> nsStyleSet::BeginShutdown calls mRoots.Clear(); would this allow rule nodes to
> be GCed while we still have style contexts referencing them?

No, since we do the GC in nsStyleSet::NotifyStyleContextDestroyed, which returns early if BeginShutdown has been called (mInShutdown).

Comment 52

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

Relanded with those changes.

Comment 53

[PikeUK]

This may have caused or exposed Bug 451168.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Did we ever figure out why we had dead rulenodes?  That really shouldn't have been happening...

Comment 55

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

No.