Skip to content
New issue

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

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

Already on GitHub? # to your account

Uncomment keys and compute_from (features A-C) #1400

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Jul 16, 2024

I'm running a little experiment here: I've searched for keys we've commented out and started uncommenting them and adding a compute_from override. This is what it looks like for the first 8 features (found in my editor with the search for # in features/{a,b,c}*.yml).

@github-actions github-actions bot added the feature definition Creating or defining new features or groups of features. label Jul 16, 2024
Comment on lines +35 to +37
- api.CSSCounterStyleRule.speakAs
- api.CSSCounterStyleRule.suffix
- api.CSSCounterStyleRule.symbols
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is by far the weirdest bit. I don't know if we should be troubled by properties for unsupported descriptors, but it's at least true (it made me stop to check that these are indeed on CSSCounterStyleRule.prototype).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's surprising that this has been exposed in CSSOM in both Chrome and Firefox if they don't actually work. But if that's true, I think they ought to be partial_implementation in BCD. I think it'd be best to not include the keys until this is investigated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked at this more closely.

For symbols, the CSSOM story is accurate: the CSSOM exposure works in Firefox and Chrome, but the descriptor is only partially implemented. Implementers other than Safari don't support <image> values, and this seems to be correct.

For speak-as, I've sent mdn/browser-compat-data#23857.

We can wait on the latter PR, or I can comment out the key until it lands. Do you have a preference (and maybe we should turn that preference into a policy)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think waiting for BCD slows us down and we need a policy that allows us to think about these things once. And the risk to minimize is the risk that whatever we assume will happen doesn't happen, and we never notice we got it wrong.

I think these won't be Baseline anyway, so OK to just proceed,

@@ -20,9 +20,8 @@ status:
safari_ios: "13.4"
compat_features:
- api.Clipboard
# TODO: include after https://github.com/mdn/browser-compat-data/pull/23593
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling we should keep this comments, and others like it. There's some good research in there, which I would hate to lose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can see that. In the interest of consolidating all of untangling of this feature, I've updated and linked to #1249 with 81296e3.

compat_features:
- api.CSSStyleSheet.CSSStyleSheet
- api.CSSStyleSheet.replace
- api.CSSStyleSheet.replaceSync
- api.Document.adoptedStyleSheets
- api.ShadowRoot.adoptedStyleSheets
# Setting the base URL is probably not important for typical uses of
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, and elsewhere. It would be nice if we had a way to keep these editorial notes in there somewhere, maybe in a more structured form.
It would be nice to see them in the dist files in fact.What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be nice if we had a way to keep these editorial notes in there somewhere, maybe in a more structured form.

I agree that it would be nice, though I think compute_from is going to handle the most common case, by marking the boundary after which things are later additions. So I'm doubtful that every comment saying "this is a later addition" will make sense to maintain.

Moreover, a lot of compat_features lists will eventually live as tags in BCD. So we'd need a separate way to list compat key commentary. I think we'd need some whole new way to annotate keys not directly referenced in authored YAML files. I'd be open to that, but I also don't have any ideas about what that would look like. Do you want to file an issue about this?

And in the meantime, do you want me to preserve this comment specifically?

It would be nice to see them in the dist files in fact.

This is the one bit I'm not especially keen on. We used to do just that and it meant that every single change to a feature YAML required a refresh. People didn't like it! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that just having a comment that says "later addition" isn't useful. I'm more concerned about the various links and pieces of info that we spent time digging up when researching a feature.

Copy link
Contributor

@captainbrosset captainbrosset left a comment

Choose a reason for hiding this comment

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

Looks good to me for these features.
I don't think my proposal to have structured comments for BCD keys is practical. But I do think that we should continue to document our findings as comments, and probably put them near compute_from, for the more complicated cases.

@@ -56,6 +56,23 @@ compat_features:
# safari_ios: "13.4"
- api.ClipboardItem.ClipboardItem

# baseline: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong, because mdn/browser-compat-data#23593 hasn't reached us through a BCD release yet. Let's wait for that and not publish an incorrect status in the interim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed with e322f9f and 9d97766.

Comment on lines +35 to +37
- api.CSSCounterStyleRule.speakAs
- api.CSSCounterStyleRule.suffix
- api.CSSCounterStyleRule.symbols
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's surprising that this has been exposed in CSSOM in both Chrome and Firefox if they don't actually work. But if that's true, I think they ought to be partial_implementation in BCD. I think it'd be best to not include the keys until this is investigated.

@foolip foolip merged commit 11389cc into web-platform-dx:main Jul 19, 2024
3 checks passed
@ddbeck ddbeck deleted the uncomment-a-c branch July 19, 2024 11:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
feature definition Creating or defining new features or groups of features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants