jsdom / webidl2js

Auto-generate JS class structures for Web IDL specifications
MIT License
79 stars 30 forks source link

stringifier as CSSOMString attribute is not allowed #254

Open cdoublev opened 2 years ago

cdoublev commented 2 years ago

When generating a wrapper class for CSS MediaList, I get the following error:

/home/vagrant/npm/css/node_modules/webidl2js/lib/constructs/interface.js:341
            throw new Error(`${msg}attribute can only be of type DOMString or USVString`);
                  ^

Error: Invalid stringifier "mediaText" on interface MediaList: attribute can only be of type DOMString or USVString
    at Interface._analyzeMembers (/home/vagrant/npm/css/node_modules/webidl2js/lib/constructs/interface.js:341:19)
    at Interface.toString (/home/vagrant/npm/css/node_modules/webidl2js/lib/constructs/interface.js:1718:12)
    at Transformer._writeFiles (/home/vagrant/npm/css/node_modules/webidl2js/lib/transformer.js:221:24)
    at async Transformer.generate (/home/vagrant/npm/css/node_modules/webidl2js/lib/transformer.js:286:5)
  ...
  stringifier attribute [LegacyNullToEmptyString] CSSOMString mediaText;
  ...

I'm not sure why CSSOMString attribute is not accepted or if it should be accepted, but obviously when appending ... && member.idlType.idlType !== "CSSOMString) ...", it works.

domenic commented 2 years ago

So, CSSOMString is basically a weird hack around the fact that it's hard for Rust code to implement DOMString APIs, and so the CSSWG decided to sacrifice edge-case interoperability (i.e. what happens to unpaired surrogates) in favor of letting Servo/Gecko write less code in their implementation. As such it's defined as either typedef USVString CSSOMString; or typedef DOMString CSSOMString;.

So the first question is, do your input IDL files properly include one of those typedefs? (I would suggest typedef DOMString CSSOMString;.) There's a chance that including that typedef will make this error go away.

But, there's also a chance that it won't. If so, this is probably ultimately an issue with how the Web IDL spec is not very good about "resolving" typedefs before it uses them. The stringifier spec says "The stringifier keyword must not be placed on an attribute unless it is declared to be of type DOMString or USVString" and it's not really defined whether "declared as" is post-typedef resolution or pre-typedef resolution. (Certainly the type that appears in the syntactic declaration is pre-typedef resolution...) A similar issue is, e.g., https://github.com/whatwg/webidl/issues/649 about how it's not clear whether the "integer types" predicate applies through typedefs.

So anyway, in terms of a fix:

You could also just manually replace CSSOMString with DOMString in your input IDL files as a temporary workaround.

cdoublev commented 2 years ago

The typedef DOMString CSSOMString; was missing in my IDL file, sorry about that. I added it (before the MediaList interface, of course) but the same error is produced, sadly, yes.

Thank you for the clarifications. I was somewhat familiar with the reasons why CSSOMString exists, but it was confusing to me because it also seemed to me that surrogates were replaced when parsing most (if not all) input values handled by the CSS API.

Filtered surrogates from the input stream, per WG resolution. Now the entire specification operates only on scalar values.

Replace any U+0000 NULL or surrogate code points in input with U+FFFD REPLACEMENT CHARACTER (�).

Both quotes are from CSS Syntax. MediaList is a list of CSS media queries that go through the above procedure when parsed. So I wonder why DOMString would have to be preferred instead of USVString (as a temporary workaround)?

domenic commented 2 years ago

Hmm, interesting. It does seem like a lot of the properties or method arguments that use CSSOMString eventually do call normalize into a token stream. So it might be the case that, at least for most APIs, using USVString at the IDL layer would allow you to skip the normalization step later.

On the other hand, it might be kind of messy since other entry points (like, saying, parsing the contents of <style>, not using any webidl2js-wrapped APIs) also need to normalize. So you couldn't always skip it. If you wanted the perf win of minimizing normalization, I think you'd need to track whether you'd normalized or not (i.e. whether the input to the parse algorithm is a USVString or DOMString). And that seems a bit annoying.

But you raise a good point that, apparently, DOMString and USVString have the same web-observable behavior for most Web IDL-generated CSS APIs, since the input usually goes through a parser. I'd never realized that before.