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

Synchronize with BCD v5.5.14 #32594

Closed
wants to merge 4 commits into from
Closed

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar requested review from a team as code owners March 7, 2024 00:26
@OnkarRuikar OnkarRuikar requested review from wbamberg, teoli2003 and dipikabh and removed request for a team March 7, 2024 00:26
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs Content:WebAPI Web API docs Content:HTTP HTTP docs Content:Manifest size/m [PR only] 51-500 LoC changed labels Mar 7, 2024
Copy link
Contributor

github-actions bot commented Mar 7, 2024

Preview URLs (43 pages)
Flaws (18)

Note! 40 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/CSS/ruby-align
Title: ruby-align
Flaw count: 1

  • macros:
    • /en-US/docs/Web/CSS/ruby-merge does not exist

URL: /en-US/docs/Web/API/Element
Title: Element
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/Element/getBoxQuads does not exist

URL: /en-US/docs/Web/API/RTCOutboundRtpStreamStats
Title: RTCOutboundRtpStreamStats
Flaw count: 16

  • macros:
    • /en-US/docs/Web/API/RTCSentRtpStreamStats does not exist
    • /en-US/docs/Web/API/RTCOutboundRtpStreamStats/qualityLimitationDurations does not exist
    • /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats does not exist
    • /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats does not exist
    • /en-US/docs/Web/API/RTCOutboundRtpStreamStats/retransmittedBytesSent does not exist
    • and 11 more flaws omitted

(comment last updated: 2024-03-12 10:29:25)

files/en-us/web/css/hanging-punctuation/index.md Outdated Show resolved Hide resolved
@@ -53,7 +53,7 @@ The `hanging-punctuation` property may be specified with one, two, or three spac

### Values

