ecmarkup icon indicating copy to clipboard operation
ecmarkup copied to clipboard

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

Open tjcrowder opened this issue 6 years ago • 3 comments

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?

tjcrowder avatar Jan 27 '19 12:01 tjcrowder

You get some weirdness also when the grammar rule itself is using ins. (Which comes up when adding new rules).

<emu-grammar type="definition">
  (...)
  <ins>Foo :</ins>
    <ins>`.`</ins>
</emu-grammar>

It'll work and render correctly in the place it is in the document, but it doesn't get copied into the appendix grammar with the other rules. I assume the parsing code that looks for the grammar rule line sees the <ins> and doesn't look inside.

A fix for the inlining issue would clean up a lot of my proposal spec changes. I'm doing the same whole line del ins you pointed out.

sirisian avatar Jan 29 '19 04:01 sirisian

@tjcrowder You might want to look at: https://github.com/rbuckton/grammarkdown since that handles the grammar parsing. Looks like with https://github.com/rbuckton/grammarkdown/issues/26 they fixed at lest one issue with ins and del. I opened two tickets. I'd say open tickets there for each example you can make.

sirisian avatar Jan 30 '19 05:01 sirisian

@sirisian - It does look like there's a grammarkdown aspect to this, but if I take your example from https://github.com/rbuckton/grammarkdown/issues/32 and use del tags, the del tags get stripped whereas in this example only ins tags get stripped, not del tags. (But I also don't get the enu-t tags you're getting your output. I'm using grammarkdown directly, tried both html and ecmarkup formats.) So there's more going on...

tjcrowder avatar Jan 30 '19 07:01 tjcrowder