Closed
Bug 3247
Opened 26 years ago
Closed 20 years ago
counters in css-generated content [GC]
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.8beta2
People
(Reporter: ian, Assigned: dbaron)
References
()
Details
(Keywords: css2, testcase, Whiteboard: [Hixie-PF]Note comment 108; file new bugs for remaining issues)
Attachments
(1 file, 24 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
You do not support the :before and :after pseudo-elements, which are part of
CSS2 generated content.
This is defined in the spec:
http://www.w3.org/TR/REC-CSS2/generate.html
Generated content isa prerequisite to correctly implementing Q (see bug #1061)
in the ua stylesheet, and would also be useful to QA in testing attribute
support (in addition, of course, to being useful in its own right on the web!).
Reporter | ||
Comment 1•26 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•26 years ago
|
Assignee: peterl → troy
Component: Style System → Layout
Comment 2•26 years ago
|
||
The content properties are now in the style contexts. Assign to rickg for the
counter support when you're done...
Rick, assigning to you for counter support. I created a new bug (marked REMIND)
concerning nested quotes (currently not implemented)
Reporter | ||
Updated•26 years ago
|
Status: RESOLVED → VERIFIED
Priority: P2 → P3
Summary: :before and :after pseudo-elements → {css2} counters in generated content
Reporter | ||
Comment 5•26 years ago
|
||
Reporter | ||
Comment 7•25 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 | ||
Comment 8•25 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 10•25 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•25 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•25 years ago
|
Reporter | ||
Updated•24 years ago
|
Summary: counters in generated content → counters in generated content [GC]
Reporter | ||
Comment 12•24 years ago
|
||
*** Bug 51393 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 13•24 years ago
|
||
*** Bug 57393 has been marked as a duplicate of this bug. ***
Comment 14•24 years ago
|
||
add pierre and erik to the cc list.
Comment 15•24 years ago
|
||
*** Bug 70191 has been marked as a duplicate of this bug. ***
Reporter | ||
Updated•24 years ago
|
Whiteboard: hit during nsbeta2 standards compliance testing → [Hixie-PF] hit during nsbeta2 standards compliance testing
Comment 16•24 years ago
|
||
*** Bug 81381 has been marked as a duplicate of this bug. ***
Comment 17•24 years ago
|
||
*** Bug 94182 has been marked as a duplicate of this bug. ***
Comment 18•23 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•23 years ago
|
||
any progress with this bug?
Reporter | ||
Comment 20•23 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•23 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 ...
Comment 22•23 years ago
|
||
#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•23 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?)
Comment 24•23 years ago
|
||
#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•23 years ago
|
||
Actually, that page appears to be correct in stating that Opera 5 implements
counters.
Comment 26•23 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•23 years ago
|
||
4 Testcases with hyperlinks "should be" pics.
Comment 28•23 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?
Comment 29•23 years ago
|
||
for reference:
url to the section of the spec:
http://www.w3.org/TR/REC-CSS2/generate.html#counters
Assignee | ||
Comment 31•23 years ago
|
||
Layout is the correct component. Please leave it there.
Component: Style System → Layout
Comment 32•23 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?
Comment 33•23 years ago
|
||
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•22 years ago
|
||
*** Bug 189465 has been marked as a duplicate of this bug. ***
Comment 35•22 years ago
|
||
*** Bug 190268 has been marked as a duplicate of this bug. ***
Comment 36•22 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•22 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•22 years ago
|
||
Counters will be in CSS3 regardless. CSS2.1 does not affect whetehr Mozilla will
implement something or not in any way.
Comment 39•22 years ago
|
||
*** Bug 193927 has been marked as a duplicate of this bug. ***
Comment 40•22 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•22 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.
Comment 42•22 years ago
|
||
.
Assignee: attinasi → other
Priority: P3 → --
QA Contact: cpetersen0953 → ian
Target Milestone: Future → ---
Comment 43•22 years ago
|
||
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•22 years ago
|
Priority: -- → P3
Target Milestone: --- → Future
Comment 44•22 years ago
|
||
*** Bug 213703 has been marked as a duplicate of this bug. ***
Comment 45•22 years ago
|
||
*** Bug 214871 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 46•22 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•21 years ago
|
Comment 47•21 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•21 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•21 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•21 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.
Comment 51•21 years ago
|
||
*** Bug 230681 has been marked as a duplicate of this bug. ***
Comment 52•21 years ago
|
||
Comment 53•21 years ago
|
||
Comment 54•21 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
Comment 55•21 years ago
|
||
I think an approach similar to that of bug 24861 could be taken here (with use
of one list per named counter to keep track of the current value or something).
David, does that seem like a viable approach to you?
Blocks: 3246
Depends on: nested-quotes
Assignee | ||
Comment 56•21 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•21 years ago
|
||
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.
Assignee | ||
Comment 58•21 years ago
|
||
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•21 years ago
|
||
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•20 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•20 years ago
|
||
Anything new ? It would be great to fix this bug for FireFox 1.0. ;-)
Comment 62•20 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•20 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•20 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•20 years ago
|
||
Attachment #153572 -
Attachment is obsolete: true
Assignee | ||
Comment 65•20 years ago
|
||
Attachment #168745 -
Attachment is obsolete: true
Assignee | ||
Comment 66•20 years ago
|
||
Attachment #171220 -
Attachment is obsolete: true
Assignee | ||
Comment 67•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #171281 -
Attachment is obsolete: true
Assignee | ||
Comment 68•20 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•20 years ago
|
||
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•20 years ago
|
||
The dynamic change handling is now implemented, although completely untested.
Attachment #171727 -
Attachment is obsolete: true
Assignee | ||
Comment 71•20 years ago
|
||
Attachment #171784 -
Attachment is obsolete: true
Assignee | ||
Comment 72•20 years ago
|
||
Attachment #171824 -
Attachment is obsolete: true
Assignee | ||
Comment 73•20 years ago
|
||
I need to check that this code behaves reasonably when pseudo-elements other
than :before or :after are involved.
Assignee | ||
Comment 74•20 years ago
|
||
Attachment #171925 -
Attachment is obsolete: true
Assignee | ||
Comment 75•20 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•20 years ago
|
Target Milestone: Future → mozilla1.8beta2
Assignee | ||
Updated•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #177060 -
Attachment is obsolete: false
Attachment #177060 -
Flags: superreview?(bzbarsky)
Attachment #177060 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•20 years ago
|
Attachment #177060 -
Attachment is obsolete: true
Comment 77•20 years ago
|
||
With Mozilla suite being end-of-lined, should this bug be moved to Firefox?
Comment 78•20 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•20 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 | ||
Comment 80•20 years ago
|
||
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•20 years ago
|
Attachment #177977 -
Attachment is obsolete: true
Attachment #178659 -
Flags: superreview?(bzbarsky)
Attachment #178659 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•20 years ago
|
Attachment #177977 -
Flags: superreview?(bzbarsky)
Attachment #177977 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 81•20 years ago
|
||
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•20 years ago
|
Attachment #178659 -
Flags: superreview?(bzbarsky)
Attachment #178659 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 82•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #178755 -
Flags: superreview?(bzbarsky)
Attachment #178755 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 84•20 years ago
|
||
Although I realized I should probably rename nsCSSValue::List to nsCSSValue::Array.
Assignee | ||
Comment 85•20 years ago
|
||
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•20 years ago
|
Attachment #179074 -
Flags: superreview?(bzbarsky)
Attachment #179074 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 86•20 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 87•20 years ago
|
||
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•20 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•20 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•20 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•20 years ago
|
||
Should address bzbarsky's comments. Passes my tests.
Assignee | ||
Updated•20 years ago
|
Attachment #179089 -
Attachment is obsolete: true
Attachment #179253 -
Flags: superreview?(bzbarsky)
Attachment #179253 -
Flags: review?(bzbarsky)
Comment 92•20 years ago
|
||
> 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.
Updated•20 years ago
|
Attachment #179089 -
Flags: superreview?(bzbarsky)
Attachment #179089 -
Flags: review?(bzbarsky)
Comment 93•20 years ago
|
||
>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•20 years ago
|
||
Attachment #179253 -
Attachment is obsolete: true
Assignee | ||
Comment 95•20 years ago
|
||
Attachment #179318 -
Attachment is obsolete: true
Attachment #179322 -
Flags: superreview?(bzbarsky)
Attachment #179322 -
Flags: review?(bzbarsky)
Updated•20 years ago
|
Attachment #179253 -
Flags: superreview?(bzbarsky)
Attachment #179253 -
Flags: superreview-
Attachment #179253 -
Flags: review?(bzbarsky)
Attachment #179253 -
Flags: review-
Comment 96•20 years ago
|
||
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•20 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•20 years ago
|
||
... except that actually isn't true.
Assignee | ||
Comment 99•20 years ago
|
||
Attachment #179322 -
Attachment is obsolete: true
Assignee | ||
Comment 100•20 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
Closed: 26 years ago → 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 101•20 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•20 years ago
|
Attachment #83764 -
Attachment is obsolete: true
Comment 102•20 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.
Comment 103•20 years ago
|
||
> 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•20 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!
Comment 105•20 years ago
|
||
> Reusing counters on child elements does not work (the 'URL')
Which part of it doesn't work?
Comment 106•20 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•20 years ago
|
||
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•20 years ago
|
||
> 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•20 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•20 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.)
Comment 111•20 years ago
|
||
> 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•20 years ago
|
||
Thanks! Now I understand the "new system" (and I think it's right and more correct).
Comment 113•20 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•20 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.
Comment 115•20 years ago
|
||
> 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•20 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.
Comment 117•20 years ago
|
||
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•19 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•19 years ago
|
Attachment #179566 -
Attachment description: simple testcase → incorrect simple testcase (missing a counter-reset)
Attachment #179566 -
Attachment is obsolete: true
Assignee | ||
Comment 119•19 years ago
|
||
See comment 108.
Comment 120•19 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•19 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•18 years ago
|
||
See also Bug 348278 - which is a regression for counters, even when correctly placed.
Assignee | ||
Comment 123•18 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+
You need to log in
before you can comment on or make changes to this bug.
Description
•