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

Add patch for ed/css/css-mixins.json #1288

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Add patch for ed/css/css-mixins.json #1288

merged 1 commit into from
Jul 12, 2024

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Jul 12, 2024

I'm not sure what approach we actually want to take here. This namespaces the <combinator> production to css-mixins because that's easy to do while the spec gets fixed. We might prefer to drop the problematic constructs altogether. Or simply to discard the extract for now since it did not exist before (as suggested in #1055).

Namespace `<combinator>` production to css-mixins
@tidoust tidoust requested a review from dontcallmedom July 12, 2024 07:00
@dontcallmedom
Copy link
Member

I was just looking into this, and I think the problem is actually a bit different: <combinator> should not be classified as a CSS type in the Selectors spec - it is describing the grammar to parse selectors, not an actual CSS type. That's true of all the values in the Grammar section of that spec.

Now the spec does use dfn-type=type for them, but that seems at odds with the dfn contract.

@tidoust
Copy link
Member Author

tidoust commented Jul 12, 2024

I'm not clear that this is the only place where CSS specs mix grammar levels. At least, I don't have a clear view of what a "CSS type" is in practice?

@dontcallmedom
Copy link
Member

dontcallmedom commented Jul 12, 2024

leaving aside what bikeshed refers as a CSS type, clearly here, <combinator> in CSS selectors is not something you can use on the right hand side of a property declaration (i.e. https://drafts.csswg.org/css-values-4/#component-types) - which it is in css-mixins

@tidoust
Copy link
Member Author

tidoust commented Jul 12, 2024

Isn't that the case for various other constructs, including at-rules? For example, https://drafts.csswg.org/css-syntax-3/#block-contents

@dontcallmedom
Copy link
Member

to be clear: I'm not arguing this is not a problem that exists in other places; I'm arguing that for this particular duplicate, this points strongly having css-mixins be the one that matches our understanding of what should appear as a type in our JSON extracts (i.e. something you can use a rhs property token).

This probably points towards cleaning our extracts more thoroughly; I'm not entirely sure what it entails in terms of patching this narrow issue, and your patch is a probably reasonable short-term fix, but I think we ought to converge on the underlying issue to make sure we know at least what to investigate next

@tidoust
Copy link
Member Author

tidoust commented Jul 12, 2024

Right, but in other places we tend to stick to what specs define even if our understanding is that it's wrong (c.f. yesterday's patch on Web Audio API event). The mixins spec was added last, I wouldn't favor it.

Side note: there is a schema issue in Reffy related to algorithms (the file schema is invalid). That makes CI tests fail. There are also a couple of algorithms extracts that have an unexpected null URL and that don't pass validation even once the schema has been fixed. I'll look into it.

@dontcallmedom
Copy link
Member

dontcallmedom commented Jul 12, 2024

I don't think the comparison with the Web Audio spec matches what I was trying to convey: the Selectors spec doesn't at all imply that <combinator> represents something that can be used on the RHS of a property value; when we define it as a type in selectors.json, we wrongfully give that interpretation.

IOW, the items defined in section 18 of the selectors spec are in an entirely different namespace than what (my understanding of) our types in JSON files are; so it feels somewhat wrong to amend the one that is in the proper namespace. And again, I don't object to doing it as a short term fix, but I want to make sure we agree on the semantics (but maybe that'd be better done in a separate issue)

@tidoust
Copy link
Member Author

tidoust commented Jul 12, 2024

Restricting CSS types to things that can be used on the RHS of a property value is more your interpertation than our interpretation. I've always approached these constructs as describing the CSS grammar as a whole. Within that grammar, it may make sense to distinguish between value types and other constructs, but that's not how CSS specs are currently written as far as I can tell.

Or is "CSS type" defined somewhere? Bikeshed's doc is evasive. The different css-syntax, css-values, css-typed-om specs seem to use "type" in a loose way too.

@dontcallmedom
Copy link
Member

Ok, so we don't have a shared model yet, which is definitely worth an issue.

My reading of type was I think https://drafts.csswg.org/css-values-3/#component-types .

I do feel strongly that we have a bug: we are patching the extract, but I don't think we can report it as a bug to the CSS spec (or expect it to be solved over spec progress) since I don't think we can argue the two combinator are in the same namespace, so they're not actual duplicates.

Let's merge this to unblock curation and open a separate issue on the model

Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

follow up issue in #1289

@tidoust tidoust merged commit 7f16282 into main Jul 12, 2024
1 check failed
@tidoust tidoust deleted the patch-20240712065501342 branch July 12, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants