Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS Counters - Explained the differences in browsers behavior with respect to CSS Counters. #13513

Closed
wants to merge 19 commits into from

Conversation

debiru
Copy link
Contributor

@debiru debiru commented Mar 5, 2022

Summary

Rewritten the Using CSS counter systematically.
And added a link for "Using CSS Counters" and normalize "See also" links.

Motivation

See #13293

Supporting details

Without losing the content of the original description, it has been rewritten in more detail.
In particular, the behavior of Firefox 82 and later was explained.

Current version:

Related issues

See #13293

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

@debiru debiru requested a review from a team as a code owner March 5, 2022 04:18
@debiru debiru requested review from estelle and removed request for a team March 5, 2022 04:18
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label Mar 5, 2022

## Using counters
Counters are essentially variables managed by CSS, not only for their own counters, but also for ordered list numbers, which can be increased or decreased by arbitrary values according to CSS rules, and can be used to define multiple named counters, or to manipulate the list-item counters that is standard generated as ordered lists.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence needs to be split into much smaller parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bb3d763 fix it.

To use a counter it must first be initialized to a value with the {{cssxref("counter-reset")}} property.
The counter's value can then be increased or decreased using {{cssxref("counter-increment")}} property.
The current value of a counter is displayed using the {{cssxref("counter()")}} or {{cssxref("counters()")}} function, typically within a [pseudo-element](/en-US/docs/Web/CSS/Pseudo-elements) {{CSSxRef("content")}} property.
Counters can be used by using {{cssxref("counter-reset")}}, {{cssxref("counter-increment")}} and {{cssxref("counter-set")}} properties, and {{cssxref("counter()")}} and {{cssxref("counters()")}} functions as a value of {{cssxref("content")}} property. For reversed counters, the `reversed()` function can also be used as a value of `counter-reset` property.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Counters can be used by using" - hard to read. Maybe counters can be used by "applying" ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9dd98a6 fix it.

@hamishwillee
Copy link
Collaborator

I'm not reviewing this properly as I'm not an expert.

That said, I do prefer this structure for the using guide. Some of the English could be improved a little - in particular if a sentence ends up being a 3 or 4 line paragraph split it into short distinct sentences.

One thing I particularly like is that I never understood why counter-set was useful. I.e. it was clear that the counter-reset creates a new counter while counter-set reuses an existing counter, but it wasn't obvious why it was useful to be able to do so (i.e. why not just create a new counter every time?). From this doc update I infer that you might use one counter across a bunch of sections (created with counter-reset) but then maybe set it if you need specific behaviour in a particular section so you can use counter-set for that section. Still not really sure why you wouldn't just create a new counter for the problematic section :-).
This is still not obvious in the counter-set doc too (and maybe it should be)

@debiru
Copy link
Contributor Author

debiru commented Mar 7, 2022

Thank you for your comnent. It is very helpful for me.

I have tried to fix it, but what do you think? In addition to the inline remarks, I also add a sentence about counter-set by 472be74.

@hamishwillee
Copy link
Collaborator

Hi @debiru

Thank you. I will take another look tomorrow when I am fresh. I am also hoping someone with more expertise around these APIs can also review. Though I do have some sort of understanding, since I did the recent iteration on reversed counters. This is why I am very happy you have updated the structure, because this is better.

@debiru
Copy link
Contributor Author

debiru commented Mar 7, 2022

Hi @hamishwillee

Thank you for your message. I have done my own self-review and made minor fixation.

If you want to see your changes immediately, you can preview them at your local environment using yarn, which is the method for editors. See step 6 of the docs https://github.com/mdn/content#more-substantial-changes. Of course, you can wait for the automatic preview to reflect. Please use your preferred method.

- {{cssxref("counter()")}} and {{cssxref("counters()")}} functions
- {{cssxref("content")}} property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was content removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, Using_CSS_counters, counter-reset, conuter-increment, counter-set, counter() and counters() descriptions were inconsistent (did not normalized).

The content property was excluded from the counter-reset and counter-set pages because it is not directly related to the CSS Counter. On the other hand, it is kept in the counter() and counters() functions.

If you have a suggestion that it should be written, I will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bdaa073 added content link.

Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only just started the review. Only one question on the see also edits, but need to read the whole "using CSS counters", which I haven't had time to finish yet.

@debiru
Copy link
Contributor Author

debiru commented Mar 9, 2022

Thank you for reviewing this. Especially, English sentences may have many mistakes because I am Japanese and not good at English.

I know this may be difficult since the document is long, but it would be helpful if you could review it. Once the review is complete I will make the revisions.

@estelle
Copy link
Member

estelle commented Mar 10, 2022

Thank you for taking the time to re-work all the counter documentation.

I think the main reason for your edits to the using CSS counters document was to highlight:

To make this happen, CSS must be written in combination with {{cssxref("counter-set")}} as follows. The reason is that {{cssxref("counter-reset")}} creates a new scope in Firefox 82 and later, when the specification was changed as described below, so it is not possible to reset the counter in the current scope again. To reset the counter for the current scope, you have to use counter-set.

However, there is another issue to consider. In fact, Safari and Safari on iOS have delayed counter-set support. If you want to match the behavior across all browsers, you need to use counter-set first, and apply counter-reset to only Safari. The following code does not take Safari into account.

This would be a great addition to BCD (browser compatibility data), and can be addressed as a note or warning in the documentation, or even subsection with a header. BCD data for CSS properties are located at https://github.com/mdn/browser-compat-data/blob/main/css/properties/

MDN documentation is geared towards both experienced developers and people just learning the topic with little to no experience. I think the original is easier to follow for those who have little experience. I encourage you to revert back to the original "Using CSS counter", amending the content to reflect modern specifications and correct implementation, then adding a note or section on how to ensure non-supporting browsers get the right results in a way that can easily be removed when Safari updates their implementation.

Also:

I am Japanese and not good at English.

Your English is excellent!

Thank you for contributing.

@debiru
Copy link
Contributor Author

debiru commented Mar 10, 2022

I know BCD. This issue is not about whether or not the specification is supported. The issue was the lack of information about how Firefox 82 or later differs from other browsers in behavior that is not specified in the specification.

However, there is another issue to consider. In fact, Safari and Safari on iOS have delayed counter-set support. If you want to match the behavior across all browsers, you need to use counter-set first, and apply counter-reset to only Safari. The following code does not take Safari into account.

This statement was inappropriate because it was information at a certain point in time. It should have read something like "If counter-set is not supported, counter-reset must be used. This paragraph is unnecessary and should be deleted. refs. fdd9b24

MDN documentation is geared towards both experienced developers and people just learning the topic with little to no experience. I think the original is easier to follow for those who have little experience. I encourage you to revert back to the original "Using CSS counter", amending the content to reflect modern specifications and correct implementation, then adding a note or section on how to ensure non-supporting browsers get the right results in a way that can easily be removed when Safari updates their implementation.

Please forget about the Safari description. That was inappropriate. Would a rewritten document be difficult to read for someone without knowledge? I do not know for whom the MDN documentation should be written.

@estelle
Copy link
Member

estelle commented Mar 10, 2022

Long format documentation which explains how to implement things, is written with the learner in mind, at a 3rd grade to 8th-grade reading level, always keeping people in mind who may have cognitive and learning differences.

@debiru
Copy link
Contributor Author

debiru commented Mar 14, 2022

Hi @estelle

MDN documentation is geared towards both experienced developers and people just learning the topic with little to no experience. I think the original is easier to follow for those who have little experience. I encourage you to revert back to the original "Using CSS counter", amending the content to reflect modern specifications and correct implementation, then adding a note or section on how to ensure non-supporting browsers get the right results in a way that can easily be removed when Safari updates their implementation.

I threw away the previous document and rewrote it anew. I have rewritten the document based on the structure of the original document, filling in the missing content. I believe that the rewrite has been done while maintaining the level of the intended audience.

Would this still be considered something of a problem? I would like to hear your honest opinion. Also, suggestions for errors in the English language are welcome.

@debiru
Copy link
Contributor Author

debiru commented Mar 30, 2022

I don't know the process for merge requests in this repository, but is it possible to get anyone to review it as it is? Please let me know what I need to do.

@hamishwillee
Copy link
Collaborator

Thanks for the ping @debiru . Sorry for the delay in review - we're all busy on different things and counters a kind of hard (at least in my opinion). I'll see if I can drag in some expertise. Feel free to ping again if you don't see any response by mid next week.

@debiru
Copy link
Contributor Author

debiru commented Apr 1, 2022

@hamishwillee

Thank you for your response. It is perfectly acceptable for the response to be delayed due to your busy schedule.

I and other contributors have no knowledge of the situation. We were concerned that the situation would be left for six months or more without any response. It is bad for a contributor's motivation to be left in the dark about the reviewer's or core committer's situation.

In this case, since the situation was explained to us, there was no problem.
Thanks.

@hamishwillee
Copy link
Collaborator

Hi @debiru

The best way to get this reviewed would be to separate the factual updates and structural improvements. As @estelle suggested here #13513 (comment) : adding a warning and update to BCD is relatively clearly of value, but because it is constrained it is easy to review.

I would start again from scratch and just add that information.

The structural changes are always going to be harder to get review for. Keeping them separate at least doesn't block the key content going in.

@debiru
Copy link
Contributor Author

debiru commented Apr 4, 2022

Hi @hamishwillee

I would like to confirm, are you saying this after reading my second rewrite of the document?

My first rewrite was indeed a mixture of structural changes and information additions. I would write about adding information to the BCD if I could, but there were two problems.

  • That information cannot be evaluated on a Yes/No basis because it is not about whether the specification is met, but about behavior undefined in the specification.
  • I can't create a PullReq because I don't know how to apply such information to the BCD.

Therefore, in my second rewrite, I added additional information in additional sections based on the improved structure.

I would start again from scratch and just add that information.

By the way, what does scratch mean? Is it a blank page? Or is it the current version, before I edited it?

If you are asking me to make further modifications, would it be possible for you to advise me more specifically? I apologize for the inconvenience caused by my inability to help you.

@hamishwillee
Copy link
Collaborator

Hi @debiru

are you saying this after reading my second rewrite of the document?

No. I have not reread the second rewrite - I scanned it and just saw "lots of change". I haven't given it a thorough review because there is no point if Estelle isn't happy.

I think you are saying that your rewrite is not so much "lots of change", but the minimal change needed to explain the browser differences. Is that right?

By the way, what does scratch mean? Is it a blank page? Or is it the current version, before I edited it?

It means start from the page "as it is now on MDN" and make the minimal changes needed to record the changed behaviour.

Let's see what @estelle says. Generally the less change there is from the original, the easier it is for us to review the change. If she says nothing I'll look at her comments tomorrow and think about how I might do the changes.

@debiru
Copy link
Contributor Author

debiru commented Apr 4, 2022

Hi @hamishwillee

My first rewrite was certainly difficult to get through the review.

In my second rewrite, I have taken note of what @estelle said and added the information while keeping the current structure.

I think you are saying that your rewrite is not so much "lots of change", but the minimal change needed to explain the browser differences. Is that right?

I have rewritten it from two perspectives.

  • To systematically explain the use of properties related to css counter.
  • To explain what is happening with the change in counter-reset behavior in Firefox 82.

The result of this work is the current difference. I don't mean to add any more extraneous explanations than necessary.

If the second rewrite does not pass review, then I will consider a third rewrite as you said.

ol { counter-reset: num list-item; }
```

Firefox 68 and later, which do not apply implicit `counter-reset: list-item`, are affected by this issue.
Copy link
Collaborator

@hamishwillee hamishwillee Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go into mdn/browser-compat-data#15666 or a subsequent patch.

I don't understand it though - can you point me to an issue number or implementation change?

2-4-2. (2-2-1)
```

Note that before {{cssxref("counter-set")}} was introduced, this case was implemented using {{cssxref("counter-reset")}}. Firefox 82 and later, which implement strict scoping, are affected by this issue.
Copy link
Collaborator

@hamishwillee hamishwillee Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually confusing. It is hard to work out what is expected and what is not.
I think we need to clarify how counter-set works by contrast. You may have defined strict scoping too. I'll come back to this once the BCD stuff works through.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Apr 5, 2022

@debiru Thanks for your patience. I'm going to try work through this the way that I always sort out these problems - I can't deconstruct what you have done. Having "some" understanding of the issues now I think I agree with Estelle - there is too much going on in this PR to work out whether the changes are helpful or not. I think the information will definitely useful, but we should integrate it carefully. EDIT -note, I might just be a bit stupid of course :-). I tend to work through these things methodically.

The first step in that is to capture all browser-specific behaviour in browser compatibility data. Generally we try and avoid having specific comments about browser behaviors in the MDN text itself if at all possible. So as a first step I have created mdn/browser-compat-data#15666 which captures the incompatibility with the spec for all browsers (and FF prior to FF82) related to how counter-reset affects sibling elements.

The reason I do this is that this ensures that there is a clear understanding of what the breaks are and where. The BCD team are pretty careful about that, so the issue won't get through if the problem isn't clearly stated.

When that's done, I would update the counter-reset doc:

  1. Make sure the description is correct
  2. Capture this possible misunderstanding on use with an example - after all, this is not how you would normally use counter-reset
  3. Update the interactive example as per Issue with "counter-reset": the example is a bit confusing interactive-examples#2793

Then I'd go to the "Using" doc and update that as necessary.

That's what Estelle means by a discrete change - we're not trying to make the whole thing better, we're trying to isolate the specific issues first.

Anyway, I see #13513 (comment) says that there is some behaviour that Firefox 68 and later have related to the implicit list-item. That will have to go into BCD too.

@debiru
Copy link
Contributor Author

debiru commented Apr 6, 2022

Thanks for your explanation.

Thanks for your patience. I'm going to try work through this the way that I always sort out these problems - I can't deconstruct what you have done. Having "some" understanding of the issues now I think I agree with Estelle - there is too much going on in this PR to work out whether the changes are helpful or not. I think the information will definitely useful, but we should integrate it carefully. EDIT -note, I might just be a bit stupid of course :-). I tend to work through these things methodically.

OK. I will proceed with the third rewrite to make really small differences. Then I will strike out all edited differences up to the my second rewrite. I also understood that compatibility information should not be included in the text.

First, let me consider updating the BCD table.

I will comment so far once.

Thanks.

@hamishwillee
Copy link
Collaborator

Sounds like a great plan.

@debiru
Copy link
Contributor Author

debiru commented Apr 8, 2022

@hamishwillee I appreciate it.

  • But what I think you are saying is that you will try and characterise the "Firefox 68 and later have related to the implicit list-item." stuff as a BCD entry too.

That's certainly true. My rewrite was triggered by the use_reset_for_siblings issue, but then I also noticed the related to the implicit list-item issue.

I wrote these bug reports, but both were my mistakes, and the Firefox implementation was correct. I decided to do a rewrite of the MDN to make these facts known to many people. However, my lack of knowledge of MDN's documentation rewrite practices resulted in this Pull-Req of the "Using" document.

I really appreciate you taking a look at this Pull-Req.

@hamishwillee
Copy link
Collaborator

Note, not forgotten this. Just still below my other priorities.

@debiru
Copy link
Contributor Author

debiru commented Jul 19, 2022

I think this Pull-Req should be rejected once.

It would be simpler to create a new diff, ignoring the changes made in this Pull-Req, even if we will modify this pages.

Conflicts have also been caused by other modifications. It is time-consuming to resolve conflicts again and again.

@estelle estelle closed this Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants