Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formalize content attribute reflection #3238

Open
Zirro opened this issue Nov 19, 2017 · 14 comments
Open

Formalize content attribute reflection #3238

Zirro opened this issue Nov 19, 2017 · 14 comments
Labels
clarification Standard could be clearer topic: reflect For issues with reflected IDL attributes and friends.

Comments

@Zirro
Copy link
Contributor

Zirro commented Nov 19, 2017

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
Copy link
Member

domenic commented Nov 19, 2017

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:

  • 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.)
  • Should we require [Reflect=bgcolor] attribute DOMString bgColor, or can we assume lowercasing?
  • How do we express clamped to a range? IDL syntax seems not flexible enough again.
  • How do we express the various other numeric variants, i.e. limited to only non-negative numbers, limited to only non-negative numbers greater than zero, limited to only non-negative numbers greater than zero with fallback? Do they each get their own extended attribute? What do we name them that isn't ludicrously long?
  • How do we handle enumerated attributes? You've provided some ideas above with ReflectEmpty/ReflectInvalid/ReflectMissing/ReflectOnly. But they don't map very well onto the spec's concepts, which are more like: is limited to only known values yes/no, missing value default state, invalid value default state, list of states, keyword -> state mapping. In particular the state -> keyword mapping is troubling. How do we handle cases like crossorigin="" where the missing value default state doesn't have a corresponding keyword at all? Maybe we should be using IDL enums, potentially one enum for keywords and one for states, but usually just one enum shared between both?? That'd be a big refactoring.

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
Copy link
Member

annevk commented Nov 20, 2017

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

Some thoughts on the particulars:

  • Implicit lowercasing seems fine.
  • I also wouldn't mind potentially long names for the various numeric variants, though we could also make ReflectNumber accept a sequence of arguments.

cc @bzbarsky @tobie

@tobie
Copy link
Contributor

tobie commented Nov 20, 2017

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 whatwg/webidl#468 (comment).

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
Copy link
Member

nox commented Nov 20, 2017

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

@annevk
Copy link
Member

annevk commented Jan 22, 2018

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
Copy link
Member

dstorey commented Jan 25, 2018

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

  • [Reflect] Reflects the content attribute with the same name as the IDL attribute
  • [Reflect="foo"] Reflects the content attribute named foo. We don't do implicit lowercasing so have Reflect="contenteditable" for example. I'd probably prefer implicit lowercasing to reduce characters/duplication
  • [ReflectMissing=] Reflects missing value with value specified. keywords are quoted, numbers are not, e.g. ReflectMissing="auto" but ReflectMissing=0
  • [ReflectInvalid=] Same as above for invalid values. Often set to empty string ReflectInvalid=""
  • [ReflectValues=] List of valid values that are reflected. Range of values are in (), e.g. ReflectValues=("_blank"|"_self"|"_parent"|"_top"). A range of values of the same type is quoted, e.g. ReflectValues="bcp47_lang". Full list I can see: "bcp47_lang", "mime_type", "character_encoding", "color", "font-family". The list of values in () can get fairly long however so might be nice if could define externally (especially if there are common shared value lists) like enums currently are.
  • [ReflectType=""] Defines if values are space [ReflectType="space-separated"], or comma [ReflectType="comma-separated"] separated
  • [ReflectMinValue=number] The minimum value allowed, e.g. [Reflect="colspan", ReflectMinValue=1]

@nox
Copy link
Member

nox commented Jan 25, 2018

[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
Copy link
Member

nox commented Jan 25, 2018

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
Copy link
Contributor

tobie commented Jan 25, 2018

@nox, that's actually easy to fix, see: #3238 (comment).

@nox
Copy link
Member

nox commented Jan 25, 2018

@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
Copy link
Member

nox commented Jan 25, 2018

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

@dstorey
Copy link
Member

dstorey commented Jan 30, 2018

CC @arronei

@dstorey
Copy link
Member

dstorey commented Feb 5, 2018

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
Copy link
Member

domenic commented Mar 19, 2020

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

  • [Reflect], [ReflectURL], [ReflectNonNegative], [ReflectPositive], all of which can optionally take a string (not identifier) attribute name. If not provided, it will use the lowercased version of the attribute it is applied to.
  • [ReflectFallback=X], [ReflectRange=(X, Y)] to supply extra information for those cases.
  • We do not yet have a plan for enumerated attributes.

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;
};

annevk added a commit that referenced this issue Feb 22, 2023
Other changes:

* Remove reflection of unrestricted double as it is buggy and unused.
* The DOMString getter steps did not account for a missing attribute.
* The native accessibility semantics map was renamed to the internal content attribute map as it's now a more general reflection concept.

Corresponding ARIA PR: w3c/aria#1876.

Fixes #8442.

Follow-up:

* w3c/core-aam#152
* w3c/aria#1110
* #3238
* #8544
* #8545
* #8926
* #8927
* #8928
* #8930
@annevk annevk added the topic: reflect For issues with reflected IDL attributes and friends. label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: reflect For issues with reflected IDL attributes and friends.
Development

No branches or pull requests

6 participants