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

RESOLVED FIXED in Future

Status

()

enhancement
P3
normal
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: sekundes, Assigned: roc)

Tracking

({css2, testcase})

Trunk
Future
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [css2.1])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

15 years ago
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:
Reporter

Updated

15 years ago
Keywords: css2, testcase

Updated

15 years ago
OS: Windows 2000 → All
Hardware: PC → All
Summary: [css 2.1] support positioned and floated generated content for non-replaced elements → support positioned and floated generated content for non-replaced elements
Priority: -- → P3
Target Milestone: --- → Future
Reporter

Comment 1

15 years ago
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.
No, it's not.  While that code would need to be changed, other code needs to be
changed as well (mostly in nsCSSFrameConstructor).
Reporter

Updated

15 years ago
Whiteboard: [css2.1]

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 251850 has been marked as a duplicate of this bug. ***
*** Bug 268062 has been marked as a duplicate of this bug. ***
Summary: support positioned and floated generated content for non-replaced elements → [GC] support positioned and floated generated content for non-replaced elements
*** Bug 280459 has been marked as a duplicate of this bug. ***
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....
Note also that it would make it easier to fix bug 159799.
Blocks: 159799
*** Bug 284982 has been marked as a duplicate of this bug. ***

Comment 9

14 years ago
Posted file testcase

Comment 10

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

Comment 11

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

Comment 12

14 years ago
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 14

14 years ago
(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).
This discussion is irrelevant.  See comment 6.

Comment 16

13 years ago
*** Bug 332487 has been marked as a duplicate of this bug. ***
*** Bug 332488 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Duplicate of this bug: 372509
Assignee: dbaron → nobody
QA Contact: ian → style-system

Updated

12 years ago
Duplicate of this bug: 398044

Updated

12 years ago
Duplicate of this bug: 410417
Duplicate of this bug: 412377

Comment 22

11 years ago
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

11 years ago
(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).
This bug is tested in acid3.
(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.
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.
Assignee: nobody → roc
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.
Posted patch fix (obsolete) — Splinter Review
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.)
Attachment #325737 - Flags: superreview?(bzbarsky)
Attachment #325737 - Flags: review?(bzbarsky)
roc, it'll probably take me a week and a bit to get to this; moving this Sunday...
There's no rush at all.
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.
Attachment #325737 - Flags: superreview?(bzbarsky)
Attachment #325737 - Flags: superreview+
Attachment #325737 - Flags: review?(dbaron)
Attachment #325737 - Flags: review?(bzbarsky)
Attachment #325737 - Flags: review+
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 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...)
Attachment #325737 - Flags: review?(dbaron) → review+
(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.
> 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.

(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.
Oh, because in the placeholder case we're reusing CreateContinuingFrame to create the continuation for the out-of-flow.  OK, makes sense now.
(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.
Hmm....  Indeed.  Nevermind on that, then.
Posted patch updated patch (obsolete) — Splinter Review
This patch contains all the changes and I think it's ready to land.
Attachment #325737 - Attachment is obsolete: true
Attachment #332613 - Flags: superreview+
Attachment #332613 - Flags: review+
Pushed dcbcc3d9070c.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Were the builds crashing or something?
I don't think so, but it was happening on all platforms so I should be able to reproduce it.
It actually seems to have been crashing in layout/generic/crashtests/382396-1.xhtml.
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.
Blocks: 237119
No longer depends on: 237119
Posted patch fix v3Splinter Review
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.
Attachment #332613 - Attachment is obsolete: true
Attachment #333217 - Flags: superreview?(dbaron)
Attachment #333217 - Flags: review?(dbaron)
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 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.
Attachment #333217 - Flags: superreview?(dbaron)
Attachment #333217 - Flags: superreview+
Attachment #333217 - Flags: review?(dbaron)
Attachment #333217 - Flags: review+
(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.
(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).
Relanded with those changes.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 53

11 years ago
This may have caused or exposed Bug 451168.
Depends on: 451170
Depends on: 451355
Did we ever figure out why we had dead rulenodes?  That really shouldn't have been happening...

Updated

11 years ago
Duplicate of this bug: 452382

Updated

11 years ago
Depends on: 452964
Depends on: 456041

Updated

11 years ago
Depends on: 460012

Updated

10 years ago
Depends on: 503813

Updated

10 years ago
Duplicate of this bug: 530920
Depends on: 748361
You need to log in before you can comment on or make changes to this bug.