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 <ins>/<del> with structured headers #533

Merged
merged 9 commits into from
Jun 18, 2023

Conversation

gibson042
Copy link
Contributor

Fixes #530
Fixes #531

Supports <ins>/<del> both inside and outside the structured-header <h1>.
Also extends baseline testing to cover warnings, and adds hopefully-temporary suppression of warnings that seem unexpected to me.

@@ -23,6 +23,8 @@ <h1>
</emu-alg>
</emu-clause>

<!-- FIXME: Shouldn't this warning be suppressed by `redefinition`? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

No - ids have effects on the document considered as a piece of HTML (i.e., for computers), not just on the document considered as a piece of writing (i.e., for humans).

"redefinition" means that we are redefining an algorithm, which is normally an error when considering the document as a piece of writing (since a human reader expects a given term to have only one definition). But it doesn't have any interaction with the error of reusing an id, since that's a problem for the computer, not just for humans: the computer needs distinct ids for each clause so that you can link to them independently.

<emu-annex id="annex11" aoid="SomeAlg">
<h1><span class="secnum">A.1</span> SomeAlg</h1>
</emu-annex>

<!-- FIXME: Shouldn't this be allowed by `namespace=annex2`? -->
Copy link
Contributor

@bakkot bakkot Jun 18, 2023

Choose a reason for hiding this comment

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

I'm not sure, but I would assume that namespaces apply to the contents of a clause/algorithm, not to the AOID of the algorithm itself. So this one makes sense - lines 70 and 76 are two clauses with the same aoid within the smae namespace.

src/utils.ts Outdated
export function traverseWhile(
node: Node | null,
relationship: NodeRelationship,
cb: (node: Node) => boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cb: (node: Node) => boolean,
predicate: (node: Node) => boolean,

@bakkot
Copy link
Contributor

bakkot commented Jun 18, 2023

Thanks!

I pushed a commit making the types for traverseWhile more precise, to avoid casts, and a second addressing my other comments. LGTM now.

@bakkot bakkot merged commit 504bfa6 into tc39:main Jun 18, 2023
@bakkot
Copy link
Contributor

bakkot commented Jun 18, 2023

Published in v17.0.1.

@justingrant
Copy link

I just tried to use this and got an error. Expected?

Spec text:

      <h1>
        <del>IsTimeZoneOffsetString</del><ins>IsOffsetTimeZoneIdentifier</ins> (
          _offsetString_: a String,
        ): a Boolean
      </h1>

Result:
Error: spec/mainadditions.html:444:21: could not find matching del tag

@gibson042
Copy link
Contributor Author

Hmm, I don't know if ecmarkup should support that level of granularity. But if it does, then the starting point would look something like this (which notably swallows the old name, a characteristic that is probably not desirable):

--- src/header-parser.ts
+++ src/header-parser.ts
@@ -70,7 +70,16 @@ function parseH1(headerText) {
     return { type: 'failure', errors };
   }
   offset += match[0].length;
-  const name = match[0].trimRight();
+  let name = match[0].trimRight();
+  if (wrappingTag !== null && name.includes(`</${wrappingTag}>`)) {
+    // correctly handle text like `<del>OldName</del><ins>NewName</ins>` as non-wrapped
+    wrappingTag = null;
+    const nameSource = `<${wrappingTag}>${name}`;
+    match = nameSource.match(/<ins[^>]*>([^<]+?)<\/ins>/) || nameSource.match(/>([^<]+?)</);
+    if (match) {
+      name = match[1];
+    }
+  }
 
   if (text === '') {
     if (wrappingTag !== null) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants