Closed Bug 3247 Opened 26 years ago Closed 20 years ago

counters in css-generated content [GC]

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: ian, Assigned: dbaron)

References

()

Details

(Keywords: css2, testcase, Whiteboard: [Hixie-PF]Note comment 108; file new bugs for remaining issues)

Attachments

(1 file, 24 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
You do not support the :before and :after pseudo-elements, which are part of CSS2 generated content. This is defined in the spec: http://www.w3.org/TR/REC-CSS2/generate.html Generated content isa prerequisite to correctly implementing Q (see bug #1061) in the ua stylesheet, and would also be useful to QA in testing attribute support (in addition, of course, to being useful in its own right on the web!).
Assignee: peterl → troy
Component: Style System → Layout
The content properties are now in the style contexts. Assign to rickg for the counter support when you're done...
Status: NEW → ASSIGNED
Thanks Peter
Target Milestone: M5
Target Milestone: M5 → M7
Assignee: troy → rickg
Status: ASSIGNED → NEW
Rick, assigning to you for counter support. I created a new bug (marked REMIND) concerning nested quotes (currently not implemented)
Status: NEW → RESOLVED
Closed: 26 years ago
Resolution: --- → REMIND
Status: RESOLVED → VERIFIED
Priority: P2 → P3
Summary: :before and :after pseudo-elements → {css2} counters in generated content
Nested quotes support is currently waiting in bug 1061. Note: A discussion on how to implement display:list-item (a css1 task) is currently playing itself out in bug 4522. That bug would be easy to solve if generated counter was implemented.
*** Bug 9840 has been marked as a duplicate of this bug. ***
When this is being implemented, don't forget that: # When the 'display' property has the value 'marker' for content generated by # an element with 'display: list-item', a marker box generated for ':before' # replaces the normal list item marker. -- http://www.w3.org/TR/REC-CSS2/generate.html#markers Also, if counter() and counters() are not implemented, then the entire 'content' declaration in which they occur should be ignored. That is so that authors can easily work around this lack of feature. NOTE! THIS IS CURRENTLY A BUG! i.e., currently, the following: SPAN:before { content: "works"; } SPAN:before { content: counter(x) "bug!"; } ...displays "bug!". It should not, since counters are not supported. It should display "works". This is due to the forward-compatability parsing rules: if a feature is not supported, then the declaration should be ignored. Rick, if we are not going to support CSS2 counters, then I suggest we file a bug against Peter so that he ignores 'content' declarations with 'counter's.
Blocks: 15174
Target Milestone: M7
This last issue is now covered by bug 15174. I am marking that bug dependent on this one, but technically it is an either|or situation.
*** Bug 14948 has been marked as a duplicate of this bug. ***
Keywords: css2
Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and {css-moz} radars should now be considered deprecated in favour of keywords. I am *really* sorry about the spam...
Reopening and moving to Future.
Status: VERIFIED → REOPENED
OS: Windows 98 → All
QA Contact: chrisd → py8ieh=bugzilla
Hardware: PC → All
Resolution: REMIND → ---
Summary: {css2} counters in generated content → counters in generated content
Whiteboard: hit during nsbeta2 standards compliance testing
Target Milestone: --- → Future
Summary: counters in generated content → counters in generated content [GC]
*** Bug 51393 has been marked as a duplicate of this bug. ***
*** Bug 57393 has been marked as a duplicate of this bug. ***
add pierre and erik to the cc list.
*** Bug 70191 has been marked as a duplicate of this bug. ***
Whiteboard: hit during nsbeta2 standards compliance testing → [Hixie-PF] hit during nsbeta2 standards compliance testing
*** Bug 81381 has been marked as a duplicate of this bug. ***
*** Bug 94182 has been marked as a duplicate of this bug. ***
Fixing summary to show up on searches for "css counter".
Summary: counters in generated content [GC] → counters in css-generated content [GC]
any progress with this bug?
not while it's assigned to nobody... reassigning to default owner.
Assignee: rickg → attinasi
Status: REOPENED → NEW
QA Contact: ian → petersen
Oh, great, after 7 duplicates of this bug, over 3 years gone by and CSS3 comming, still nobody who cares for counters? Maybe it's a provocation since Opera 6 supports it ...
#21 > Oh, great, after 7 duplicates of this bug, over 3 years gone by and CSS3 > comming, still nobody who cares for counters? It's not like nobody cares about it. I do, but can't fix it as I'm not a coder. > Maybe it's a provocation since Opera 6 supports it ... Ugh...
Speaking of CSS3, counters are likely to work quite differently in CSS3 than CSS2 since the counter model in CSS2 has serious problems. (Is it really implemented *correctly* in Opera?)
#23 > Speaking of CSS3, counters are likely to work quite differently in CSS3 than > CSS2 since the counter model in CSS2 has serious problems. Like? (Out of curiosity) > (Is it really implemented *correctly* in Opera?) http://www.xs4all.nl/~ppk/css2tests/counter.html says yes. I didn't test it extensively though.
Actually, that page appears to be correct in stating that Opera 5 implements counters.
Yeah, Opera supports it including nested counters. Don't know how far and if correctly in really hard environments, but as far I could see good enough for normal sites. My first comment should thought of a wake-up to this old bug and as he mailed me someone is really working on counters.
Attached file Testcases for counter(s) (obsolete) (deleted) —
4 Testcases with hyperlinks "should be" pics.
This bug has been open for over 3 years now. It would be nice to see a foreseeable milestone set for this bug. -- should this bug not fall under the Style System Component?
-> Style System
Component: Layout → Style System
Layout is the correct component. Please leave it there.
Component: Style System → Layout
Named counters have been removed in CSS 2.1: * http://www.w3.org/TR/CSS21/changes.html#q81 * http://www.w3.org/TR/CSS21/generate.html Does this make the bug FIXED, seeing as the original problem was to implement :before and :after?
no, they are still valid for CSS 2.0 and will be for CSS 3, so it only lowers the priority for most of the missing CSS properties (except outline)
*** Bug 189465 has been marked as a duplicate of this bug. ***
*** Bug 190268 has been marked as a duplicate of this bug. ***
Comment #32 says that named counters were removed in the proposal for CSS2.1, but reading the links provided, I see no such evidence. The word "counter" is not mentioned in the changes file at the link provided, and (to my untrained eye) the section on named counters looks to be identical in the CSS2 rec and proposed CSS2.1 spec. http://www.w3.org/TR/CSS21/generate.html#counters http://www.w3.org/TR/REC-CSS2/generate.html#counters Thus, as far as I can tell, this is just as much of a bug as back in 1999. If I'm reading the specs wrong, perhaps someone (Marcus?) could clarify comment #32 to avoid future confusion? Or perhaps since CSS2.1 is still a working draft, things changed in the interim that comment was made?
I should have originally linked to the actual draft file but, yes, it seems that they've added counters back into the latest draft (released on 28th January 2003). I think I remember reading that they added it back in because Opera could support it. Previous generated content section: http://www.w3.org/TR/2002/WD-CSS21-20020802/generate.html Current generated content section: http://www.w3.org/TR/2003/WD-CSS21-20030128/generate.html Strangely, unless I'm missing something, there doesn't seem to be any mention of this in the changes: http://www.w3.org/TR/2003/WD-CSS21-20030128/changes.html#q17
Counters will be in CSS3 regardless. CSS2.1 does not affect whetehr Mozilla will implement something or not in any way.
*** Bug 193927 has been marked as a duplicate of this bug. ***
So will the counter support be fixed anytime soon? This *is* an important feature for sites (probably mostly corporate intranets) that deal with a lot of outline-numbered documents; it would make cut-and-paste between documents (or includes of common subdocuments) a he!! of a lot easier to deal with. Opera 7 does it quite nicely; this has been in the CSS2 spec for over four years now...
Fixing bugs with existing features, making our performance better, making us take less disk space and memory, and making us crash less are all considered more important right now. Having said that, we welcome patches, if you want to implement it.
.
Assignee: attinasi → other
Priority: P3 → --
QA Contact: cpetersen0953 → ian
Target Milestone: Future → ---
I think HTML 4.01 Strict, XHTML 1.0 Strict and XHTML 1.1+ would benefit from this bug being fixed. Consider: <ol type="number" start="3"> <li>Three</li> <li>Four</li> </ol> The start attribute in HTML 4.01 is deprecated. Thus, the CSS counter-reset property is apparently the only way for valid HTML 4.01 Strict documents to have numbered lists that don't start at 1.
Priority: -- → P3
Target Milestone: --- → Future
*** Bug 213703 has been marked as a duplicate of this bug. ***
*** Bug 214871 has been marked as a duplicate of this bug. ***
->style system, for lack of a better component for what probably centers around frame construction
Assignee: other → dbaron
Component: Layout → Style System
Is there a separate bug for: p::before{ content:url(data.txt); } ? Simple tests: http://www.annevankesteren.nl/test/css/p/content/ (only the second 'works' in Mozilla, the first depends on another bug/CSS3)
comment 47 is unrelated to this bug. comment 47 is about the css3 extensions to the content property, but the assertions made in the are probably not valid anyway.
Why is that CSS3 (please don't start about the ::, with : it doesn't work either)? <http://www.w3.org/TR/CSS21/generate.html#propdef-content> ><uri> > The value is a URI that designates an external resource. If a user agent > cannot support the resource because of the media types it supports, it must > ignore the resource. I think Mozilla supports .txt so it should so the file. I just wanted to know if this is the same bug, 'cause the third test case from comment 1 demonstrates this with a .html file.
This bug is about counters. Please don't clutter it up with unrelated comments. If counters are implemented, this bug will just be marked fixed, and the comments will be ignored.
Blocks: majorbugs
*** Bug 230681 has been marked as a duplicate of this bug. ***
Comment on attachment 138874 [details] Advanced counter example, using the counters() function sorry, the same one uploaded twice
Attachment #138874 - Attachment is obsolete: true
I think an approach similar to that of bug 24861 could be taken here (with use of one list per named counter to keep track of the current value or something). David, does that seem like a viable approach to you?
Blocks: 3246
Depends on: nested-quotes
Similar, but not identical. Counters are easier since they are limited to a subtree, but harder since there are many of them. We definitely want to exploit the fact that they're limited to a subtree for finding the right counter to use, but yes, each counter could probably be stored in a similar way to the way that patch stores quotes (and hopefully even share some code).
Status: NEW → ASSIGNED
Attached patch some work in progress (obsolete) (deleted) — Splinter Review
This improves the parsing code and makes a bunch of other changes such that "all" that is left is the actual frame construction code for the counters and the integration / replacement of the existing block frame marker code. Then again, the implementation of markers and the conversion of list items to use them could wait until a second patch.
Blocks: 241719
Attached patch work in progress (obsolete) (deleted) — Splinter Review
This adds some merging of the counters and quotes code that may or may not be appropriate. I want to get some spec clarifications on a few issues before proceeding (the current spec is quite vague, and what Opera implements doesn't quite make sense). There's also a big "XXX XXX" comment that probably means I'll need to undo the code merging.
Attachment #146367 - Attachment is obsolete: true
Attached patch work in progress (obsolete) (deleted) — Splinter Review
This is what I have in my tree now. A bunch of the new stuff is just rough sketches and probably isn't right. But I'm not planning to work on it more until the spec is (a) clear and (b) sensible regarding interaction of counter-{increment,reset} on an element and on its :before pseudo-element, and likewise regarding the scoping of the counters for the same distinction (element vs. its :before).
Attachment #150755 - Attachment is obsolete: true
SInce the clarification of the spec is a long way to go, could you check in the current implementation as -moz-counter-* stuff?
Anything new ? It would be great to fix this bug for FireFox 1.0. ;-)
(In reply to comment #61) > Anything new ? It would be great to fix this bug for FireFox 1.0. ;-) Oh, come on, it's not even been open for six years yet. You can't expect David to work this fast. :)
Whiteboard: [Hixie-PF] hit during nsbeta2 standards compliance testing → [Hixie-PF]SEE COMMENT 59 FOR STATUS ;hit during nsbeta2 standards compliance testing
After reseaving a private e-mail, I would like to ask people not to take my previous comment seriously. This was my way to say "hey don't forget this bug". More over, I admit that I read too quickly the comment saying : "I want to get some spec clarifications on a few issues before proceeding (the current spec is quite vague, and what Opera implements doesn't quite make sense).".
Attached patch work in progress (obsolete) (deleted) — Splinter Review
Attachment #153572 - Attachment is obsolete: true
Attached patch work in progress (obsolete) (deleted) — Splinter Review
Attachment #168745 - Attachment is obsolete: true
Attached patch work in progress (obsolete) (deleted) — Splinter Review
Attachment #171220 - Attachment is obsolete: true
Attached patch work in progress (obsolete) (deleted) — Splinter Review
Attachment #171281 - Attachment is obsolete: true
The problem with this patch (other than not quite working yet for the static case) is that it uses the wrong data structures. It would be much easier to handle the dynamic cases if I use a single linked list for each counter name, rather than one per scope. The scopes should just be elements in the list (probably just "start scope", not push/pop). Each list element should have a pointer to its appropriate "start scope" element (which has the parent number).
Attached patch work in progress (obsolete) (deleted) — Splinter Review
This approach seems better. This patch passes all but one of my static testcases (one that Opera also fails, but differently). I need to reexamine that testcase (t1204-counter-scope-00-c.xht) and work on the dynamic change handling, which should be relatively easy with this approach.
Attachment #171302 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
The dynamic change handling is now implemented, although completely untested.
Attachment #171727 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #171784 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #171824 - Attachment is obsolete: true
I need to check that this code behaves reasonably when pseudo-elements other than :before or :after are involved.
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #171925 - Attachment is obsolete: true
Comment on attachment 177060 [details] [diff] [review] patch There are still some open issues before the WG (well, one I want to revisit, at least), but the necessary change to the patch is tiny, so I may as well request review.
Attachment #177060 - Flags: superreview?(bzbarsky)
Attachment #177060 - Flags: review?(bzbarsky)
Blocks: 286303
Target Milestone: Future → mozilla1.8beta2
Blocks: 280443
Whiteboard: [Hixie-PF]SEE COMMENT 59 FOR STATUS ;hit during nsbeta2 standards compliance testing → [patch][Hixie-PF]SEE COMMENT 59 FOR STATUS ;hit during nsbeta2 standards compliance testing
Attached patch patch (obsolete) (deleted) — Splinter Review
Merged to trunk, with a bug I noticed (accidentally removed a |break|) fixed as well.
Attachment #177060 - Attachment is obsolete: true
Attachment #177977 - Flags: superreview?(bzbarsky)
Attachment #177977 - Flags: review?(bzbarsky)
Attachment #177060 - Attachment is obsolete: false
Attachment #177060 - Flags: superreview?(bzbarsky)
Attachment #177060 - Flags: review?(bzbarsky)
Attachment #177060 - Attachment is obsolete: true
With Mozilla suite being end-of-lined, should this bug be moved to Firefox?
No. This is a "bug" in the Mozilla Platform (the Core product). Fixing this bug will make this feature be supported on all products based on the Mozilla Platform. Firefox is one such product.
(In reply to comment #78) > No. This is a "bug" in the Mozilla Platform (the Core product). Fixing this bug > will make this feature be supported on all products based on the Mozilla > Platform. Firefox is one such product. Thanks for the clarification, Anne. I was making my assumption based on the "Target Milestone" field, but from now on I'll know to pay more attention to "Product" instead.
Blocks: 282569
Attached patch patch (obsolete) (deleted) — Splinter Review
This cleans up some things and changes the behavior a little -- I found some problems with what the working group agreed to. I've updated my tests as well.
Attachment #177977 - Attachment is obsolete: true
Attachment #178659 - Flags: superreview?(bzbarsky)
Attachment #178659 - Flags: review?(bzbarsky)
Attachment #177977 - Flags: superreview?(bzbarsky)
Attachment #177977 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) (deleted) — Splinter Review
This fixes 'none', 'disc', 'circle', and 'square' when used with counters to at least do something reasonable (although it uses glyphs instead of the special code we have for list bullets).
Attachment #178659 - Attachment is obsolete: true
Attachment #178755 - Flags: superreview?(bzbarsky)
Attachment #178755 - Flags: review?(bzbarsky)
Attachment #178659 - Flags: superreview?(bzbarsky)
Attachment #178659 - Flags: review?(bzbarsky)
Comment on attachment 178755 [details] [diff] [review] patch > PresShell::NotifyDestroyingFrame(nsIFrame* aFrame) > { >+ // XXX Should this be inside mIgnoreFrameDestruction test? >+ mFrameConstructor->NotifyDestroyingFrame(aFrame); >+ Oh, and I answered my own question here (and moved it inside). The answer is yes since the only thing that sets it is nsFrameManager::Destroy, and right before we call that we call mFrameConstructor->WillDestroyFrameTree, which clears the counter and quote lists.
Attached patch patch (obsolete) (deleted) — Splinter Review
Merged to trunk, plus what I mentioned in comment 82.
Attachment #178755 - Attachment is obsolete: true
Attachment #179074 - Flags: superreview?(bzbarsky)
Attachment #179074 - Flags: review?(bzbarsky)
Attachment #178755 - Flags: superreview?(bzbarsky)
Attachment #178755 - Flags: review?(bzbarsky)
Although I realized I should probably rename nsCSSValue::List to nsCSSValue::Array.
Attached patch patch (obsolete) (deleted) — Splinter Review
This fixes a bunch of bugs in the nsCSSValue and related changes, does the renaming I mentioned in the previous comment, and adds computed value support too.
Attachment #179074 - Attachment is obsolete: true
Attachment #179089 - Flags: superreview?(bzbarsky)
Attachment #179089 - Flags: review?(bzbarsky)
Attachment #179074 - Flags: superreview?(bzbarsky)
Attachment #179074 - Flags: review?(bzbarsky)
I realized serialization of counters() isn't quite right -- I should probably store the params in the order specified so they don't get reordered, and just deal with that in the code that uses the nsCSSValue::Array.
Comment on attachment 179089 [details] [diff] [review] patch >Index: layout/base/nsCSSFrameConstructor.cpp >@@ -6818,24 +6864,28 @@ nsCSSFrameConstructor::InitAndRestoreFra >+ if (mCounterManager.AddCounterResetsAndIncrements(aNewFrame)) { Should probably assert that aNewFrame is a first-in-flow here. Otherwise things will get a bit confused, I think. Also, there are a few places in the frame constructor that don't call InitAndRestoreFrame but may want to affect counters (the replacement wrapper frame for <img> elements and letter frames, to be precise)... >Index: layout/base/nsCSSFrameConstructor.h >+ void NotifyDestroyingFrame(nsIFrame* aFrame); Want to add a comment here about when this method is to be called (for all frame destructions happening before Destroy(), basically)? >Index: layout/base/nsCounterManager.cpp >+#include "nsCounterManager.h" >+// The text that should be displayed for this quote. >+void >+nsCounterUseNode::GetText(nsString& aResult) s/quote/counter/ ? >+ for (PRInt32 i = stack.Count() - 1;; --i) { >+ nsCounterNode *n = NS_STATIC_CAST(nsCounterNode*, stack[i]); >+ stack.RemoveElementAt(i); Why bother removing it? The array will go out of scope and go away when we return anyway... >+nsCounterList::SetScope(nsCounterNode *aNode) I'm having a hard time following the logic in this method.... Could you maybe explain the basic idea of the algorithm up front before diving into the code? >+nsCounterManager::AddResetOrIncrement(nsIFrame *aFrame, PRInt32 aIndex, >+ counterList->Insert(node); >+ if (!counterList->IsLast(node)) { >+ return PR_TRUE; >+ } >+ node->Calc(counterList); I guess we don't need to call Calc() in the !IsLast() case because caller should recalc the whole list? If so, worth documenting, I think. >+nsCounterManager::CounterListFor(const nsSubstring& aCounterName) >+ counterList = new nsCounterList; |new nsCounterList()| is probably clearer (add the parens). >Index: layout/base/nsCounterManager.h >+struct nsCounterNode : public nsGenConNode { >+ enum Type { RESET, INCREMENT, USE }; Add a comment saying that RESET and INCREMENT correspond to counter-reset and counter-increment respectively and that USE corresponds to a counter() or counters()? > + nsCounterNode(nsIFrame* aPseudoFrame, PRInt32 aContentIndex, Type aType) What's aContentIndex here? And why aPseudoFrame? Those names make sense for USE nodes, but not so much for INCREMENT and RESET nodes..... Some documentation on what those args mean would be good. >+ // mScopeStart points to the node (usually a RESET, but not in the >+ // case of an implied 'counter-reset' that created the scope for >+ // this element (for a RESET, its outer scope). Missing ')' after "'counter-reset'"? What does "outer scope" mean, exactly? It's not defined in the spec that I can see, and it's not really defined in the code.... Worth explaining in more detail, perhaps. I'm guessing this means that for RESET nodes this points to the scope right outside the scope the RESET creates. >+ // mScopePrev points to the previous node that is in the same scope, >+ // or for a RESET, the previous node in the scope outside of the >+ // reset. >+ >+ // May be null for all types, but only when mScopeStart is also >+ // null. Because otherwise it could always point to the mScopeStart RESET node? May be worth saying so explicitly. >+struct nsCounterUseNode : public nsCounterNode { >+ nsCounterUseNode(nsCSSValue::Array* aCounterStyle, nsIFrame* aPseudoFrame, >+ PRUint32 aContentIndex, PRBool aAllCounters) Again, please document the args? In particular what is aCounterStyle expected to contain, exactly? Alternately, and perhaps better, document mCounterStyle and just say here that aCounterStyle should be what we store in mCounterStyle. >+ // The text that should be displayed for this quote. >+ void GetText(nsString& aResult); s/quote/counter/ >+ nsCounterChangeNode(nsIFrame* aPseudoFrame, It's not really necessarily a pseudoframe, is it? >+class nsCounterList : public nsGenConList { >+ void SetScope(nsCounterNode *aNode); Document what that does? >+ void RecalcAll(); And this. >+/** >+ * The counter tree can be used to look up >+ */ Look up what? >+ PRBool AddResetOrIncrement(nsIFrame *aFrame, PRInt32 aIndex, >+ const nsStyleCounterData *aCounterData, >+ nsCounterNode::Type aType); What's aIndex here? What does this function do? >Index: layout/base/nsGenConList.cpp >+nsGenConList::NodeAfter(const nsGenConNode* aNode1, const nsGenConNode* aNode2) >+ if (pseudoType1 == 0 || pseudoType2 == 0) { >+ if (content1 == content2) { >+ NS_ASSERTION(pseudoType1 != pseudoType2, "identical"); >+ return pseudoType2 == 0; >+ } >+ // We want to treat an element as coming before its :before (preorder >+ // traversal), so treating both as :before now works. >+ pseudoType1 = -1; >+ pseudoType2 = -1; I'm not sure I follow. Say we have the following markup: <div><span /></div> and content1 is the div, content2 is the span, with pseudoType2 == 0. Then we get into this case. But the return value should clearly depend on whether we're talking about the ::before or ::after of the <div>. At the same time, this code will call DoCompareTreePosition with the same arguments for both of these cases. That can't be right. >Index: 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; Maybe mFrame, then? Since it's not always a pseudo-frame? And document what it means in the struct decls for the specific node types? >+ // Index within the list of things specified by the 'content' property, >+ // which is needed to do 'content: open-quote open-quote' correctly. And to do counters right too, no? Also, for counter increment/reset this isn't the index inside 'content' at all, is it? Perhaps just call it mIndex and document what it means for different node types in the struct decls for those types? >+ NS_ASSERTION(aContentIndex < >+ PRInt32(aPseudoFrame->GetStyleContent()->ContentCount()), >+ "index out of range"); >+ // We allow negative values of mContentIndex for 'counter-reset' and >+ // 'counter-increment'. And this assert is meaningless for those types of nodes, right? Perhaps the assert should be moved down into the relevant node type constructors? >+ NS_ASSERTION(aContentIndex < 0 || >+ aPseudoFrame->GetStyleContext()->GetPseudoType() == >+ nsCSSPseudoElements::before || >+ aPseudoFrame->GetStyleContext()->GetPseudoType() == >+ nsCSSPseudoElements::after, >+ "not :before/:after generated content and not counter use"); "Counter change", you mean? Counter use _does_ require generated content... >+ NS_ASSERTION(aContentIndex < 0 || >+ aPseudoFrame->GetStateBits() & NS_FRAME_GENERATED_CONTENT, >+ "not generated content and not counter use"); Again, "counter change". >+ virtual ~nsGenConNode() {} // XXX Avoid, perhaps? Don't really see how we can, since we have subclasses with members that get deleted in destructors and we want to delete them through the nsGenConNode pointer.... >Index: layout/generic/nsBulletFrame.cpp >+nsBulletFrame::GetListItemText(const nsStyleList& aListStyle, >+ PRBool success = >+ AppendCounterText(aListStyle.mListStyleType, mOrdinal, result); Before that, assert that aListStyle.mListStyleType isn't one of DISC/CIRCLE/SQUARE? For those, the boolean that function returns is undefined... (perhaps it would be better to define the boolean too, to prevent compiler warnings?) >Index: layout/style/nsCSSDeclaration.cpp Like you said, the serialization needs to be fixed up to deal with the original array order (mostly a note to myself so I remember to check it in new patch versions). The code in nsCounterUseNode::GetText will need fixing too. >Index: layout/style/nsCSSParser.cpp >+PRBool CSSParserImpl::GetNonCloseParenToken(nsresult& aErrorCode, PRBool aSkipWS) >+{ >+ if (!GetToken(aErrorCode, aSkipWS)) >+ return PR_FALSE; Do we want to report the EOF error via error reporting here? I think we do... And perhaps an UNEXPECTED_TOKEN error if the token is a paren? >-#ifdef ENABLE_COUNTERS > if (ParseCounter(aErrorCode, aValue)) { > return PR_TRUE; > } >-#endif > return PR_FALSE; How about just: return ParseCounter(aErrorCode, aValue); ? Seems clearer to me..... > PRBool CSSParserImpl::ParseCounter(nsresult& aErrorCode, nsCSSValue& aValue) >+ if (!GetNonCloseParenToken(aErrorCode, PR_TRUE) || >+ eCSSToken_Ident != mToken.mType) { >+ SkipUntil(aErrorCode, ')'); I think we want to REPORT_UNEXPECTED_TOKEN here for the case when GetNonCloseParenToken succeeds but the resulting token is not an ident. >+ // get mandatory string Worth documenting that this is the separator string from the spec? >+ if (!ExpectSymbol(aErrorCode, ',', PR_TRUE) || >+ !(GetNonCloseParenToken(aErrorCode, PR_TRUE) && >+ eCSSToken_String == mToken.mType)) { >+ SkipUntil(aErrorCode, ')'); Again, worth having some error reporting here.... >+ if (!success) { >+ SkipUntil(aErrorCode, ')'); Again, error reporting. >+ val->Item(1).SetIntValue(type, eCSSUnit_Enumerated); And note to self that this should be fixed up to deal with serialization. >+ if (!ExpectSymbol(aErrorCode, ')', PR_TRUE)) { >+ SkipUntil(aErrorCode, ')'); And again, error reporting. > PRBool CSSParserImpl::ParseCounterData(nsresult& aErrorCode, >+ if (ident->LowerCaseEqualsLiteral(sv->str)) { >+ if (ExpectEndProperty(aErrorCode, PR_TRUE)) { ... >+ } >+ return PR_FALSE; Does this need error reporting? >+ if (!GetToken(aErrorCode, PR_TRUE) || mToken.mType != eCSSToken_Ident) { >+ break; Error reporting? >+ nsCSSCounterData *data = *next = new nsCSSCounterData(); I can't quite figure out which constructor gets used for data->mCounter and data->mValue when you do this.... Is it ok that you're not really initializing data->mValue in some cases? Would it make more sense to explicitly init it to 0? >Index: layout/style/nsCSSValue.cpp >+nsCSSValue::nsCSSValue(nsCSSValue::Array* aValue, nsCSSUnit aUnit) >+ : mUnit(aUnit) Asssert in the method that aUnit is valid array unit? > nsCSSValue::nsCSSValue(const nsCSSValue& aCopy) > : mUnit(aCopy.mUnit) Want to factor out the shared code from here and operator= so we don't have to keep changing both places every time? Separate bug fine for this if you want. >+void nsCSSValue::SetArrayValue(nsCSSValue::Array* aValue, nsCSSUnit aUnit) >+{ >+ Reset(); >+ mUnit = aUnit; Assert that aUnit is valid array unit? >Index: layout/style/nsCSSValue.h >+ Array* GetArrayValue() const >+ { >+ NS_ASSERTION(eCSSUnit_Array <= mUnit && mUnit <= eCSSUnit_Counters, >+ "not a list value"); s/list/array/ >Index: layout/style/nsComputedDOMStyle.cpp >+nsComputedDOMStyle::GetCounterIncrement(nsIFrame *aFrame, >+ const nsStyleContent *content = nsnull; >+ GetStyleData(eStyleStruct_Content, (const nsStyleStruct*&)content, aFrame); >+ >+ if (content->CounterIncrementCount() == 0) { Null-check |content|? You do this later, but you'd crash here if it were null... >+nsComputedDOMStyle::GetCounterReset(nsIFrame *aFrame, Same issue here. >Index: layout/style/nsRuleNode.cpp > if (NS_SUCCEEDED(content->AllocateCounterIncrements(count))) { > PRInt32 increment; That's unused now, no? > ourIncrement = contentData.mCounterIncrement; > PRInt32 increment; As is this (you moved it into the while() loop there). > if (NS_SUCCEEDED(content->AllocateCounterResets(count))) { > PRInt32 reset; Again, unused. > ourReset = contentData.mCounterReset; > PRInt32 reset; And here. Looks good other than that.
(In reply to comment #87) > Should probably assert that aNewFrame is a first-in-flow here. Otherwise > things will get a bit confused, I think. Actually, it should check. > Also, there are a few places in the frame constructor that don't call > InitAndRestoreFrame but may want to affect counters (the replacement wrapper > frame for <img> elements and letter frames, to be precise)... Not calling InitAndRestoreFrame is a bug, and fixing those bugs shouldn't be in the scope of this patch. > >+/** > >+ * The counter tree can be used to look up > >+ */ > > Look up what? Oops, the counter tree doesn't exist anymore, and hasn't since comment 69.
(In reply to comment #87) > >+PRBool CSSParserImpl::GetNonCloseParenToken(nsresult& aErrorCode, PRBool aSkipWS) > Do we want to report the EOF error via error reporting here? I think we do... > And perhaps an UNEXPECTED_TOKEN error if the token is a paren? No, it should be the callers job, and I really don't want to bloat the property value parsers with error reporting anyway except in cases of common errors. Likewise on your other comments suggesting error reporting.
(In reply to comment #87) > >+ nsCSSCounterData *data = *next = new nsCSSCounterData(); > > I can't quite figure out which constructor gets used for data->mCounter and > data->mValue when you do this.... Is it ok that you're not really initializing > data->mValue in some cases? Would it make more sense to explicitly init it to > 0? They're nsCSSValue objects, so they have their own constructor (init to eCSSUnit_Null), so it's fine. Init to zero or one would be loss of information for serialization, and would also require knowing in this code which of 'counter-reset' or 'counter-increment' is being parsed.
Attached patch patch (obsolete) (deleted) — Splinter Review
Should address bzbarsky's comments. Passes my tests.
Attachment #179089 - Attachment is obsolete: true
Attachment #179253 - Flags: superreview?(bzbarsky)
Attachment #179253 - Flags: review?(bzbarsky)
> fixing those bugs shouldn't be in the scope of this patch. Agreed. Should file them, though. > I really don't want to bloat the property value parsers with error reporting OK, fair enough. Looking at updated patch now.
Attachment #179089 - Flags: superreview?(bzbarsky)
Attachment #179089 - Flags: review?(bzbarsky)
>Index: layout/base/nsCounterManager.cpp >+nsCounterList::SetScope(nsCounterNode *aNode) >+ // we're not in the same scope that it is, then it's too deep in the >+ // frame tree, so we walk up parent scopes until we find something >+ // appropriate. I think this could use the following additional comment (feel free to fix wording as desired, of course): ---------- The scope created by a RESET or implied-reset node is defined to contain the following content nodes and their descendants: 1) The content node associated to the RESET 2) All following siblings of that content node up to, but not including, the next RESET for the same counter. Therefore, given a non-RESET aNode and a otherNode that preceds aNode in content order, aNode will be in the same scope as otherNode if some ancestor (not necessarily proper) of the content of aNode is either the content of otherNode or a following sibling of the content of otherNode. This would mean that this ancestor's parent is the same as the parent of the content of otherNode. So it's enough to check that the parent of the content of aNode is a descendant (not necessarily proper) of the parent of the content of otherNode. Note that for pseudo-elements the content node pointed to by mPseudoFrame is already the parent of the pseudo-element. ---------- For RESET nodes things seem more complicated, though... Consider the following markup: <p style="counter-reset: mycounter" /> <div><span style="counter-reset: mycounter" /></div> and let's trace through this function for the second RESET node. nodeContent will end up being the <span>, since aNode is a RESET node. |start| will be the previous RESET node, and startContent will end up being the <p>, again because aNode is a RESET node. But the <span> is not a descendant of the <p>, so we'll end up with a null mScopeStart for the second RESET node, which is wrong, as I understand. The following case also has a similar issue, but may need a bit more care to get right: parent::before { conter-reset: mycounter; } descendant { counter-reset: mycounter; } <parent><child><descendant /></child></parent> So maybe what we want to do for RESET nodes is add the following comment: ---------- If aNode is a RESET node, then aNode is only in the scope of otherNode if a _proper_ ancestor of the content of aNode is either the content of otherNode or a following sibling of the content of otherNode. Therefore, for a RESET aNode it's enough to check that the grandparent of the content of aNode is a descendant of the parent of the content of otherNode. If at any point getting a parent node would give null, that means that we've reached the top of the tree. At that point, just use the root node for the relevant comparison and take special care with pseudo-element children of the root. ---------- This last part of the comment refers to the following case: root::before { counter-reset: mycounter } child { counter-reset: mycounter } <root><child/><root> In this case the second reset is not scoped inside the first one.... So perhaps the code should perhaps look something like: nsIContent *nodeContent = aNode->mPseudoFrame->GetContent(); if (!aNode->mPseudoFrame->GetStyleContext->GetPseudoType() && nodeContent->GetParent()) { nodeContent = nodeContent->GetParent(); } nsIContent *origNodeContent = nodeContent; if (aNode->mType == nsCounterNode::RESET && nodeContent->GetParent()) { nodeContent = nodeContent->GetParent(); } and later nsIContent *startContent = start->mPseudoFrame->GetContent(); if (!start->mPseudoFrame->GetStyleContext()->GetPseudoType() && startContent->GetParent()) { startContent = startContent->GetParent(); } And then do: if (!(aNode->mType == nsCounterNode::RESET && start->mPseudoFrame->GetStyleContext()->GetPseudoType() && startContent == origNodeContent) && nsContentUtils::ContentIsDescendantOf(nodeContent, startContent)) { ... } Note the startContent == origNodeContnet check -- this is needed to get the interaction of ::before and roots and grandkids of nodes right, I think... The comment to go with this last part: ---------- If we're placing a RESET node and the start node's frame is a pseudo frame, and the startContent is the root node, then we could have origNodeContent also be the root node and nodeContent == origNodeContent. But in this case, the start node is scoped to the ::before of the root and aNode is associated to a child of the root, so |start| is not the scope start for aNode, even though nodeContent is a descendant of startContent. ---------- >Index: layout/base/nsCounterManager.h >+ // for |AddConuterResetsAndIncrements| only "counter", not "conuter". >Index: layout/style/nsCSSDeclaration.cpp >+ nsCSSProperty prop = >+ ((eCSSUnit_Counter <= unit && unit <= eCSSUnit_Counters) && i == 1) >+ ? eCSSProperty_list_style_type : aProperty; That's not right for counters(), is it? For counters() you want i == 2, since you fixed the parsing code to be order-preserving. >Index: layout/style/nsCSSValue.cpp >+void nsCSSValue::SetArrayValue(nsCSSValue::Array* aValue, nsCSSUnit aUnit) This still needs an assert on the unit. The diff to nsHTMLCSSStyleSheet that you mailed me looks fine.
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #179253 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #179318 - Attachment is obsolete: true
Attachment #179322 - Flags: superreview?(bzbarsky)
Attachment #179322 - Flags: review?(bzbarsky)
Attachment #179253 - Flags: superreview?(bzbarsky)
Attachment #179253 - Flags: superreview-
Attachment #179253 - Flags: review?(bzbarsky)
Attachment #179253 - Flags: review-
Comment on attachment 179322 [details] [diff] [review] patch >Index: layout/base/nsCounterManager.cpp >+nsCounterList::SetScope(nsCounterNode *aNode) >+ nsIContent *nodeContent = aNode->mPseudoFrame->GetContent(); >+ if (!aNode->mPseudoFrame->GetStyleContext()->GetPseudoType()) { >+ nodeContent = nodeContent->GetParent(); >+ } Add an NS_ASSERTION(nodeContent) here, please (since aNode has a previous node, it can't possibly be for the root and all. With that, r+sr=bzbarsky.
Attachment #179322 - Flags: superreview?(bzbarsky)
Attachment #179322 - Flags: superreview+
Attachment #179322 - Flags: review?(bzbarsky)
Attachment #179322 - Flags: review+
(In reply to comment #96) > Add an NS_ASSERTION(nodeContent) here, please (since aNode has a previous node, > it can't possibly be for the root and all. No, it can be for the root: :root { counter-reset: c 0 c 0 c 0; counter-increment: c 0 c 0 c 1; } The list will start with 6 counters on the root. However, the null check on startContent is sufficient since if nodeContent is null then startContent will be too, since startContent is something before nodeContent. I'll add an NS_ASSERTION(nodeContent || !startContent, "...") after we get start content asserting that, as discussed on IRC.
... except that actually isn't true.
Attached patch patch (deleted) — Splinter Review
Attachment #179322 - Attachment is obsolete: true
Checked in to trunk, 2005-04-01 15:07 -0800. Filed bug 288704 on using counters for list numbering. Filed bug 288706 on InitAndRestoreFrame not being called all the time. Filed bug 288707 on duplication of code between nsCSSValue's constructor and assignment operator.
Status: ASSIGNED → RESOLVED
Closed: 26 years ago20 years ago
Resolution: --- → FIXED
Blocks: 288704
Blocks: 137285
Comment on attachment 138875 [details] Advanced counter example, using the counters() function This testcase has two problems (at least), FWIW. First, the comment about the author's understanding of counter scoping is correct in that the testcase is wrong: Hn numbering can't be done with nested counters of the same name, and requires counters of different names. Second, it relies on selectors like "h1.appendix h2:before" which aren't going to match anything, since the h2 isn't a descendant of the h1.
Attachment #138875 - Attachment is obsolete: true
Attachment #83764 - Attachment is obsolete: true
(In reply to comment #101) > (From update of attachment 138875 [details] [edit]) > This testcase has two problems (at least), FWIW. First, the comment about the > author's understanding of counter scoping is correct in that the testcase is > wrong: Hn numbering can't be done with nested counters of the same name, and > requires counters of different names. Your understanding is wrong. In CSS 2.1 spec: > Counters are "self-nesting", in the sense that re-using a counter in a child element automatically creates a new instance of the counter. http://www.w3.org/TR/CSS21/generate.html#scope Second point is correct. You must use indirect adjacent combinator '~' instead of descendant combinator, but this is one of the CSS 3 spec.
> Counters are "self-nesting", in the sense that re-using a counter in a child > element automatically creates a new instance of the counter. Right. But note the "child element" part. Here you're reusing a counter on a _sibling_ element.
(In reply to comment #103) > > Counters are "self-nesting", in the sense that re-using a counter in a child > > element automatically creates a new instance of the counter. > > Right. But note the "child element" part. Here you're reusing a counter on a > _sibling_ element. Oh, that's my mistake. Reusing counters on child elements does not work (the 'URL'), so I misunderstood. Sorry!
> Reusing counters on child elements does not work (the 'URL') Which part of it doesn't work?
(In reply to comment #105) > > Reusing counters on child elements does not work (the 'URL') > > Which part of it doesn't work? It seems to be correct thing. I saw wrong results the other day... But this URI shows wrong. (Not my site) http://saiton.net/motors.htm CSS: http://saiton.net/moto.css 1. ul element does not create an instance of counter (no 'counter-reset'). Its child list-item elements increment counter 'item', but all of these have '1' value. In the spec: > If 'counter-increment' refers to a counter that is not in the scope (see below) of any 'counter-reset', the counter is assumed to have been reset to 0 by the root element. 2. ol element creates an instance of counter which have same name as implicit counter (point 1). Mozilla should create new instance of counter, not override existence one. This is a screenshot of this site in Opera 8 beta 3. http://maguroban.s41.xrea.com/image/saito_opera8b3.png
Attached file incorrect simple testcase (missing a counter-reset) (obsolete) (deleted) —
1. No 'counter-reset' in 1st level lists. 2. 'counter-increment' in :before pseudo-element, not list item. Modifying any of these will show correct result.
> I saw wrong results the other day... The test was wrong (missing a counter-increment) a few days back; it's been fixed since. > the counter is assumed to have been reset to 0 by the root element. This part of the spec is getting changed; what David implemented is the current agreement in the working group for what it should say. There are also other changes, such as what exactly constitutes counter scope, etc, that go hand in hand with this, by the way. See the tests at the URL in the URL bar.
What I'm curious about is that "counter-reset" can't be specified in the :before-pseudo-element (as far as I tested). Look for example at the sample CSS found in the current working draft or CSS 2.1: <style type="text/css"> H1:before { content: "Chapter " counter(chapter) ". "; counter-increment: chapter; /* Add 1 to chapter */ counter-reset: section; /* Set section to 0 */ } H2:before { content: counter(chapter) "." counter(section) " "; counter-increment: section; } </style> This is supposed to number chapters and sections, but it doesn't increment the counters. If the counter-reset is moved to h1 instead of h1:before, the sections are incremented correctly (the chapter counter is never reset, so this is wrong according to what you wrote). I couldn't figure out why this shouldn't work. What's the problem?
Please read all the comments that say CSS 2.1 is irrelevant here. Please be patient and wait for the next CSS 2.1 draft which will say exactly what Mozilla does. (As Mozilla implemented a proposal the CSS WG agreed with.)
> can't be specified in the :before-pseudo-element Sure it can. It'll be in effect for the scope of the reset: the children of the :before (there aren't any) and its following siblings (the kids of the <h1>). But the <h2> is not a child of the <h1>, so the scope of the reset on the :before doesn't cover it. As Anne said, one can hope the next draft of CSS2.1 will spell this out better (and fix the examples).
Thanks! Now I understand the "new system" (and I think it's right and more correct).
No longer blocks: majorbugs
Can I have a question? I have read very carefully the CSS-2 starndard and this thread. I'm not sure if it is possible the following numbering: Let's have this XML code: <doc> <sec> <para/> <para/> </sec> <sec> <para/> <para/> </sec> </doc> And I want this numbering: 1. section (1) paragraph (2) paragraph 2. section (3) paragraph (4) paragraph I.e. paragraph counter is global in the whole document. I created simmilar CSS: * {display: block;} sec {counter-increment: sec;} sec:before {content: counter(sec) ".";} para {counter-increment: para;} para:before {content: "(" counter(para) ")";} But the contemporary rendering in firefox is: 1. section (1) paragraph (2) paragraph 2. section (1) paragraph (2) paragraph
That seems wrong, according to CSS 2.1 and the current CSS 3 Content draft. "If 'counter-increment' refers to a counter that is not in the scope (see below) of any 'counter-reset', the counter is assumed to have been reset to 0 by the root element." My reading of that suggests that the second level should have the pretend reset on <doc>, and thus count 1, 2, 3, 4, but instead it acts like the reset is on the <sec>. Putting doc {counter-reset: para;} does make it work as desired, whether the rendering without is correct or not.
> That seems wrong, according to CSS 2.1 and the current CSS 3 Content draft. See comment 108. The implied reset is not on the root but on the element with the increment-not-in-reset-scope, so doesn't extend to the paragraphs in following sections. Putting an explicit reset on the root element, as James suggests, is the right thing to do.
Thanks for clarification. I found out some strange undeterministic behaviour in counter incrementation depending on display property (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050529 Firefox/1.0+). Let's have this XML code: <doc> <sec> <title/> </sec> </doc> and CSS: doc {counter-reset: sec;} sec { /*display: block;*/ counter-increment: sec; } title {display: block;} title:before {content: counter(sec) ".";} Expexted beahaviour is rendering "1.". But it shows ussualy "2.", sometimes "1." If I set the display property of sec to "block", it will count right.
Filed bug 296083 on comment 116. In general, please file separate bugs on followup issues.
Whiteboard: [patch][Hixie-PF]SEE COMMENT 59 FOR STATUS ;hit during nsbeta2 standards compliance testing → [Hixie-PF]Note comment 108; file new bugs for remaining issues
Excuse me. I know this bug in fixed. But why am I still seeing this bug in Firefox 1.5.0.3? The counter-increment doesn't seem to work... Try out the "simple testcase" above if you don't believe. :-(
Attachment #179566 - Attachment description: simple testcase → incorrect simple testcase (missing a counter-reset)
Attachment #179566 - Attachment is obsolete: true
Thanks for the comment. So be sure to put counter-increment on the real element, not in the pseudo-element (:before in this case)... Interestingly Opera 8.54 supports counter-increment in pseudo-elements as well. Maybe it is a bug? Anyway, I'll stop here as I don't mean to add meaningless comment in this bug.
Opera implemented the old model (although not entirely). Firefox its implementation is based on the current specification (largely based on feedback from David Baron).
Status: RESOLVED → VERIFIED
See also Bug 348278 - which is a regression for counters, even when correctly placed.
Depends on: 367220
My tests are now checked in to the trunk in layout/reftests/counters/, thanks to reftest conversion work by Rob Campbell.
Flags: in-testsuite+
Depends on: 534171
Blocks: 1392682
No longer blocks: 1392682
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: