whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
7.98k stars 2.61k forks source link

Formalize content attribute reflection #3238

Open Zirro opened 6 years ago

Zirro commented 6 years ago

The details surrounding content attribute reflection on specific attributes is currently only defined in prose, with the specification containing 199 variations of the sentence "The IDL attribute Foo must reflect the Bar content attribute" at the moment.

There are at least two implementations (Blink and jsdom) which have introduced Web IDL extended attributes declaring the specifics of the reflection in IDL. Here is an example:

[CEReactions, Reflect=checked] attribute boolean defaultChecked;

This uses the [Reflect] extended attribute to declare that the defaultChecked attribute should be reflected as the checked content attribute. The information can then be used to generated code that automatically handles the specifics of the reflection.

In addition, Blink uses several variations of [Reflect] to cover cases including missing, invalid and limited sets of values. These extended attributes along with descriptions can be found on this page, but here is the summary:

[Reflect] indicates that a given attribute should reflect the values of a corresponding content attribute.

[ReflectEmpty="value"] gives the attribute keyword value to reflect when an attribute is present, but without a value; it supplements [ReflectOnly] and [Reflect].

[ReflectInvalid="value"] gives the attribute keyword value to reflect when an attribute has an invalid/unknown value. It supplements [ReflectOnly] and [Reflect].

[ReflectMissing="value"] gives the attribute keyword value to reflect when an attribute isn't present. It supplements [ReflectOnly] and [Reflect].

[ReflectOnly=<list>] indicates that a reflected string attribute should be limited to a set of allowable values; it supplements [Reflect].

There are additional cases that could be considered such as when an attribute is defined as representing a URL. This is also only covered by prose at the moment.

I believe it would be helpful for implementations and aid in automated testing if the specification were to formally define and use extended attributes or another non-prose method to cover the details of reflection in Web IDL.

domenic commented 6 years ago

I think we should definitely move this stuff into IDL if possible. However, when I've tried in the past, I've gotten stuck. Basically, my intuition was that it would be too confusing to only move some of the stuff into IDL, and leave the rest in prose. Which means solving it all at once. Which is hard.

Some particular issues:

It's possible others would find it OK to only have some of the reflection defined in IDL and others defined in prose, at least for now. I'd welcome thoughts. But my instinct is to try to put in a lot of work to come up with the perfect scheme that covers all cases, including modifying Web IDL to give us better syntax, and do it all at once. Which is hard :).

annevk commented 6 years ago

If we have a rough plan for all of it, I wouldn't mind it landing in phases.

Some thoughts on the particulars:

cc @bzbarsky @tobie

tobie commented 6 years ago

Web IDL only allows identifiers (not strings) on the right-hand side of equals signs in extended attributes. So, we can't do [Reflect="accept-charset"] or even [Reflect=accept-charset]. We have to do something like [Reflect=accept_charset] with a heuristic that _s get converted to -s. This is silly. (But, we could probably fix it in Web IDL.)

Quoting from WebIDL:

The ExtendedAttribute grammar symbol matches nearly any sequence of tokens, however the extended attributes defined in this document only accept a more restricted syntax. Any extended attribute encountered in an IDL fragment is matched against the following five grammar symbols to determine which form (or forms) it is in: […]

So it's easy to add other forms, see for example the proposed wildcard attributed for [Exposed] in https://github.com/heycam/webidl/issues/468#issuecomment-340062144.

So adding support for [Reflect="accept-charset"] would be roughly trivial as adding the following non-terminal to the list of supported ExtendedAttribute grammar symbols:

ExtendedAttributeString :
    identifier "=" string
nox commented 6 years ago

It should also be noted that at least Servo's, Gecko's and Blink's WebIDL parsers already understand this syntax.

annevk commented 6 years ago

Note that at least for <canvas>'s width/height there's also a requirement to be able to have some custom steps run before the reflecting steps. If the reflecting steps were all their own algorithms though that'd be fairly straightforward.

dstorey commented 6 years ago

Edge also uses custom extended attributes for reflection. We've just added this for Edge15 (next version of Edge, so the syntax could change):

nox commented 6 years ago

[Foo=("_blank"|"_self"|"_parent"|"_top")] is not a valid WebIDL syntax. Did you mean , instead of |?

https://heycam.github.io/webidl/#idl-extended-attributes

nox commented 6 years ago

Seems like the only way to do that with current WebIDL is to do (__blank, __self, __parent, __top) (note the 2 underscores because the leading underscore in WebIDL identifiers should be stripped).

tobie commented 6 years ago

@nox, that's actually easy to fix, see: https://github.com/whatwg/html/issues/3238#issuecomment-345621963.

nox commented 6 years ago

@domenic @tobie I just remembered that - is actually valid in an identifier.

https://heycam.github.io/webidl/#prod-identifier

That's why we can do this for CSSStyleDeclaration in Servo:

  [CEReactions, SetterThrows, TreatNullAs=EmptyString] attribute DOMString backgroundPositionY;
  [CEReactions, SetterThrows, TreatNullAs=EmptyString] attribute DOMString background-position-y;
nox commented 6 years ago

OTOH, /_?[A-Za-z][0-9A-Z_a-z-]*/ doesn't even allow __blank.

dstorey commented 6 years ago

CC @arronei

dstorey commented 6 years ago

Re[Foo=("_blank"|"_self"|"_parent"|"_top")] That is just how we did it as it followed what we already had elsewhere in our code base. We`re happy to change it to commas.

domenic commented 4 years ago

Some progress has been made on a concrete proposal for this over in the jsdom project. Our proposal so far is:

Examples (trimmed of various things that are not reflected attributes):

interface HTMLInputElement : HTMLElement {
  [CEReactions, Reflect="checked"] attribute boolean defaultChecked;
  [CEReactions, ReflectNonNegative] attribute long maxLength;
};

interface HTMLBaseElement : HTMLElement {
  [CEReactions, ReflectURL] attribute USVString href;
};

interface HTMLOListElement : HTMLElement {
  [CEReactions, Reflect] attribute boolean reversed;
  [CEReactions, Reflect, ReflectFallback=1] attribute long start;
};

interface HTMLTableColElement : HTMLElement {
  [CEReactions, Reflect, ReflectRange=(1, 1000), ReflectFallback=1] attribute unsigned long span;
};

interface HTMLProgressElement : HTMLElement {
  [CEReactions, ReflectPositive, ReflectFallback=1.0] attribute double max;
};