Better reflection support by TimothyGu · Pull Request #2790 · jsdom/jsdom

TimothyGu · 2020-01-10T21:33:53Z

This PR aims to address the longstanding issues with reflection. Currently, we support reflecting content attributes in IDL attributes using the Chrome-style [Reflect] extended attribute in webidl2js. However, this proved to be inflexible, as supporting additional reflection varieties requires putting more jsdom-specific details in webidl2js. This has resulted in better reflection support being stalled over the past few years.

This PR introduces a different way to implement reflection. Rather than putting annotations in the IDL definition, the reflection is implemented using "decorator" functions for implementation classes, much like how event listener IDL attributes are implemented currently. For example, the reflected IDL attributes of HTMLInputElement are done as:

// parameters: (implementation class prototype, IDL attribute, content attribute[, default value])
reflectLimitedToNonNegativeGreaterThanZeroWithFallback(HTMLTextAreaElementImpl.prototype, "cols", "cols", 20);
reflectLimitedToNonNegativeGreaterThanZeroWithFallback(HTMLTextAreaElementImpl.prototype, "rows", "rows", 2);
reflectLimitedToNonNegative(HTMLTextAreaElementImpl.prototype, "maxLength", "maxlength");
reflectLimitedToNonNegative(HTMLTextAreaElementImpl.prototype, "minLength", "minlength");

Right now, I only introduced these very specific reflectors, though we could certainly extend it to the cases currently covered by [Reflect] in the future.

Even though brevity is on the side of [Reflect], I see several advantages for implementing reflections in the implementation file:

Allow reflection to be more closely integrated with the rest of jsdom.
More metadata about the reflected attribute can be specified in JavaScript than in Web IDL. (E.g., range of clamped attributes, allowed values for enumerated attributes.)
Implementation classes can make use of the reflected attributes, instead of having to use the clunkier attribute manipulator.
Smoother path to the automated updating of IDL files, if done in the future.
Better layering between jsdom and webidl2js.

On the other hand, a downside for moving reflection to the implementation file is that the implementation class is not aware of the type of the IDL attribute. This makes reflection of some IDL attributes that use default behaviors that are currently serviced by [Reflect] more verbose than before. (Chrome uses a hybrid behavior, using [Reflect] for some attributes but C++ for more complex behavior.)

Admittedly, the function names are a bit long at the moment, as I'm pretty much using the spec terminology verbatim. I'm open to suggestions on better names. Another idea is we could introduce a utility function, like addReflectedAttributes() which we can use as follows:

addReflectedAttributes(HTMLTextAreaElementImpl.prototype, {
  cols: { attribute: "cols", kind: "unsigned-positive-with-fallback", defaultValue: 20 },
  maxLength: { attribute: "maxlength", kind: "signed-non-negative" },
  type: { attribute: "type", kind: "enumerated-known-values", validValues: acceptedTypesSet, missingValueDefault: "text", invalidValueDefault: "text" }
});

where kind is

for strings: "string", "url", "enumerated-nullable", "enumerated-known-values"
for boolean: "boolean"
for numbers: "signed", "signed-non-negative", "unsigned", "unsigned-positive", "unsigned-positive-with-fallback", "unsigned-clamped", "float", and "float-positive"

This PR is rebased on top of #2786 and also uses one commit from #2789. Opening it as a draft for discussion and the Travis CI run.

With the exception of the removal of "datetime" <input> type state, these changes do not appear to be observable (or testable).

Even though this is unobservable, when we refer to the <input>'s value we should always use input._value, to avoid calling the complex value getter.

Restore to the original HTML definition of nonexistent for anything other than type=week.

Also make sure to use ASCII lowercase instead of Unicode lowercase during comparison.

This is more consistent with the rest of jsdom. Also make sure there is no Unicode whitespace before the number appears.

This also provides a high-precision version of the function that returns a Decimal object, using decimal.js, should it be needed in the future.

domenic · 2020-01-11T02:58:30Z

My main hesitation here is that I do hope to one day add [Reflect] attributes in the spec. I was kind of hoping we could use jsdom as a testing ground for figuring out what that would look like. whatwg/html#3238

That said, this would definitely be a step in the right direction.

