The default bug view has changed. See this FAQ.

counters in css-generated content [GC]

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
CSS Parsing and Computation
P3
enhancement
VERIFIED FIXED
18 years ago
3 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: dbaron)

Tracking

(Blocks: 2 bugs, {css2, testcase})

Trunk
mozilla1.8beta2
css2, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 24 obsolete attachments)

170.82 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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!).
(Reporter)

Comment 1

18 years ago
Test cases for generated content...
...with text:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/content/1.html
...with quotes:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/content/2.html
...with url()s:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/content/3.html
...and with attr()s:
  http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/content/4.html

Updated

18 years ago
Assignee: peterl → troy
Component: Style System → Layout

Comment 2

18 years ago
The content properties are now in the style contexts. Assign to rickg for the
counter support when you're done...

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 3

18 years ago
Thanks Peter

Updated

18 years ago
Target Milestone: M5

Updated

18 years ago
Target Milestone: M5 → M7

Updated

18 years ago
Assignee: troy → rickg
Status: ASSIGNED → NEW

Comment 4

18 years ago
Rick, assigning to you for counter support. I created a new bug (marked REMIND)
concerning nested quotes (currently not implemented)

Updated

18 years ago
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → REMIND
(Reporter)

Updated

18 years ago
Status: RESOLVED → VERIFIED
Priority: P2 → P3
Summary: :before and :after pseudo-elements → {css2} counters in generated content
(Reporter)

Comment 5

18 years ago
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.
(Reporter)

Comment 6

18 years ago
*** Bug 9840 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

18 years ago
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.
(Reporter)

Updated

18 years ago
Blocks: 15174
Target Milestone: M7
(Reporter)

Comment 8

18 years ago
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.
(Reporter)

Comment 9

18 years ago
*** Bug 14948 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

17 years ago
Keywords: css2
(Reporter)

Comment 10

17 years ago
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...
(Reporter)

Comment 11

17 years ago
Reopening and moving to Future.
Status: VERIFIED → REOPENED
Keywords: correctness, testcase
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
(Reporter)

Updated

17 years ago
(Reporter)

Updated

17 years ago
Summary: counters in generated content → counters in generated content [GC]
(Reporter)

Comment 12

17 years ago
*** Bug 51393 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 13

17 years ago
*** Bug 57393 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
add pierre and erik to the cc list.
*** Bug 70191 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

16 years ago
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. ***

Comment 18

16 years ago
Fixing summary to show up on searches for "css counter".
Summary: counters in generated content [GC] → counters in css-generated content [GC]

Comment 19

16 years ago
any progress with this bug?
(Reporter)

Comment 20

15 years ago
not while it's assigned to nobody... reassigning to default owner.
Assignee: rickg → attinasi
Status: REOPENED → NEW
QA Contact: ian → petersen

Comment 21

15 years ago
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...
(Assignee)

Comment 23

15 years ago
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.

Comment 25

15 years ago
Actually, that page appears to be correct in stating that Opera 5 implements
counters.

Comment 26

15 years ago
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.

Comment 27

15 years ago
Created attachment 83764 [details]
Testcases for counter(s)

4 Testcases with hyperlinks "should be" pics.

Comment 28

15 years ago
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?
for reference:
url to the section of the spec:
http://www.w3.org/TR/REC-CSS2/generate.html#counters

Comment 30

15 years ago
-> Style System
Component: Layout → Style System
(Assignee)

Comment 31

15 years ago
Layout is the correct component.  Please leave it there.
Component: Style System → Layout

Comment 32

15 years ago
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)

Comment 34

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

Comment 35

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

Comment 36

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

Comment 37

14 years ago
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
(Reporter)

Comment 38

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

Comment 40

14 years ago
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...
(Reporter)

Comment 41

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

Updated

14 years ago
Priority: -- → P3
Target Milestone: --- → Future

Comment 44

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

Comment 45

14 years ago
*** Bug 214871 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 46

14 years ago
->style system, for lack of a better component for what probably centers around
frame construction
Assignee: other → dbaron
Component: Layout → Style System
(Assignee)

Updated

14 years ago

Comment 47

14 years ago
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)
(Assignee)

Comment 48

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

Comment 49

14 years ago
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.
(Assignee)

Comment 50

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

Updated

14 years ago
Blocks: 163993
*** Bug 230681 has been marked as a duplicate of this bug. ***

Comment 52

13 years ago
Created attachment 138874 [details]
Advanced counter example, using the counters() function

Comment 53

13 years ago
Created attachment 138875 [details]
Advanced counter example, using the counters() function

Comment 54

13 years ago
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: 24861
(Assignee)

Comment 56

13 years ago
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
(Assignee)

Comment 57

13 years ago
Created attachment 146367 [details] [diff] [review]
some work in progress

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
(Assignee)

Comment 58

13 years ago
Created attachment 150755 [details] [diff] [review]
work in progress

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
(Assignee)

Comment 59

13 years ago
Created attachment 153572 [details] [diff] [review]
work in progress 

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

Comment 60

13 years ago
SInce the clarification of the spec is a long way to go, could you check in the
current implementation as -moz-counter-* stuff?

Comment 61

13 years ago
Anything new ? It would be great to fix this bug for FireFox 1.0. ;-)

Comment 62

13 years ago
(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. :)
(Assignee)

Updated

13 years ago
Whiteboard: [Hixie-PF] hit during nsbeta2 standards compliance testing → [Hixie-PF]SEE COMMENT 59 FOR STATUS ;hit during nsbeta2 standards compliance testing

Comment 63

13 years ago
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).".
(Assignee)

Comment 64

12 years ago
Created attachment 168745 [details] [diff] [review]
work in progress
Attachment #153572 - Attachment is obsolete: true
(Assignee)

Comment 65

12 years ago
Created attachment 171220 [details] [diff] [review]
work in progress
Attachment #168745 - Attachment is obsolete: true
(Assignee)

Comment 66

12 years ago
Created attachment 171281 [details] [diff] [review]
work in progress
Attachment #171220 - Attachment is obsolete: true
(Assignee)

Comment 67

12 years ago
Created attachment 171302 [details] [diff] [review]
work in progress
(Assignee)

Updated

12 years ago
Attachment #171281 - Attachment is obsolete: true
(Assignee)

Comment 68

12 years ago
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).
(Assignee)

Comment 69

12 years ago
Created attachment 171727 [details] [diff] [review]
work in progress

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
(Assignee)

Comment 70

12 years ago
Created attachment 171784 [details] [diff] [review]
patch

The dynamic change handling is now implemented, although completely untested.
Attachment #171727 - Attachment is obsolete: true
(Assignee)

Comment 71

12 years ago
Created attachment 171824 [details] [diff] [review]
patch
Attachment #171784 - Attachment is obsolete: true
(Assignee)

Comment 72

12 years ago
Created attachment 171925 [details] [diff] [review]
patch
Attachment #171824 - Attachment is obsolete: true
(Assignee)

Comment 73

12 years ago
I need to check that this code behaves reasonably when pseudo-elements other
than :before or :after are involved.
(Assignee)

Comment 74

12 years ago
Created attachment 177060 [details] [diff] [review]
patch
Attachment #171925 - Attachment is obsolete: true
(Assignee)

Comment 75

12 years ago
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)
(Assignee)

Updated

12 years ago
Blocks: 286303
(Assignee)

Updated

12 years ago
Target Milestone: Future → mozilla1.8beta2
(Assignee)

Updated

12 years ago
Blocks: 280443
(Assignee)

Updated

12 years ago
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
(Assignee)

Comment 76

12 years ago
Created attachment 177977 [details] [diff] [review]
patch

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)
(Assignee)

Updated

12 years ago
Attachment #177060 - Attachment is obsolete: false
Attachment #177060 - Flags: superreview?(bzbarsky)
Attachment #177060 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #177060 - Attachment is obsolete: true

Comment 77

12 years ago
With Mozilla suite being end-of-lined, should this bug be moved to Firefox?

Comment 78

12 years ago
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.

Comment 79

12 years ago
(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.
(Assignee)

Updated

12 years ago
Blocks: 282569
(Assignee)

Comment 80

12 years ago
Created attachment 178659 [details] [diff] [review]
patch

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

Updated

12 years ago
Attachment #177977 - Attachment is obsolete: true
Attachment #178659 - Flags: superreview?(bzbarsky)
Attachment #178659 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #177977 - Flags: superreview?(bzbarsky)
Attachment #177977 - Flags: review?(bzbarsky)
(Assignee)

Comment 81

12 years ago
Created attachment 178755 [details] [diff] [review]
patch

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)
(Assignee)

Updated

12 years ago
Attachment #178659 - Flags: superreview?(bzbarsky)
Attachment #178659 - Flags: review?(bzbarsky)
(Assignee)

Comment 82

12 years ago
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.
(Assignee)

Comment 83

12 years ago
Created attachment 179074 [details] [diff] [review]
patch

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)
(Assignee)

Updated

12 years ago
Attachment #178755 - Flags: superreview?(bzbarsky)
Attachment #178755 - Flags: review?(bzbarsky)
(Assignee)

Comment 84

12 years ago
Although I realized I should probably rename nsCSSValue::List to nsCSSValue::Array.
(Assignee)

Comment 85

12 years ago
Created attachment 179089 [details] [diff] [review]
patch

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)
(Assignee)

Updated

12 years ago
Attachment #179074 - Flags: superreview?(bzbarsky)
Attachment #179074 - Flags: review?(bzbarsky)
(Assignee)

Comment 86

12 years ago
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.
(Assignee)

Comment 88

12 years ago
(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.
(Assignee)

Comment 89

12 years ago
(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.
(Assignee)

Comment 90

12 years ago
(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.
(Assignee)

Comment 91

12 years ago
Created attachment 179253 [details] [diff] [review]
patch

Should address bzbarsky's comments.  Passes my tests.
(Assignee)

Updated

12 years ago
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.
(Assignee)

Comment 94

12 years ago
Created attachment 179318 [details] [diff] [review]
patch
Attachment #179253 - Attachment is obsolete: true
(Assignee)

Comment 95

12 years ago
Created attachment 179322 [details] [diff] [review]
patch
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+
(Assignee)

Comment 97

12 years ago
(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.
(Assignee)

Comment 98

12 years ago
... except that actually isn't true.
(Assignee)

Comment 99

12 years ago
Created attachment 179324 [details] [diff] [review]
patch
Attachment #179322 - Attachment is obsolete: true
(Assignee)

Comment 100

12 years ago
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
Last Resolved: 18 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 288704
(Assignee)

Updated

12 years ago
Blocks: 137285
(Assignee)

Comment 101

12 years ago
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
(Assignee)

Updated

12 years ago
Attachment #83764 - Attachment is obsolete: true

Comment 102

12 years ago
(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.

Comment 104

12 years ago
(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?

Comment 106

12 years ago
(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

Comment 107

12 years ago
Created attachment 179566 [details]
incorrect simple testcase (missing a counter-reset)

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.

Comment 109

12 years ago
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?

Comment 110

12 years ago
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).

Comment 112

12 years ago
Thanks! Now I understand the "new system" (and I think it's right and more correct).

Updated

12 years ago
No longer blocks: 163993

Comment 113

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

Comment 114

12 years ago
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.

Comment 116

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

Comment 118

11 years ago
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. :-(
(Assignee)

Updated

11 years ago
Attachment #179566 - Attachment description: simple testcase → incorrect simple testcase (missing a counter-reset)
Attachment #179566 - Attachment is obsolete: true
(Assignee)

Comment 119

11 years ago
See comment 108.

Comment 120

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

Comment 121

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

Comment 122

11 years ago
See also Bug 348278 - which is a regression for counters, even when correctly placed.

(Assignee)

Updated

10 years ago
Depends on: 367220
(Assignee)

Comment 123

10 years ago
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
You need to log in before you can comment on or make changes to this bug.