- `none`
- `none` {{experimental_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

However also it looks like the script here is marking every value of the property experimental, rather than the main property itself. That doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bot simply mimics BCD. It's a bug in BCD:
table

The property should be marked experimental in BCD then the bot will reflect it in the page baner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asking the question again which got wrongfully made in review comment below.

@Elchi3 why the hanging-punctuation property was made non-experimental and not its values? At least one value should've been non-experimental right?

@@ -32,31 +32,31 @@ margin-trim: unset;

### Values

- `none`
- `none` {{experimental_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here again, if the property itself is experimental, why mark all the values experimental?

Copy link
Contributor Author

@OnkarRuikar OnkarRuikar Mar 7, 2024

Choose a reason for hiding this comment

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

Technically, because BCD tracks it like that: all the values have a separate status tracked.

BCD table

The bot simply mimics the BCD i.e. track property(page header) and values(inline badges) separately.

Pros of separate tracking:

  1. Keeps the implementation simple. Keeps BCD the single source of truth.
  2. Third party scraping tools won't have to be intelligent too. Many popular IDEs use MDN snippets as it is in tool tips.
  3. Readers landed from search engines directly on the property won't have to scroll up to see if it's experimental.
  4. Reviews will become complex as in localised diff a value, that remained experimental after the property becomes non experimental, will look like going non-experimental to experimental.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this is different from the decision that was made about secure context: https://github.com/orgs/mdn/discussions/482#discussioncomment-7796301. Why are we inconsistent here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are we inconsistent here?

Does the inline secure context discussion and decision apply to all the inline macros? Putting this in weekly meeting agenda.

browser-compat: css.properties.content-visibility
---

{{CSSRef}}{{SeeCompatTable}}
{{CSSRef}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and in many other cases, the main property is not marked experimental, but all its values are. What does this mean?

@@ -35,7 +35,7 @@ The keyword value `normal`, or a `<number>` optionally followed by an `<integer>

### Values

- `normal`
- `normal` {{experimental_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, the main property is marked experimental, but not all its values are.

Copy link
Contributor Author

@OnkarRuikar OnkarRuikar Mar 9, 2024

Choose a reason for hiding this comment

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

This is a BCD issue. @Elchi3 why the property was made non-experimental and not its values? At least one value should've been non-experimental right?

Copy link
Member

@Elchi3 Elchi3 Mar 11, 2024

Choose a reason for hiding this comment

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

This is a BCD issue.

Not sure what this has to do with initial-letter.

@Elchi3 why the property was made non-experimental and not its values? At least one value should've been non-experimental right?

The initial-letter property is supported in Chrome and Safari, so it is not experimental. (although, given it is only supported with a prefix in Safari, one could argue). The normal property is likely also supported in Safari and not just Chrome, so this is the bug in BCD. Maybe @queengooborg did not take into account prefixes when adding CSS property values?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefixes are not tracked by the mdn-bcd-collector, so any prefix-based data was recorded manually. I've just submitted mdn/browser-compat-data#22596 to correct the Safari data for the normal value.

In regards to the experimental status, actually, the initial-letter property is accurately marked as experimental. Per the experimental status guidelines, prefixes are basically ignored when picking an experimental status. Experimental should be set to false if Chrome and Safari both supported the property without a prefix; in this case, only one browser does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this has to do with initial-letter.

Ohh, the statement was for the hanging-punctuation. The comment accidentally got made in the review box below (here).

However, mdn/browser-compat-data#22596 made it confusing for the initial-letter property. It is experimental but its value(s) are not. 😕

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment accidentally got made in the review box below (here).

?? My comment here was for initial-letter, and it is that the property is marked experimental at the top level, but not all its values are. I don't understand this. If the property is experimental "as a whole", how can it have values that are not experimental?

hanging-punctuation has a different issue, that I've also commented on, which is that it is not marked experimental at the top level, but all its values are. If all its values are experimental, how does it make sense for the property itself to be not experimental?

- : The inline contents are aligned to the right edge of the line box.
- `center` {{deprecated_inline}} {{non-standard_inline}}
- `center`
Copy link
Collaborator

Choose a reason for hiding this comment

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

justify-all is marked as experimental in this one but missing in the BCD table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

files/en-us/web/css/text-size-adjust/index.md Outdated Show resolved Hide resolved
@@ -38,7 +38,7 @@ text-decoration-skip: unset;

### Values

- `none`
- `none` {{experimental_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property is marked experimental, not all values are.

Copy link
Contributor Author

@OnkarRuikar OnkarRuikar Mar 9, 2024

Choose a reason for hiding this comment

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

BCD doesn't cover all the values here. The page author(s) should've have added these to BCD. Or we should have some mechanism to report these in BCD.

- : The text is justified by adding space between words (effectively varying {{cssxref("word-spacing")}}), which is most appropriate for languages that separate words using spaces, like English or Korean.
- `inter-character`
- `inter-character` {{experimental_inline}}
- : The text is justified by adding space between characters (effectively varying {{cssxref("letter-spacing")}}), which is most appropriate for languages like Japanese.
- `distribute` {{deprecated_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property is not marked experimental, all values are except this one, which is not listed in BCD.

files/en-us/web/css/timeline-scope/index.md Outdated Show resolved Hide resolved
@@ -32,7 +32,7 @@ view-timeline-inset: 20% 200px;

Allowed values for `view-timeline-inset` are:

- `auto`
- `auto` {{experimental_inline}}
- : If set, the corresponding {{cssxref("scroll-padding")}} (or equivalent longhand value) for that edge of the scrollport is used. If this is not set (or set to `auto`), the value will usually be 0, although some user agents may use heuristics to determine a different default value if appropriate.
- {{cssxref("length-percentage")}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property is marked experimental but this value isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value doesn't exist in BCD.

@@ -26,7 +26,7 @@ view-transition-name: none;

- {{cssxref("custom-ident")}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property is marked experimental but this value isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value doesn't exist in BCD.

@OnkarRuikar
Copy link
Contributor Author

Pinging BCD maintainers those who have worked on CSS props recently @queengooborg @hamishwillee @Elchi3
Common issues are:

  1. If a property is made non-experimental from experimental then at least one value should be also made non-experimental.
  2. There are some values in content that are not tracked in BCD. How can we get them tracked in BCD? Report an issue in BCD whenever the script finds such values?

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Mar 12, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@OnkarRuikar
Copy link
Contributor Author

Closing in favour of #32835

@OnkarRuikar OnkarRuikar deleted the sync_bcd branch March 27, 2024 03:42
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 Content:HTTP HTTP docs Content:Manifest Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants