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.

Sign up for GitHub

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

Ignore dfns that have an invalid type #659

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Ignore dfns that have an invalid type #659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a286a85
Ignore dfns that have an invalid type
tidoust Jun 21, 2021
e1c5b36
Switch invalid `idl` dfn types to `method` when needed
tidoust Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 81 additions & 3 deletions src/browserlib/extract-dfns.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,77 @@ import {parse} from "../../node_modules/webidl2/index.js";
* @return {Array(Object)} An Array of definitions
*/

function definitionMapper(el, idToHeading) {
function normalize(str) {
return str.trim().replace(/\s+/g, ' ');
function normalize(str) {
return str.trim().replace(/\s+/g, ' ');
}

// Valid types defined in https://tabatkins.github.io/bikeshed/#dfn-types
// (+ "namespace" and "event" which are not yet in the doc)
function hasValidType(el) {
const validDfnTypes = [
// CSS types
'property',
'descriptor',
'value',
'type',
'at-rule',
'function',
'selector',

// Web IDL types
'namespace',
'interface',
'constructor',
'method',
'argument',
'attribute',
'callback',
'dictionary',
'dict-member',
'enum',
'enum-value',
'exception',
'const',
'typedef',
'stringifier',
'serializer',
'iterator',
'maplike',
'setlike',
'extended-attribute',
'event',

// Element types
'element',
'element-state',
'element-attr',
'attr-value',


// URL scheme
'scheme',

// HTTP header
'http-header',

// Grammar type
'grammar',

// "English" terms
'abstract-op',
'dfn'
];

const type = el.getAttribute('data-dfn-type') ?? 'dfn';
const isValid = validDfnTypes.includes(type);
if (!isValid) {
console.warn('[reffy]', `"${type}" is an invalid dfn type for "${normalize(el.textContent)}"`);
}
return isValid;
}


function definitionMapper(el, idToHeading) {
let definedIn = 'prose';
const enclosingEl = el.closest('dt,pre,table,h1,h2,h3,h4,h5,h6,.note,.example') || el;
switch (enclosingEl.nodeName) {
Expand Down Expand Up @@ -140,6 +206,18 @@ export default function (spec, idToHeading = {}) {
}

return [...document.querySelectorAll(definitionsSelector)]
.map(node => {
// 2021-06-21: Temporary preprocessing of invalid "idl" dfn type (used for
// internal slots) while fix for https://github.com/w3c/respec/issues/3644
// propagates to all EDs and /TR specs. To be dropped once crawls no
// longer produce warnings.
if (node.getAttribute('data-dfn-type') === 'idl') {
node.setAttribute('data-dfn-type', 'attribute');
Copy link
Member

Choose a reason for hiding this comment

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

I've commented in https://github.com/w3c/respec/issues/3644#issuecomment-864894696 - I'm not convinced attribute is the right approach.

Code looks good otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're still discussing whether it would be worth creating an additional type in speced/spec-dfn-contract#1.

In the meantime, w3c/respec#3644 was closed with ReSpec now flagging internal slots with an attribute type and internal methods with a method type. I've updated the code to the same here and propose to go ahead with this while we converge on new types (or on making these dfns private by default one way or another).

console.warn('[reffy]', `Fixed invalid "idl" dfn type "${normalize(node.textContent)}"`);
}
return node;
})
.filter(hasValidType)
Copy link
Member

Choose a reason for hiding this comment

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

Thinking some more - instead of removing definitions with an invalid type, shouldn't we default them back to dfn?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hesitant to do that. The good thing about removing definitions with an invalid type is that no one can reference them. When someone needs to do that, they will notice that the dfn is missing and raise an issue somewhere to have that fixed, and this should eventually appear on our radar.

If we default these definitions to dfn, people may just ask for a data-export attribute to be added, and then they can start to link to it, probably using a wrong shorthand syntac. We may not notice the issue until both the source spec that defines the term and the spec that references it are wrong.

Obviously, we could notice if we had a proper way to deal with the "warnings" that Reffy logs. I'm not utterly confident that we'll look into the workflow logs, and it's more work to have a better alert mechanism. Useful work but more work... Plus, if we do that, we can just as well remove the dfns: we would have them fixed in the source spec.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I can see that making kind of sense; the risks are probably limited in any case if ReSpec starts flagging unknown definition types the same way bikeshed does.

.map(node => definitionMapper(node, idToHeading));
}

Expand Down
9 changes: 9 additions & 0 deletions src/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,15 @@ async function processSpecification(spec, callback, args, counter) {
await isReady();
});

// Capture and report Reffy's browserlib warnings
page.on('console', msg => {
const text = msg.text();
if (text.startsWith('[reffy] ')) {
console.warn(spec.url, `[${msg.type()}]`, msg.text().substr('[reffy] '.length));
}
});

// Capture and report when page throws an error
page.on('pageerror', err => {
console.error(err);
});
Expand Down
8 changes: 8 additions & 0 deletions tests/extract-dfns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ const tests = [
html: "<dfn data-lt='foo \n |\nbar' id=foo>Foo</dfn>",
changesToBaseDfn: [{linkingText: ["foo", "bar"]}]
},
{title: "ignores dfns with an invalid data-dfn-type",
html: "<dfn id=foo data-dfn-type=invalidtype>Foo</dfn>",
changesToBaseDfn: []
},
{title: "automatically fixes dfns with an invalid 'idl' data-dfn-type",
html: "<dfn id=foo data-dfn-type=idl>Foo</dfn>",
changesToBaseDfn: [{type: "attribute", access: "public"}]
},
{title: "handles HTML spec conventions of definitions in headings",
html: '<h6 id="parsing-main-inselect"><span class="secno">12.2.6.4.16</span> The "<dfn>in select</dfn>" insertion mode<a href="#parsing-main-inselect" class="self-link"></a></h6>',
changesToBaseDfn: [{id: "parsing-main-inselect",
Expand Down