Bug 213227 – [WPT] img.decoding does not reflect the content attribute limited to known values as specified [WPT] form.action does not return document.url when unset (part of https://wpt.live/html/dom/reflection-embedded.html)

• Status: NEW
• Component: DOM

[Sam Weinig]

WPT: img.decoding does not reflect the content attribute limited to known values as specified [WPT] form.action does not return document.url when unset

Test: https://wpt.live/html/dom/reflection-embedded.html
Spec: https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decoding

From the spec: "The decoding IDL attribute must reflect the decoding content attribute, limited to only known values." The known values are the set of image decoding hints (https://html.spec.whatwg.org/multipage/images.html#image-decoding-hint): "sync", "async" and "auto".

[Sam Weinig]

Created attachment 401973 [details]
WIP

[Sam Weinig]

While a fix like the one I just attached as a WIP is straightforward, I think it would be preferable to come up with an automatic way to handle the reflection "limited to only known values" (see https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#limited-to-only-known-values) case better, so we can keep as much of the autogenerated Reflect code as we can.

The most straightforward solution would be to add a variant of Reflect that allowed a custom getter (e.g. ReflectWithCustomGetter). Then, at the very least HTMLImageElement::setDecoding would not be something we need implement.



We could potentially add a delegation point that does the limiting / filtering, e.g.

static AtomString HTMLImageElement::decodingContentAttributeLimiter(const AtomString& decodingMode)
{
     // convert to limited values here
}

and have the bindings generator generate use something like the following:

interface HTMLImageElement {
    ...
    [ReflectWithLimits] attribute DOMString decoding;
    ...
}

to generate:

   return HTMLImageElement::decodingContentAttributeLimiter(element.attributeWithoutSynchronization(decodingAttr));




Or, we could try to go all out and encode the limits into the bindings with something like the following in the IDL:

enum ValidDecodingContentAttributeValues { "sync", "async", "auto" };

interface HTMLImageElement {
    ...
    [Reflect, LimitTo=ValidDecodingContentAttributeValues, InvalidDefault="auto", MissingDefault="auto"] attribute DOMString decoding;`
    ...
}

[Sam Weinig]

Darin, as steward of the "Reflect" extended attribute, would be very keen to hear your thoughts on this.

[Darin Adler]

Some disorganized thoughts.

One thing I think about is that “reflect” terminology originally came from the specification wording. How do the specifications describe all these cases? What words are used in the specifications?

I like the one based on an enumeration. The original motivation was to cut down on mistakes in repetitive code so generating more seems good.

But the example looks wordy so I’m a little concerned.

I guess there are two ways to think about this. One is “set as a string, but get as an enumeration”. Another is “set the content attribute but get the effective value”. When you think about it the second way, what you want is to implement a getter that shares the logic with whatever code parses the value. It’s not part of the binding exactly. It’s part of the attribute parsing.

One option is that you write the getter but it returns an enum value, not a string. This makes it more likely that the same getter can be used inside the element class implementation.

I think “ReflectWithCustomGetter” is kind of a funny way to put it because “custom getter” is just normal. Maybe “ReflectSetterOnly” is better?

Really looking for a way to avoid handwriting all these parsers for attribute values. That really does point towards the enum style because that’s effectively what an enum does; just normally the string part stays entirely in the binding layer but for attributes the parsing is needed any time the attribute changes.

My goal would be to not write the “parse for expected values” code.

Maybe syntax for defining the content attribute itself, not just the binding?

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Some disorganized thoughts.
> 
> One thing I think about is that “reflect” terminology originally came from
> the specification wording. How do the specifications describe all these
> cases? What words are used in the specifications?

The spec still uses the term "reflect" extensively. For instance, for img.decoding, it writes:

"The decoding IDL attribute must *reflect* the decoding content attribute, *limited to only known values."  (https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decoding)

with *reflect* linking to the extensive documentation of the reflection process: https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes

> 
> I like the one based on an enumeration. The original motivation was to cut
> down on mistakes in repetitive code so generating more seems good.
> 
> But the example looks wordy so I’m a little concerned.
> 
> I guess there are two ways to think about this. One is “set as a string, but
> get as an enumeration”. Another is “set the content attribute but get the
> effective value”. When you think about it the second way, what you want is
> to implement a getter that shares the logic with whatever code parses the
> value. It’s not part of the binding exactly. It’s part of the attribute
> parsing.
> 
> One option is that you write the getter but it returns an enum value, not a
> string. This makes it more likely that the same getter can be used inside
> the element class implementation.
> 
> I think “ReflectWithCustomGetter” is kind of a funny way to put it because
> “custom getter” is just normal. Maybe “ReflectSetterOnly” is better?
> 
> Really looking for a way to avoid handwriting all these parsers for
> attribute values. That really does point towards the enum style because
> that’s effectively what an enum does; just normally the string part stays
> entirely in the binding layer but for attributes the parsing is needed any
> time the attribute changes.
> 
> My goal would be to not write the “parse for expected values” code.
> 
> Maybe syntax for defining the content attribute itself, not just the binding?

This last point really speaks to me. There is a lot of code in WebCore that we could probably generate if we didn't think of this as solely a bindings problem. Just as we started generating more CSS code with the introduction of CSSProperties.json, this leads me to think it is at least worth investigating adding Elements.json (or maybe HTMLElement.json as it is likely scoped pretty narrowly, but I am not sure) where we could describe the elements and their attributes and start generating help functions where necessary. What I am envisioning is something like (very rough):

{
    ...
    "img" : {
        "class" : "HTMLImageElement",
        "content-attributes": {
             "alt: { ... },
             "src: { ... },
             ...
             "decoding: {
                 "enumerated": {
                     "values": ["sync", "async", "auto"],
                     "missing": "auto",
                     "invalid": "auto",
                 },
                 "status": "supported",
                 "specification": {
                     "category": "html",
                     "url": "https://html.spec.whatwg.org/multipage/embedded-content.html#attr-img-decoding"
                 }
             }
        }
}

You get the idea. This could then be used to generate things like the HTMLImageElement::decodingMode() function, and be used as input to bindings generation for defining reflection details when needed.

This also has the benefit of allowing us to add something like https://webkit.org/css-status/ (which is generated from CSSProperties.json) for HTMLElements/attributes.

[Radar WebKit Bug Importer]

<rdar://problem/121075155>