From d7e6743d2641183b05ff66f3a6c820f275bc4460 Mon Sep 17 00:00:00 2001 From: Francois Daoust Date: Tue, 22 Jul 2025 09:58:47 +0200 Subject: [PATCH 1/2] Force add CSS functions/types that do not have a proper dfn When a function or type was found in a production rule that does not have a ``, it was added to the list of "missing definitions". In practice, the only cases where that happens are: - When a spec extends a construct defined elsewhere, see #1647. In such cases, we'd want the extended type definition to appear in the extract. - For ``, see #1878. In such cases, we'd rather not add the type since the underlying spec needs fixing, but that can be caught automatically, see below. This code forces the creation of a function/type. This is breaking change because, up until now, we guaranteed that all entries would have an `href` and that will no longer be the case. Ideally, we'd restrict this logic to extensions, but production rules are processed as raw text, meaning we don't have the link information that would tell us that we're looking at an extension. That is not such a big deal though: it is easy to tell when an entry without an `href` extends another, and it seems fine to use patching to curate the data (only one occurrence today). CSS consolidation is updated to deal with extended functions and types. As opposed to extensions of properties where new values are added to the syntax, function/type extensions re-define the entire syntax. --- src/browserlib/extract-cssdfn.mjs | 27 ++++++++----- src/postprocessing/cssmerge.js | 32 ++++++++++++---- test/extract-css.js | 19 ++++++++-- test/merge-css.js | 63 +++++++++++++++++++++++++++++-- 4 files changed, 119 insertions(+), 22 deletions(-) diff --git a/src/browserlib/extract-cssdfn.mjs b/src/browserlib/extract-cssdfn.mjs index 7b02da08..e61aa1a6 100644 --- a/src/browserlib/extract-cssdfn.mjs +++ b/src/browserlib/extract-cssdfn.mjs @@ -184,16 +184,25 @@ export default function () { } if (matchingValues.length === 0) { // Dangling production rule. That should never happen for properties, - // at-rules, descriptors and functions, since they should always be - // defined somewhere. That happens from time to time for types that are - // described in prose and that don't have a dfn. One could perhaps argue - // that these constructs ought to have a dfn too. - if (!res.warnings) { - res.warnings = [] + // at-rules, descriptors: they should always be defined somewhere. That + // happens from time to time for functions and types that are defined + // in a spec and (temporarily) extended in another spec. + if (rule.name.match(/^<.*>$/)) { + const isFunction = !!rule.name.match(/\(\)/); + res.values.push({ + name: isFunction ? rule.name.replace(/^<(.*)>$/, '$1') : rule.name, + type: isFunction ? 'function' : 'type', + value: rule.value + }); + } + else { + if (!res.warnings) { + res.warnings = []; + } + const warning = Object.assign({ msg: 'Missing definition' }, rule); + warnings.push(warning); + rootDfns.push(warning); } - const warning = Object.assign({ msg: 'Missing definition' }, rule); - warnings.push(warning); - rootDfns.push(warning); } } } diff --git a/src/postprocessing/cssmerge.js b/src/postprocessing/cssmerge.js index af199322..652eb9e0 100644 --- a/src/postprocessing/cssmerge.js +++ b/src/postprocessing/cssmerge.js @@ -111,8 +111,9 @@ export default { // The job is "almost" done but we now need to de-duplicate entries. // Duplicated entries exist when: - // - A property is defined in one spec and extended in other specs. We'll - // consolidate the entries (and syntaxes) to get back to a single entry. + // - A property, function or type is defined in one spec and extended in + // other specs. We'll consolidate the entries (and syntaxes) to get back to + // a single entry. // - An at-rule is defined in one spec. Additional descriptors are defined // in other specs. We'll consolidate the entries similarly. // - A descriptor is defined in one level of a spec series, and re-defined @@ -179,13 +180,22 @@ export default { // Identify the base definition for each feature, using the definition // (that has some known syntax) in the most recent level. Move that base // definition to the beginning of the array and get rid of other base - // definitions. - // (Note: the code chooses one definition if duplicates of base - // definitions in unrelated specs still exist) + // definitions. Notes: + // - For properties, an extension is an entry with a `newValues` key. + // - For functions and types, an extension is an entry without an `href` + // key (the spec that extends the base type does not contain any ``) + // - The code chooses one definition if duplicates of base definitions in + // unrelated specs still exist. for (const [name, dfns] of Object.entries(featureDfns)) { - let actualDfns = dfns.filter(dfn => dfn.syntax); + let actualDfns = dfns.filter(dfn => dfn.href && dfn.syntax); if (actualDfns.length === 0) { - actualDfns = dfns.filter(dfn => !dfn.newValues); + actualDfns = dfns.filter(dfn => dfn.href && !dfn.newValues); + } + if (actualDfns.length === 0) { + // No base definition, not a real type, let's discard it + // (problem should be captured through tests on individual extracts) + delete featureDfns[name]; + continue; } const best = actualDfns.reduce((dfn1, dfn2) => { if (dfn1.spec.series.shortname !== dfn2.spec.series.shortname) { @@ -240,6 +250,14 @@ export default { } baseDfn.syntax += ' | ' + dfn.newValues; } + else if (dfn.syntax) { + // Extensions of functions and types are *re-definitions* in + // practice, new syntax overrides the base one. There should be + // only one such extension in unrelated specs, the code assumes + // that some sort of curation already took place, and picks up + // a winner randomly. + baseDfn.syntax = dfn.syntax; + } if (baseDfn.descriptors && dfn.descriptors?.length > 0) { baseDfn.descriptors.push(...dfn.descriptors.filter(desc => !hasNewerDescriptorDfn(desc, dfn, dfns))); diff --git a/test/extract-css.js b/test/extract-css.js index 8a9b76b5..5d399af6 100644 --- a/test/extract-css.js +++ b/test/extract-css.js @@ -1010,16 +1010,29 @@ that spans multiple lines */ ] }, + { + title: 'creates a definition when one is missing for a type', + html: ` + 
<my-type> = none | auto
+ `, + propertyName: 'values', + css: [{ + name: '', + type: 'type', + value: 'none | auto' + }] + }, + { title: 'issues a warning when a definition is missing', html: ` - 
<my-type> = none | auto
+    
foo = bar

     `,
     propertyName: 'warnings',
     css: [{
       msg: 'Missing definition',
-      name: '',
-      value: 'none | auto'
+      name: 'foo',
+      value: 'bar'
     }]
   },
 
diff --git a/test/merge-css.js b/test/merge-css.js
index 1c8aca53..051e6c93 100644
--- a/test/merge-css.js
+++ b/test/merge-css.js
@@ -54,7 +54,7 @@ const descriptorBase = {
   type: 'discrete'
 };
 
-const descriptorExtended = Object.assign({}, descriptorBase, {
+const descriptorExtension = Object.assign({}, descriptorBase, {
   href: 'https://drafts.csswg.org/css-stuff-2/#descdef-descriptor',
   value: 'extended'
 });
@@ -77,6 +77,7 @@ const property1 = {
 
 const propertyLegacy = {
   name: 'good-old-overlay',
+  href: 'https://compat.spec.whatwg.org/#good-old-overlay',
   legacyAliasOf: 'overlay'
 };
 
@@ -93,6 +94,12 @@ const type1 = {
   value: 'repeat | space | round | no-repeat'
 };
 
+const type1Extension = {
+  name: '',
+  type: 'type',
+  value: 'bis repetita'
+};
+
 const functionVar = {
   name: 'var()',
   href: 'https://drafts.csswg.org/css-variables-2/#funcdef-var',
@@ -432,7 +439,7 @@ describe('CSS extracts consolidation', function () {
         css: Object.assign({}, emptyExtract, {
           atrules: [
             Object.assign({}, atrule2, {
-              descriptors: [descriptorExtended]
+              descriptors: [descriptorExtension]
             })
           ]
         })
@@ -442,7 +449,7 @@ describe('CSS extracts consolidation', function () {
     assert.deepEqual(result.atrules, [
       conv(Object.assign({}, atrule2, {
         syntax: '@media foo',
-        descriptors: [descriptorExtended]
+        descriptors: [descriptorExtension]
       }))
     ]);
   });
@@ -630,4 +637,54 @@ describe('CSS extracts consolidation', function () {
       ]
     })));
   });
+
+  it('merges extended types', async () => {
+    const results = structuredClone([
+      {
+        shortname: 'css-stuff-1',
+        series: { shortname: 'css-stuff' },
+        seriesVersion: '1',
+        css: Object.assign({}, emptyExtract, {
+          values: [
+            Object.assign({}, type1)
+          ]
+        })
+      },
+      {
+        shortname: 'css-otherstuff-1',
+        series: { shortname: 'css-otherstuff' },
+        seriesVersion: '1',
+        css: Object.assign({}, emptyExtract, {
+          values: [
+            Object.assign({}, type1Extension)
+          ]
+        })
+      },
+    ]);
+    const result = await cssmerge.run({ results });
+    assert.deepEqual(result, conv(Object.assign({}, emptyMerged, {
+      types: [
+        Object.assign({}, conv(type1), {
+          syntax: type1Extension.value
+        })
+      ]
+    })));
+  });
+
+  it('discards type extensions without a base definition', async () => {
+    const results = structuredClone([
+      {
+        shortname: 'css-stuff-1',
+        series: { shortname: 'css-stuff' },
+        seriesVersion: '1',
+        css: Object.assign({}, emptyExtract, {
+          values: [
+            Object.assign({}, type1Extension)
+          ]
+        })
+      }
+    ]);
+    const result = await cssmerge.run({ results });
+    assert.deepEqual(result, conv(emptyMerged));
+  });
 });

From 37830087a0a40f58cb8799d0ff2ea83ac2861d2a Mon Sep 17 00:00:00 2001
From: Francois Daoust 
Date: Fri, 25 Jul 2025 16:12:28 +0200
Subject: [PATCH 2/2] Fix merge

Merge operation duplicated a few lines of code...
---
 src/postprocessing/cssmerge.js | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/src/postprocessing/cssmerge.js b/src/postprocessing/cssmerge.js
index 0277e344..f939b643 100644
--- a/src/postprocessing/cssmerge.js
+++ b/src/postprocessing/cssmerge.js
@@ -263,14 +263,6 @@ export default {
             baseDfn.syntax = dfn.syntax;
             baseDfn.extended = [dfn.spec.crawled ?? dfn.spec.url];
           }
-          else if (dfn.syntax) {
-            // Extensions of functions and types are *re-definitions* in
-            // practice, new syntax overrides the base one. There should be
-            // only one such extension in unrelated specs, the code assumes
-            // that some sort of curation already took place, and picks up
-            // a winner randomly.
-            baseDfn.syntax = dfn.syntax;
-          }
           if (baseDfn.descriptors && dfn.descriptors?.length > 0) {
             baseDfn.descriptors.push(...dfn.descriptors.filter(desc =>
               !hasNewerDescriptorDfn(desc, dfn, dfns)));