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

fix(core/dfn): treat internal slots as IDL attributes #3646

Merged
merged 23 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
38 changes: 32 additions & 6 deletions src/core/dfn.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Module core/dfn
// - Finds all <dfn> elements and populates definitionMap to identify them.

import { getDfnTitles, norm } from "./utils.js";
import { getDfnTitles, norm, showError } from "./utils.js";
import { registerDefinition } from "./dfn-map.js";

export const name = "core/dfn";
Expand All @@ -12,10 +12,9 @@ export function run() {
const titles = getDfnTitles(dfn);
registerDefinition(dfn, titles);

// Treat Internal Slots as IDL.
if (!dfn.dataset.dfnType && /^\[\[\w+\]\]$/.test(titles[0])) {
dfn.dataset.dfnType = "idl";
}
const [firstTitle] = titles;

associateInternalSlotWithIDL(firstTitle, dfn);

// Per https://tabatkins.github.io/bikeshed/#dfn-export, a dfn with dfnType
// other than dfn and not marked with data-no-export is to be exported.
Expand All @@ -26,9 +25,36 @@ export function run() {
}

// Only add `lt`s that are different from the text content
if (titles.length === 1 && titles[0] === norm(dfn.textContent)) {
if (titles.length === 1 && firstTitle === norm(dfn.textContent)) {
return;
}
dfn.dataset.lt = titles.join("|");
});
}
/**
*
* @param {string} title
* @param {HTMLElement} dfn
*/
function associateInternalSlotWithIDL(title, dfn) {
if (!/^\[\[\w+\]\]$/.test(title)) return;
if (!dfn.dataset.idl) {
dfn.dataset.idl = "";
}

// Automatically use the closest data-dfn-for as the parent.
/** @type HTMLElement */
const parent = dfn.closest("[data-dfn-for]");
if (parent && dfn !== parent && parent.dataset.dfnFor) {
dfn.dataset.dfnFor = parent.dataset.dfnFor;
}

// Assure that it's data-dfn-for= something.
if (!dfn.dataset.dfnFor) {
const msg = `Internal slot "${title}" must be associated with a WebIDL interface.`;
const hint =
"Use a `data-dfn-for` attribute to associate this dfn with a WebIDL interface.";
showError(msg, name, { hint, elements: [dfn] });
}
dfn.dataset.dfnType = "attribute";
Copy link
Member

@sidvishnoi sidvishnoi Jun 22, 2021

Choose a reason for hiding this comment

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

This is something that will be fixed in follow-up, right? i.e., a new internalSlot type and internalSlotMethod maybe?

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
Collaborator

Choose a reason for hiding this comment

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

Will the existing tools throw if we introduce a new dfnType here?

Copy link
Member

Choose a reason for hiding this comment

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

re new type, discussion started in speced/spec-dfn-contract#1

}
53 changes: 53 additions & 0 deletions tests/spec/core/dfn-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,57 @@ describe("Core — Definitions", () => {
expect(el.hash).toBe("#dfn-first");
}
});

describe("internal slot definitions", () => {
const body = `
<section data-dfn-for="Test" data-cite="HTML">
<h2>Internal slots</h2>
<pre class="idl">
[Exposed=Window]
interface Foo{};
</pre>
<p>
<dfn id="simple">[[\\slot]]</dfn>
<dfn id="parent" data-dfn-for="Window">[[\\slot]]</dfn>
<dfn id="broken" data-dfn-for="">[[\\broken]]</dfn>
</p>
<section data-dfn-for="">
<h2>.</h2>
<p>
<dfn id="broken-parent">[[\\broken]]</dfn>
</p>
</section>
<p>
{{Test/[[slot]]}}
{{Window/[[slot]]}}
</p>
</section>
`;
it("sets the data-dfn-type an idl attribute", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
Comment on lines +190 to +191
Copy link
Member

Choose a reason for hiding this comment

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

This is repeated (with same args) in each test here. Can probably move it into a beforeAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's specific to this particular describe() group, so beforeAll is probably not ideal... wish they had a "before you run these tests".

Copy link
Member

Choose a reason for hiding this comment

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

I think beforeAll is scoped to desribe() blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will try. The docs said they beforeAll was global.

const dfn = doc.getElementById("simple");
expect(dfn.textContent).toBe("[[slot]]");
expect(dfn.dataset.dfnType).toBe("attribute");
expect(dfn.dataset.idl).toBe("");
});

it("when data-dfn-for is missing, it use the closes data-dfn-for as parent", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfn = doc.getElementById("simple");
expect(dfn.dataset.dfnFor).toBe("Test");
const dfnWithParent = doc.getElementById("parent");
expect(dfnWithParent.dataset.dfnFor).toBe("Window");
});

it("errors if the internal slot is not for something", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfnErrors = doc.respec.errors.filter(
({ plugin }) => plugin === "core/dfn"
);
expect(dfnErrors).toHaveSize(2);
});
});
});
4 changes: 3 additions & 1 deletion tests/spec/core/link-to-dfn-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ describe("Core — Link to definitions", () => {
const bodyText = `
<section>
<h2>Test section</h2>
<p><dfn>[[\\test]]</dfn><a id="testAnchor">[[\\test]]</a>
<p>
<dfn data-dfn-for="Window">[[\\test]]</dfn>
<a data-link-for="Window" id="testAnchor">[[\\test]]</a>
</section>`;
const ops = makeStandardOps(null, bodyText);
const doc = await makeRSDoc(ops);
Expand Down
5 changes: 4 additions & 1 deletion tests/spec/core/simple.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ <h2>Some title</h2>
Some content
</p>
<p><dfn id="custom_id(¡™£¢∞§¶•ªº)">custom id</dfn></p>
<p><dfn>[[\escapedSlot]]</dfn> <a>[[\escapedSlot]]</a></p>
</section>
<section data-dfn-for="Test">
<h2><dfn>Test</dfn> interface</h2>
Expand All @@ -42,5 +41,9 @@ <h2><dfn>Test</dfn> interface</h2>
</pre>
<section>
<h3><dfn>foo()</dfn> method</h3>
<p>
<dfn>[[\escapedSlot]]</dfn>
{{Test/[[escapedSlot]]}}
</p>
</section>
</section>
4 changes: 2 additions & 2 deletions tests/spec/core/xref-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -756,8 +756,8 @@ describe("Core — xref", () => {
it("links internalSlots", async () => {
const body = `
<section>
<p><dfn>[[\\type]]</dfn></p>
<p id="link1">{{ [[type]] }}</p>
<p><dfn data-dfn-for="Window">[[\\type]]</dfn></p>
<p id="link1">{{ Window/[[type]] }}</p>
<p id="link2">{{ Credential.[[type]] }}</p>
</section>
`;
Expand Down