Last Comment Bug 3247 - counters in css-generated content [GC]
: counters in css-generated content [GC]
Status: VERIFIED FIXED
[Hixie-PF]Note comment 108; file new ...
: css2, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 enhancement with 63 votes (vote)
: mozilla1.8beta2
Assigned To: David Baron :dbaron: ⌚️UTC-10
: Hixie (not reading bugmail)
: Jet Villegas (:jet)
Mentors:
http://dbaron.org/css/test/sec1202
: 9840 14948 51393 57393 70191 81381 94182 189465 190268 193927 213703 214871 230681 (view as bug list)
Depends on: nested-quotes 367220 534171
Blocks: 3246 288704 15174 137285 241719 280443 282569 286303
  Show dependency treegraph
 
Reported: 1999-02-23 12:45 PST by Hixie (not reading bugmail)
Modified: 2014-04-26 03:18 PDT (History)
90 users (show)
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcases for counter(s) (3.28 KB, text/html)
2002-05-15 12:21 PDT, Christian Eyrich
no flags Details
Advanced counter example, using the counters() function (3.13 KB, text/html)
2004-01-12 07:41 PST, Tobias Herp
no flags Details
Advanced counter example, using the counters() function (3.13 KB, text/html)
2004-01-12 07:43 PST, Tobias Herp
no flags Details
some work in progress (71.12 KB, patch)
2004-04-17 13:50 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (91.92 KB, patch)
2004-06-14 16:23 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (93.17 KB, patch)
2004-07-18 00:43 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (110.73 KB, patch)
2004-12-14 21:42 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (132.05 KB, patch)
2005-01-13 17:58 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (138.47 KB, patch)
2005-01-14 12:32 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (141.96 KB, patch)
2005-01-14 15:32 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (140.76 KB, patch)
2005-01-19 01:12 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (145.64 KB, patch)
2005-01-19 12:25 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (148.64 KB, patch)
2005-01-19 17:16 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (148.71 KB, patch)
2005-01-20 14:17 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (148.85 KB, patch)
2005-03-10 15:31 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (148.72 KB, patch)
2005-03-19 12:12 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (150.17 KB, patch)
2005-03-26 02:35 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (151.08 KB, patch)
2005-03-27 15:49 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (151.08 KB, patch)
2005-03-30 09:55 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (163.08 KB, patch)
2005-03-30 11:39 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (166.43 KB, patch)
2005-04-01 00:03 PST, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
patch (170.17 KB, patch)
2005-04-01 14:20 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (170.38 KB, patch)
2005-04-01 14:32 PST, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch (170.82 KB, patch)
2005-04-01 14:56 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
incorrect simple testcase (missing a counter-reset) (614 bytes, text/html)
2005-04-04 03:40 PDT, Anbo Motohiko
no flags Details

Description Hixie (not reading bugmail) 1999-02-23 12:45:38 PST
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!).
Comment 2 Peter Linss 1999-04-02 11:13:59 PST
The content properties are now in the style contexts. Assign to rickg for the
counter support when you're done...
Comment 3 troy 1999-04-02 11:27:59 PST
Thanks Peter
Comment 4 troy 1999-04-27 08:16:59 PDT
Rick, assigning to you for counter support. I created a new bug (marked REMIND)
concerning nested quotes (currently not implemented)
Comment 5 Hixie (not reading bugmail) 1999-05-26 06:44:59 PDT
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.
Comment 6 Hixie (not reading bugmail) 1999-09-26 06:25:59 PDT
*** Bug 9840 has been marked as a duplicate of this bug. ***
Comment 7 Hixie (not reading bugmail) 1999-09-26 06:31:59 PDT
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.
Comment 8 Hixie (not reading bugmail) 1999-09-29 05:10:59 PDT
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.
Comment 9 Hixie (not reading bugmail) 1999-10-03 13:35:59 PDT
*** Bug 14948 has been marked as a duplicate of this bug. ***
Comment 10 Hixie (not reading bugmail) 2000-01-13 16:07:59 PST
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...
Comment 11 Hixie (not reading bugmail) 2000-08-04 14:55:03 PDT
Reopening and moving to Future.
Comment 12 Hixie (not reading bugmail) 2000-09-06 00:03:29 PDT
*** Bug 51393 has been marked as a duplicate of this bug. ***
Comment 13 Hixie (not reading bugmail) 2000-10-20 01:32:44 PDT
*** Bug 57393 has been marked as a duplicate of this bug. ***
Comment 14 Frank Tang 2000-10-26 21:19:35 PDT
add pierre and erik to the cc list.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2001-02-26 09:04:41 PST
*** Bug 70191 has been marked as a duplicate of this bug. ***
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2001-05-17 07:07:44 PDT
*** Bug 81381 has been marked as a duplicate of this bug. ***
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2001-08-07 14:31:04 PDT
*** Bug 94182 has been marked as a duplicate of this bug. ***
Comment 18 Jesse Ruderman 2001-09-26 15:22:14 PDT
Fixing summary to show up on searches for "css counter".
Comment 19 Madhur Bhatia 2001-10-24 16:33:59 PDT
any progress with this bug?
Comment 20 Hixie (not reading bugmail) 2002-01-19 19:42:48 PST
not while it's assigned to nobody... reassigning to default owner.
Comment 21 Christian Eyrich 2002-05-13 11:23:36 PDT
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 ...
Comment 22 Sören 'Chucker' Kuklau (gone) 2002-05-13 11:57:07 PDT
#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...
Comment 23 David Baron :dbaron: ⌚️UTC-10 2002-05-13 11:58:15 PDT
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?)
Comment 24 Sören 'Chucker' Kuklau (gone) 2002-05-13 12:01:11 PDT
#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 Greg K. 2002-05-13 12:21:43 PDT
Actually, that page appears to be correct in stating that Opera 5 implements
counters.
Comment 26 Christian Eyrich 2002-05-13 14:22:45 PDT
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 Christian Eyrich 2002-05-15 12:21:08 PDT
Created attachment 83764 [details]
Testcases for counter(s)

4 Testcases with hyperlinks "should be" pics.
Comment 28 Madhur Bhatia 2002-05-29 17:07:05 PDT
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?
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2002-06-15 11:47:35 PDT
for reference:
url to the section of the spec:
http://www.w3.org/TR/REC-CSS2/generate.html#counters
Comment 30 Marcus Campbell 2002-07-31 03:34:11 PDT
-> Style System
Comment 31 David Baron :dbaron: ⌚️UTC-10 2002-07-31 09:49:10 PDT
Layout is the correct component.  Please leave it there.
Comment 32 Marcus Campbell 2002-08-07 05:41:33 PDT
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?
Comment 33 Kai Lahmann (is there, where MNG is) 2002-08-07 05:44:18 PDT
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 Olivier Cahagne 2003-01-17 08:50:09 PST
*** Bug 189465 has been marked as a duplicate of this bug. ***
Comment 35 Michael Lefevre 2003-01-23 03:13:46 PST
*** Bug 190268 has been marked as a duplicate of this bug. ***
Comment 36 Nathan Kurz 2003-02-06 03:23:24 PST
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 Marcus Campbell 2003-02-06 05:50:47 PST
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
Comment 38 Hixie (not reading bugmail) 2003-02-08 07:43:44 PST
Counters will be in CSS3 regardless. CSS2.1 does not affect whetehr Mozilla will
implement something or not in any way.
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2003-02-18 14:18:53 PST
*** Bug 193927 has been marked as a duplicate of this bug. ***
Comment 40 Jeff Dickey 2003-02-21 14:44:17 PST
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...
Comment 41 Hixie (not reading bugmail) 2003-02-22 09:09:31 PST
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.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2003-05-01 22:53:17 PDT
.
Comment 43 Alex Vincent [:WeirdAl] 2003-06-20 20:54:41 PDT
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.
Comment 44 Bill Mason 2003-07-24 08:40:21 PDT
*** Bug 213703 has been marked as a duplicate of this bug. ***
Comment 45 Bill Mason 2003-08-02 09:02:45 PDT
*** Bug 214871 has been marked as a duplicate of this bug. ***
Comment 46 David Baron :dbaron: ⌚️UTC-10 2003-08-02 13:35:40 PDT
->style system, for lack of a better component for what probably centers around
frame construction
Comment 47 Anne (:annevk) 2003-11-01 11:53:40 PST
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 48 David Baron :dbaron: ⌚️UTC-10 2003-11-01 11:59:09 PST
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 Anne (:annevk) 2003-11-01 12:05:35 PST
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.
Comment 50 David Baron :dbaron: ⌚️UTC-10 2003-11-01 14:11:01 PST
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.
Comment 51 Christian :Biesinger (don't email me, ping me on IRC) 2004-01-12 05:13:40 PST
*** Bug 230681 has been marked as a duplicate of this bug. ***
Comment 52 Tobias Herp 2004-01-12 07:41:44 PST
Created attachment 138874 [details]
Advanced counter example, using the counters() function
Comment 53 Tobias Herp 2004-01-12 07:43:24 PST
Created attachment 138875 [details]
Advanced counter example, using the counters() function
Comment 54 Tobias Herp 2004-01-12 07:44:46 PST
Comment on attachment 138874 [details]
Advanced counter example, using the counters() function

sorry, the same one uploaded twice
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2004-04-10 12:19:10 PDT
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?
Comment 56 David Baron :dbaron: ⌚️UTC-10 2004-04-10 12:32:55 PDT
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).
Comment 57 David Baron :dbaron: ⌚️UTC-10 2004-04-17 13:50:17 PDT
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.
Comment 58 David Baron :dbaron: ⌚️UTC-10 2004-06-14 16:23:28 PDT
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.
Comment 59 David Baron :dbaron: ⌚️UTC-10 2004-07-18 00:43:01 PDT
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).
Comment 60 Sekundes 2004-08-19 20:21:33 PDT
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 Worontzoff Philippe 2004-11-04 07:45:15 PST
Anything new ? It would be great to fix this bug for FireFox 1.0. ;-)
Comment 62 mozbugs3 2004-11-04 08:55:39 PST
(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. :)
Comment 63 Worontzoff Philippe 2004-11-04 15:52:52 PST
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).".
Comment 64 David Baron :dbaron: ⌚️UTC-10 2004-12-14 21:42:24 PST
Created attachment 168745 [details] [diff] [review]
work in progress
Comment 65 David Baron :dbaron: ⌚️UTC-10 2005-01-13 17:58:22 PST
Created attachment 171220 [details] [diff] [review]
work in progress
Comment 66 David Baron :dbaron: ⌚️UTC-10 2005-01-14 12:32:28 PST
Created attachment 171281 [details] [diff] [review]
work in progress
Comment 67 David Baron :dbaron: ⌚️UTC-10 2005-01-14 15:32:15 PST
Created attachment 171302 [details] [diff] [review]
work in progress
Comment 68 David Baron :dbaron: ⌚️UTC-10 2005-01-17 01:24:27 PST
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).
Comment 69 David Baron :dbaron: ⌚️UTC-10 2005-01-19 01:12:58 PST
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.
Comment 70 David Baron :dbaron: ⌚️UTC-10 2005-01-19 12:25:01 PST
Created attachment 171784 [details] [diff] [review]
patch