I like the addReflectedAttributes variant for readability. It is a shame that we lose the type information. A few small ways you could make it shorter:

Infer attribute from the key via lowercasing. (You can still supply it explicitly if necessary.)
Consolidate unsigned-positive-with-fallback and unsigned-positive. The presence of defaultValue (fallbackValue?) makes it fallback.
Consolidate unsigned-clamped and unsigned. The presence of valueRange: [x, y] makes it clamped.

I'm a little unsure about expressing enumerated attributes with just a set of valid values. The full complexity of the keyword-vs.-state mapping seems hard to deal with without encoding the table, which may include keywordless states, or multiple keywords per state. Good test cases here are <area>'s shapes="" (multiple keywords per state), and crossorigin="" attributes (the empty string is a valid keyword, and the missing value default is a keywordless state). I think it's worth prototyping those particular cases before going to far.

BTW, here's a sketch of what would be needed for IDL attributes, setting aside enumerated attributes for now:

[Reflect], [ReflectURL], [ReflectNonNegative], [ReflectPositive], all of which can optionally take a (string) attribute name.
[ReflectFallback=X], [ReflectRange=(X, Y)] to supply extra information for those cases.

pmdartus · 2020-01-12T10:02:16Z

The interesting thing about the [Reflect] extended attribute is the declarative aspect of it.

However, this proved to be inflexible, as supporting additional reflection varieties requires putting more jsdom-specific details in webidl2js.

What do you think about generalizing the webidl2js processor that we were planning to use for [CEReactions] and [HTMLConstructor] to handle as well [Reflection] extended attribute? By default, webidl2js would ignore such extended attributes except if there is a processor for it. It feels strange that webidl2js known about how to reflect a property as an attribute is a jsdom implementation detail.

TimothyGu · 2020-01-29T06:13:54Z

In general, it sounds like extended attributes would work. Let me prototype it out. This PR was created because it seemed like the most frictionless way to get more complicated reflection working at all.

Consolidate unsigned-positive-with-fallback and unsigned-positive. The presence of defaultValue (fallbackValue?) makes it fallback.

This is not true. unsigned-positive takes a default value as well; the "with fallback" version gives different handling.

The other suggestions sound like they would work.

I'm a little unsure about expressing enumerated attributes with just a set of valid values. The full complexity of the keyword-vs.-state mapping seems hard to deal with without encoding the table, which may include keywordless states, or multiple keywords per state. Good test cases here are <area>'s shapes="" (multiple keywords per state), and crossorigin="" attributes (the empty string is a valid keyword, and the missing value default is a keywordless state). I think it's worth prototyping those particular cases before going to far.

The more tricky attributes only seem to affect how they are interpreted semantically, not how the reflection works. So it's still okay for the IDL level to be less aware of what states the keywords will get mapped to.

I think a good way to specify enumerated attributes at the IDL level could be to reuse the enum infrastructure, and have a [ReflectEnumerated=ValidKeywordsEnum] extended attribute. Hmm.

BTW, here's a sketch of what would be needed for IDL attributes, setting aside enumerated attributes for now:

[Reflect], [ReflectURL], [ReflectNonNegative], [ReflectPositive], all of which can optionally take a (string) attribute name.

[ReflectFallback=X], [ReflectRange=(X, Y)] to supply extra information for those cases.

I'm a bit concerned about essentially hardcoding all of these names into webidl2js. Maybe a better way would be to have a "super" extended attribute that tells webidl2js to hand over the control of the attribute to a custom processor, which can then inspect the IDL in depth to figure out what to do. So instead of [ReflectURL], have [Reflect, ReflectURL].

By the way, I don't believe [ReflectFallback=X] and [ReflectRange=(X, Y)] where X and Y are numbers are one of the valid Web IDL extended attribute grammar symbols right now. If these do get added to HTML then certainly we can change it, but webidl2.js has been tightening up the parsing of extended attributes quite a bit, so we might have to opt for some work-arounds like [ReflectFallback(optional double defaultValue = 0.3)].

domenic · 2020-02-01T22:11:34Z

The more tricky attributes only seem to affect how they are interpreted semantically, not how the reflection works. So it's still okay for the IDL level to be less aware of what states the keywords will get mapped to.

This is a good point. I was kind of envisioning something that generated the state-mapping code for us (so that impl code could look up something like this.shapesState), but that's going further than existing reflection support, and probably not necessary.

I think a good way to specify enumerated attributes at the IDL level could be to reuse the enum infrastructure, and have a [ReflectEnumerated=ValidKeywordsEnum] extended attribute. Hmm.

I agree (assuming we don't need the extra state mapping information).

I'm a bit concerned about essentially hardcoding all of these names into webidl2js. Maybe a better way would be to have a "super" extended attribute that tells webidl2js to hand over the control of the attribute to a custom processor, which can then inspect the IDL in depth to figure out what to do. So instead of [ReflectURL], have [Reflect, ReflectURL].

I think it'd be very reasonable to just hand over anything prefixed with Reflect to a processor. There's some value in avoiding repetition.

By the way, I don't believe [ReflectFallback=X] and [ReflectRange=(X, Y)] where X and Y are numbers are one of the valid Web IDL extended attribute grammar symbols right now. If these do get added to HTML then certainly we can change it, but webidl2.js has been tightening up the parsing of extended attributes quite a bit

My understanding per whatwg/webidl#574 is that is grammatically allowed, so I think if webidl2.js tries to disallow it, we should be able to push back on them. It makes sense as a warning (similar to other "semantic" mismatches such as non-readonly promise attributes, etc.) but a parser like webidl2.js should let us do what we want here.

TimothyGu · 2020-02-03T00:03:25Z

Looks like we'd rather go forward with #2813. Closing this one then.

TimothyGu added 12 commits January 9, 2020 19:53

Roll Web Platform Tests

3d1221e

More consistent applicability checking for <input> attributes

ca51c73

With the exception of the removal of "datetime" <input> type state, these changes do not appear to be observable (or testable).

Consistently use <input>._value for constraint validation

b78dd1e

Even though this is unobservable, when we refer to the <input>'s value we should always use input._value, to avoid calling the complex value getter.

Clean up _defaultStepBase in <input>

0fe57c9

Restore to the original HTML definition of nonexistent for anything other than type=week.

Clean up <input> typeMismatch constraint validation

205bc30

Add comment about radio button activation muddiness

793d82a

Implement <input> type state switch logic

5f2bb6d

Also make sure to use ASCII lowercase instead of Unicode lowercase during comparison.

Add support for <input type=time> reverse range validation

3943c52

Use null as "error" value for parseFloatingPointNumber

dbc0404

This is more consistent with the rest of jsdom. Also make sure there is no Unicode whitespace before the number appears.

Add rules for parsing non-negative integers

ed96e11

This also provides a high-precision version of the function that returns a Decimal object, using decimal.js, should it be needed in the future.

Fix reflection for <input> and <textarea>'s min/maxLength

494b193

Add reflection limited to only non negative numbers greater than zero

d420bd2

TimothyGu force-pushed the reflection branch from cca30aa to d420bd2 Compare January 10, 2020 22:35

This was referenced Jan 29, 2020

Move reflection support to host-side using processors jsdom/webidl2js#161

Merged

Better reflection support: take 2 #2813

Merged

TimothyGu closed this Feb 3, 2020

domenic mentioned this pull request Feb 3, 2020

More permissive extended attributes grammar w3c/webidl2.js#455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Better reflection support#2790

Better reflection support#2790
TimothyGu wants to merge 12 commits intojsdom:masterfrom
TimothyGu:reflection

TimothyGu commented Jan 10, 2020

Uh oh!

domenic commented Jan 11, 2020 •

edited

Loading

Uh oh!

pmdartus commented Jan 12, 2020

Uh oh!

TimothyGu commented Jan 29, 2020

Uh oh!

domenic commented Feb 1, 2020

Uh oh!

TimothyGu commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TimothyGu commented Jan 10, 2020

Uh oh!

domenic commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmdartus commented Jan 12, 2020

Uh oh!

TimothyGu commented Jan 29, 2020

Uh oh!

domenic commented Feb 1, 2020

Uh oh!

TimothyGu commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

domenic commented Jan 11, 2020 •

edited

Loading