213227 2020-06-15 20:51:07 -0700 [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) 2024-01-16 13:49:45 -0800 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified NEW InRadar P2 Normal --- 1 sam sam cdumez changseok darin esprehn+autocc ews-watchlist gyuyoung.kim kondapallykalyan webkit-bug-importer oldest_to_newest 1662959 0 sam 2020-06-15 20:51:07 -0700 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". 1662960 1 401973 sam 2020-06-15 20:51:41 -0700 Created attachment 401973 WIP 1662965 2 sam 2020-06-15 21:09:17 -0700 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;` ... } 1662966 3 sam 2020-06-15 21:10:44 -0700 Darin, as steward of the "Reflect" extended attribute, would be very keen to hear your thoughts on this. 1662985 4 darin 2020-06-15 22:17:55 -0700 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? 1663526 5 sam 2020-06-17 09:21:26 -0700 (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. 2005390 6 webkit-bug-importer 2024-01-16 13:49:45 -0800 <rdar://problem/121075155> 401973 2020-06-15 20:51:41 -0700 2020-06-15 20:51:41 -0700 WIP img.diff text/plain 2447 sam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcAkocmV2aXNpb24g MjYzMDQ0KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcAkod29y a2luZyBjb3B5KQpAQCAtNjc3LDYgKzY3NywyOSBAQCBib29sIEhUTUxJbWFnZUVsZW1lbnQ6OmNv bXBsZXRlKCkgY29uc3QKICAgICByZXR1cm4gbV9pbWFnZUxvYWRlci0+aW1hZ2VDb21wbGV0ZSgp OwogfQogCit2b2lkIEhUTUxJbWFnZUVsZW1lbnQ6OnNldERlY29kaW5nKGNvbnN0IEF0b21TdHJp bmcmIHZhbHVlKQoreworICAgIHNldEF0dHJpYnV0ZVdpdGhvdXRTeW5jaHJvbml6YXRpb24oZGVj b2RpbmdBdHRyLCB2YWx1ZSk7Cit9CisKK0F0b21TdHJpbmcgSFRNTEltYWdlRWxlbWVudDo6ZGVj b2RpbmcoKQoreworICAgIHN0YXRpYyBNYWluVGhyZWFkTmV2ZXJEZXN0cm95ZWQ8Y29uc3QgQXRv bVN0cmluZz4gc3luYygic3luYyIsIEF0b21TdHJpbmc6OkNvbnN0cnVjdEZyb21MaXRlcmFsKTsK KyAgICBzdGF0aWMgTWFpblRocmVhZE5ldmVyRGVzdHJveWVkPGNvbnN0IEF0b21TdHJpbmc+IGFz eW5jKCJhc3luYyIsIEF0b21TdHJpbmc6OkNvbnN0cnVjdEZyb21MaXRlcmFsKTsKKyAgICBzdGF0 aWMgTWFpblRocmVhZE5ldmVyRGVzdHJveWVkPGNvbnN0IEF0b21TdHJpbmc+IGF1dG9LZXl3b3Jk KCJhdXRvIiwgQXRvbVN0cmluZzo6Q29uc3RydWN0RnJvbUxpdGVyYWwpOworCisgICAgc3dpdGNo IChkZWNvZGluZ01vZGUoKSkgeworICAgIGNhc2UgRGVjb2RpbmdNb2RlOjpTeW5jaHJvbm91czoK KyAgICAgICAgcmV0dXJuIHN5bmM7CisgICAgY2FzZSBEZWNvZGluZ01vZGU6OkFzeW5jaHJvbm91 czoKKyAgICAgICAgcmV0dXJuIGFzeW5jOworICAgIGNhc2UgRGVjb2RpbmdNb2RlOjpBdXRvOgor ICAgICAgICByZXR1cm4gYXV0b0tleXdvcmQ7CisgICAgfQorICAgIEFTU0VSVF9OT1RfUkVBQ0hF RCgpOworICAgIHJldHVybiBhdXRvS2V5d29yZDsKK30KKwogRGVjb2RpbmdNb2RlIEhUTUxJbWFn ZUVsZW1lbnQ6OmRlY29kaW5nTW9kZSgpIGNvbnN0CiB7CiAgICAgY29uc3QgQXRvbVN0cmluZyYg ZGVjb2RpbmdNb2RlID0gYXR0cmlidXRlV2l0aG91dFN5bmNocm9uaXphdGlvbihkZWNvZGluZ0F0 dHIpOwpJbmRleDogU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmgKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmgJKHJldmlzaW9u IDI2MzA0NCkKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5oCSh3b3Jr aW5nIGNvcHkpCkBAIC05Myw2ICs5Myw4IEBAIHB1YmxpYzoKIAogICAgIFdFQkNPUkVfRVhQT1JU IGJvb2wgY29tcGxldGUoKSBjb25zdDsKIAorICAgIHZvaWQgc2V0RGVjb2RpbmcoY29uc3QgQXRv bVN0cmluZyYpOworICAgIEF0b21TdHJpbmcgZGVjb2RpbmcoKTsKICAgICBEZWNvZGluZ01vZGUg ZGVjb2RpbmdNb2RlKCkgY29uc3Q7CiAgICAgCiAgICAgV0VCQ09SRV9FWFBPUlQgdm9pZCBkZWNv ZGUoUmVmPERlZmVycmVkUHJvbWlzZT4mJik7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hU TUxJbWFnZUVsZW1lbnQuaWRsCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRN TEltYWdlRWxlbWVudC5pZGwJKHJldmlzaW9uIDI2MzA0NCkKKysrIFNvdXJjZS9XZWJDb3JlL2h0 bWwvSFRNTEltYWdlRWxlbWVudC5pZGwJKHdvcmtpbmcgY29weSkKQEAgLTQwLDcgKzQwLDcgQEAK ICAgICBbQ0VSZWFjdGlvbnM9Tm90TmVlZGVkLCBSZWZsZWN0XSBhdHRyaWJ1dGUgRE9NU3RyaW5n IHVzZU1hcDsKICAgICBbQ0VSZWFjdGlvbnM9Tm90TmVlZGVkLCBSZWZsZWN0XSBhdHRyaWJ1dGUg dW5zaWduZWQgbG9uZyB2c3BhY2U7CiAgICAgW0NFUmVhY3Rpb25zPU5vdE5lZWRlZF0gYXR0cmli dXRlIHVuc2lnbmVkIGxvbmcgd2lkdGg7Ci0gICAgW0NFUmVhY3Rpb25zPU5vdE5lZWRlZCwgUmVm bGVjdF0gYXR0cmlidXRlIERPTVN0cmluZyBkZWNvZGluZzsKKyAgICBbQ0VSZWFjdGlvbnM9Tm90 TmVlZGVkXSBhdHRyaWJ1dGUgRE9NU3RyaW5nIGRlY29kaW5nOwogCiAgICAgW0NvbmRpdGlvbmFs PUFUVEFDSE1FTlRfRUxFTUVOVCwgRW5hYmxlZEF0UnVudGltZT1BdHRhY2htZW50RWxlbWVudF0g cmVhZG9ubHkgYXR0cmlidXRlIERPTVN0cmluZyBhdHRhY2htZW50SWRlbnRpZmllcjsKIAo=