tc39 / tc39/ecmarkup

ins and del inline within a grammar production don't work correctly

Open
#150 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
245
Forks
80
Avg merge
10h 46m
Merged PRs (30d)
2

Description

I don't know if this is expected to work or being used incorrectly, but the private methods proposal has this ecmarkup with del and ins inline within a grammar production ((...) below means I've elided some further definitions there):

<emu-grammar>
  MethodDefinition[Yield, Await] :
    <del>PropertyName</del><ins>ClassElementName</ins>[?Yield, ?Await] `(` UniqueFormalParameters[~Yield, ~Await] `)` `{` FunctionBody[~Yield, ~Await] `}`
    (...)
</emu-grammar>

The resulting markup (pretty-printed a bit) is:

<emu-grammar>
  <emu-production name="MethodDefinition" params="Yield, Await" id="prod-MethodDefinition">
    <emu-nt params="Yield, Await"><a href="#prod-MethodDefinition">MethodDefinition</a>
      <emu-mods>
        <emu-params>[Yield, Await]</emu-params>
      </emu-mods>
    </emu-nt>
    <emu-geq>:</emu-geq>
    <emu-rhs a="13ccd732"><del><emu-nt><a href="https://tc39.github.io/ecma262/#prod-PropertyName">PropertyName</a></emu-nt></del>
      <emu-nt params="?Yield, ?Await" id="_ref_94"><a href="#prod-ClassElementName">ClassElementName</a>
        <emu-mods>
          <emu-params>[?Yield, ?Await]</emu-params>
        </emu-mods>
      </emu-nt>
      <emu-t>(</emu-t>
      <emu-nt params="~Yield, ~Await"><a href="https://tc39.github.io/ecma262/#prod-UniqueFormalParameters">UniqueFormalParameters</a>
        <emu-mods>
          <emu-params>[~Yield, ~Await]</emu-params>
        </emu-mods>
      </emu-nt>
      <emu-t>)</emu-t>
      <emu-t>{</emu-t>
      <emu-nt params="~Yield, ~Await"><a href="https://tc39.github.io/ecma262/#prod-FunctionBody">FunctionBody</a>
        <emu-mods>
          <emu-params>[~Yield, ~Await]</emu-params>
        </emu-mods>
      </emu-nt>
      <emu-t>}</emu-t>
    </emu-rhs>
    (...)
  </emu-production>
</emu-grammar>

You can see the current rendered result here.

There are two issues:

  1. The del element was retained, but the ins element was removed.
  2. When rendered, the current CSS shows the del element with a red background but without text-decoration: line-through because of this rule in elements.css.
    a {
        text-decoration: none;
        color: #206ca7;
    }
    

Presumably (2) can be fixed by adding a del a { } rule after the rule mentioned above:

del a {
    text-decoration: line-through;
}

Looking at (1), though, the only reference to del tags I see in the source is in utils.ts, and it handles ins as well:

export function shouldInline(node: Node) {
  let parent = node.parentNode;
  if (!parent) return false;

  while (parent && parent.parentNode &&
    (parent.nodeName === 'EMU-GRAMMAR' || parent.nodeName === 'EMU-IMPORT' || parent.nodeName === 'INS' || parent.nodeName === 'DEL')
  ) {
    parent = parent.parentNode;
  }

  return ['EMU-ANNEX', 'EMU-CLAUSE', 'EMU-INTRO', 'EMU-NOTE', 'BODY'].indexOf(parent.nodeName) === -1;
}

So I'm at a bit of a loss there.

Obviously, we can change the ecmarkup of the proposal to use del and ins around the entire line:

<emu-grammar>
  MethodDefinition[Yield, Await] :
    <del>PropertyName[?Yield, ?Await] `(` UniqueFormalParameters[~Yield, ~Await] `)` `{` FunctionBody[~Yield, ~Await] `}`</del>
    <ins>ClassElementName[?Yield, ?Await] `(` UniqueFormalParameters[~Yield, ~Await] `)` `{` FunctionBody[~Yield, ~Await] `}`</ins>
    (...)
</emu-grammar>

That works, although it slightly obscures the change being made.

Is the current inline use meant to be supported?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the inline del/ins grammar example from the issue and compare its generated markup with the rendered result. Inspect shouldInline in utils.ts and the anchor rule in elements.css, then verify that inline ins content is retained and links inside del elements remain struck through.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
documentation, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.