The dynamic change handling is now implemented, although completely untested.
Comment 71 David Baron :dbaron: ⌚️UTC-10 2005-01-19 17:16:33 PST
Created attachment 171824 [details] [diff] [review]
patch
Comment 72 David Baron :dbaron: ⌚️UTC-10 2005-01-20 14:17:43 PST
Created attachment 171925 [details] [diff] [review]
patch
Comment 73 David Baron :dbaron: ⌚️UTC-10 2005-01-25 10:42:06 PST
I need to check that this code behaves reasonably when pseudo-elements other
than :before or :after are involved.
Comment 74 David Baron :dbaron: ⌚️UTC-10 2005-03-10 15:31:57 PST
Created attachment 177060 [details] [diff] [review]
patch
Comment 75 David Baron :dbaron: ⌚️UTC-10 2005-03-18 16:55:42 PST
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.
Comment 76 David Baron :dbaron: ⌚️UTC-10 2005-03-19 12:12:18 PST
Created attachment 177977 [details] [diff] [review]
patch

Merged to trunk, with a bug I noticed (accidentally removed a |break|) fixed as
well.
Comment 77 Adam Messinger 2005-03-22 14:17:53 PST
With Mozilla suite being end-of-lined, should this bug be moved to Firefox?
Comment 78 Anne (:annevk) 2005-03-22 14:20:12 PST
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 Adam Messinger 2005-03-24 06:22:48 PST
(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.
Comment 80 David Baron :dbaron: ⌚️UTC-10 2005-03-26 02:35:07 PST
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.
Comment 81 David Baron :dbaron: ⌚️UTC-10 2005-03-27 15:49:34 PST
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).
Comment 82 David Baron :dbaron: ⌚️UTC-10 2005-03-27 16:26:16 PST
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.
Comment 83 David Baron :dbaron: ⌚️UTC-10 2005-03-30 09:55:09 PST
Created attachment 179074 [details] [diff] [review]
patch

Merged to trunk, plus what I mentioned in comment 82.
Comment 84 David Baron :dbaron: ⌚️UTC-10 2005-03-30 09:58:02 PST
Although I realized I should probably rename nsCSSValue::List to nsCSSValue::Array.
Comment 85 David Baron :dbaron: ⌚️UTC-10 2005-03-30 11:39:00 PST
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.
Comment 86 David Baron :dbaron: ⌚️UTC-10 2005-03-31 10:47:48 PST
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 87 Boris Zbarsky [:bz] (still a bit busy) 2005-03-31 20:17:12 PST
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.
Comment 88 David Baron :dbaron: ⌚️UTC-10 2005-03-31 23:23:04 PST
(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.
Comment 89 David Baron :dbaron: ⌚️UTC-10 2005-03-31 23:30:44 PST
(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.
Comment 90 David Baron :dbaron: ⌚️UTC-10 2005-03-31 23:38:46 PST
(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.
Comment 91 David Baron :dbaron: ⌚️UTC-10 2005-04-01 00:03:35 PST
Created attachment 179253 [details] [diff] [review]
patch

Should address bzbarsky's comments.  Passes my tests.
Comment 92 Boris Zbarsky [:bz] (still a bit busy) 2005-04-01 09:11:28 PST
> 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.
Comment 93 Boris Zbarsky [:bz] (still a bit busy) 2005-04-01 13:10:17 PST
>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.
Comment 94 David Baron :dbaron: ⌚️UTC-10 2005-04-01 14:20:15 PST
Created attachment 179318 [details] [diff] [review]
patch
Comment 95 David Baron :dbaron: ⌚️UTC-10 2005-04-01 14:32:05 PST
Created attachment 179322 [details] [diff] [review]
patch
Comment 96 Boris Zbarsky [:bz] (still a bit busy) 2005-04-01 14:36:34 PST
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.
Comment 97 David Baron :dbaron: ⌚️UTC-10 2005-04-01 14:41:26 PST
(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.
Comment 98 David Baron :dbaron: ⌚️UTC-10 2005-04-01 14:46:43 PST
... except that actually isn't true.
Comment 99 David Baron :dbaron: ⌚️UTC-10 2005-04-01 14:56:01 PST
Created attachment 179324 [details] [diff] [review]
patch
Comment 100 David Baron :dbaron: ⌚️UTC-10 2005-04-01 15:48:37 PST
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.
Comment 101 David Baron :dbaron: ⌚️UTC-10 2005-04-01 17:27:20 PST
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.
Comment 102 Anbo Motohiko 2005-04-03 01:36:09 PST
(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.
Comment 103 Boris Zbarsky [:bz] (still a bit busy) 2005-04-03 09:01:34 PDT
> 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 Anbo Motohiko 2005-04-03 15:50:42 PDT
(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!
Comment 105 Boris Zbarsky [:bz] (still a bit busy) 2005-04-03 15:57:12 PDT
> Reusing counters on child elements does not work (the 'URL')

Which part of it doesn't work?
Comment 106 Anbo Motohiko 2005-04-04 03:28:11 PDT
(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 Anbo Motohiko 2005-04-04 03:40:40 PDT
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.
Comment 108 Boris Zbarsky [:bz] (still a bit busy) 2005-04-04 07:51:25 PDT
> 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 Daniel Kraft 2005-04-04 12:10:49 PDT
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 Anne (:annevk) 2005-04-04 12:13:40 PDT
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.)
Comment 111 Boris Zbarsky [:bz] (still a bit busy) 2005-04-04 12:52:22 PDT
> 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 Daniel Kraft 2005-04-04 13:23:49 PDT
Thanks! Now I understand the "new system" (and I think it's right and more correct).
Comment 113 petr.pisar 2005-05-30 05:32:21 PDT
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 James Ross 2005-05-30 05:51:54 PDT
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.
Comment 115 Boris Zbarsky [:bz] (still a bit busy) 2005-05-30 08:06:49 PDT
> 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 petr.pisar 2005-05-31 04:50:48 PDT
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.
Comment 117 Boris Zbarsky [:bz] (still a bit busy) 2005-05-31 09:22:51 PDT
Filed bug 296083 on comment 116.  In general, please file separate bugs on
followup issues.
Comment 118 Ng Ming Hong 2006-05-21 01:04:14 PDT
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. :-(
Comment 119 David Baron :dbaron: ⌚️UTC-10 2006-05-21 01:11:53 PDT
See comment 108.
Comment 120 Ng Ming Hong 2006-05-21 01:17:44 PDT
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 Anne (:annevk) 2006-05-21 02:15:11 PDT
Opera implemented the old model (although not entirely). Firefox its implementation is based on the current specification (largely based on feedback from David Baron).
Comment 122 David A. Wheeler 2006-08-10 15:48:42 PDT
See also Bug 348278 - which is a regression for counters, even when correctly placed.

Comment 123 David Baron :dbaron: ⌚️UTC-10 2007-03-22 23:35:23 PDT
My tests are now checked in to the trunk in layout/reftests/counters/, thanks to reftest conversion work by Rob Campbell.

Note You need to log in before you can comment on or make changes to this bug.