pp-webidl fails on Review Drafts

[annevk]

We're running into a subtle issue with Review Drafts. This first surfaced in #1194 but also affects the prior RD. Due to various settings in Bikeshed the resulting HTML ends up differing slightly between LS and RD which causes the JS to trip.

The simplest of these is probably

document.querySelector("div[data-fill-with=\"grammar-index\"]").innerHTML = wrap(output);

failing due to that div element no longer being present. This is happening to several other similar div elements as well, but those end up failing silently. The result though is that various grammar productions do not end up in the final copy of the standard.

@tabatkins do you know what could cause this? I looked a bit at slimBuildArtifact in Bikeshed and various other settings, but nothing really jumped out to me.

For debugging, you can compare:

• https://webidl.spec.whatwg.org/review-drafts/2022-03/
• https://webidl.spec.whatwg.org/commit-snapshots/e5afaf75c697ef14df26a2fe4557fb789ac60b49/

You can find the script at <script class="remove"> in the source of the first one as it essentially failed server-side when generating the draft.

[tabatkins]

Yes, it's a result of Slim Build Artifact. It has effects in a few places:

/usr/local/google/home/tabatkins/bikeshed/bikeshed/highlight.py:
   34  
   35  def addSyntaxHighlighting(doc: t.SpecT) -> None:
   36:     if doc.md.slimBuildArtifact:
   37          return
   38      normalizeHighlightMarkers(doc)

/usr/local/google/home/tabatkins/bikeshed/bikeshed/unsortedJunk.py:
  266  
  267  def addVarClickHighlighting(doc: t.SpecT) -> None:
  268:     if doc.md.slimBuildArtifact:
  269          return
  270      doc.extraStyles[
  ...
  968  def decorateAutolink(doc: t.SpecT, el: t.ElementT, linkType: str, linkText: str, ref: refs.RefWrapper) -> None:
  969      # Add additional effects to autolinks.
  970:     if doc.md.slimBuildArtifact:
  971          return
  972  
  ...
 1058  
 1059  def addSelfLinks(doc: t.SpecT) -> None:
 1060:     if doc.md.slimBuildArtifact:
 1061          return
 1062  
 ....
 1306              el.set("data-noexport", "")
 1307  
 1308:         if doc.md.slimBuildArtifact:
 1309              # Remove *all* data- attributes.
 1310              for attrName in el.attrib:

/usr/local/google/home/tabatkins/bikeshed/bikeshed/idl.py:
  330          h.addClass(el, "highlight")
  331          highlightingOccurred = True
  332:     if doc.md.slimBuildArtifact:
  333          # Remove the highlight-only spans
  334          for el in idlEls:

Notably, it prevents or removes highlighting, a few other assorted "human-readability" effects, and also removes all of the data-* attributes that Bikeshed uses thruout to signal various things. So if your script is depending on such a data-* attribute to exist, it'll fail.

[domenic]

Makes sense; thanks for debugging for us, Tab! I think we should be able to find an alternate way of identifying the div we need to manipulate, even in the slim version... I can give it a try next week when I'm back from vacation.

[annevk]

Note that it's not a single div element. This is just the one that fails the most loudly. It does seem like we could replace these data-* attributes with non-data-* attributes (e.g., idl-grammar attributes) as the data-fill-with values are custom and not recognized by Bikeshed anyway, but we'd have to update the inline script as well as the pp-webidl library at https://github.com/tobie/webidl-grammar-post-processor. That would require @tobie's help.

[tobie]

LMK how I can help. I've also added a bunch of you to that repo to lower the bus factor.

[domenic]

I have a fix. Basically we'll just stop using data-fill-with="" and instead use a non-data attribute, grammar-fill="" or similar. And then strip them all out in post-processing to keep the document valid.

I'll work on a couple PRs.