253814 2023-03-13 02:00:59 -0700 Concat with a CSSStyleSheet and shadowRoot.adoptedStyleSheets returns array in array 2023-03-13 17:30:07 -0700 1 1 1 Unclassified WebKit CSS Safari 16 All All RESOLVED FIXED InRadar P2 Major --- 1 mail ysuzuki cdumez ggaren mark.lam webkit-bug-importer ysuzuki oldest_to_newest 1940814 0 mail 2023-03-13 02:00:59 -0700 In Safari 16.4, when concatting a CSSStyleSheet with the shadowRoot.adoptedStyleSheets, it returns a nested array instead of a flat one: Correct: > [new CSSStyleSheet].concat([]) < [CSSStyleSheet] (1) > [new CSSStyleSheet].concat([new CSSStyleSheet]) < [CSSStyleSheet, CSSStyleSheet] (2) Not correct: > $0.shadowRoot.adoptedStyleSheets < [CSSStyleSheet] (1) > [new CSSStyleSheet].concat($0.shadowRoot.adoptedStyleSheets) < [CSSStyleSheet, Array] (2) Expected: < [CSSStyleSheet, CSSStyleSheet] (2) Correct: > [new CSSStyleSheet, ...$0.shadowRoot.adoptedStyleSheets] < [CSSStyleSheet, CSSStyleSheet] (2) 1940869 1 cdumez 2023-03-13 07:27:44 -0700 Question for JSC people (added in cc). Do you know what I need to do in order to make concat() behave correctly with this new IDL type? 1940875 2 cdumez 2023-03-13 07:33:36 -0700 (In reply to Chris Dumez from comment #1) > Question for JSC people (added in cc). Do you know what I need to do in > order to make concat() behave correctly with this new IDL type? The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a subclass of JSC::JSArray. 1940883 3 cdumez 2023-03-13 07:45:36 -0700 (In reply to Chris Dumez from comment #2) > (In reply to Chris Dumez from comment #1) > > Question for JSC people (added in cc). Do you know what I need to do in > > order to make concat() behave correctly with this new IDL type? > > The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a > subclass of JSC::JSArray. Looks like concat relies on isJSArray(): inline bool isJSArray(JSCell* cell) { ASSERT((cell->classInfo() == JSArray::info()) == (cell->type() == ArrayType)); return cell->type() == ArrayType; } So `cell->type() == ArrayType` is likely false for JSC::JSObservableArray. 1940888 4 cdumez 2023-03-13 07:50:50 -0700 (In reply to Chris Dumez from comment #3) > (In reply to Chris Dumez from comment #2) > > (In reply to Chris Dumez from comment #1) > > > Question for JSC people (added in cc). Do you know what I need to do in > > > order to make concat() behave correctly with this new IDL type? > > > > The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a > > subclass of JSC::JSArray. > > Looks like concat relies on isJSArray(): > > inline bool isJSArray(JSCell* cell) > { > ASSERT((cell->classInfo() == JSArray::info()) == (cell->type() == > ArrayType)); > return cell->type() == ArrayType; > } > > So `cell->type() == ArrayType` is likely false for JSC::JSObservableArray. cell->type() is DerivedArrayType for JSObservableArray, not ArrayType. 1940890 5 cdumez 2023-03-13 08:04:12 -0700 Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2]. IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then fallback to calling IsArray() [3]. ObservableArray is defined here [4]. [1] https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.concat [2] https://tc39.es/ecma262/multipage/indexed-collections.html#sec-isconcatspreadable [3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray [4] https://webidl.spec.whatwg.org/#es-observable-array 1940891 6 cdumez 2023-03-13 08:06:44 -0700 (In reply to Chris Dumez from comment #5) > Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2]. > IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then > fallback to calling IsArray() [3]. > > ObservableArray is defined here [4]. > > [1] > https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array. > prototype.concat > [2] > https://tc39.es/ecma262/multipage/indexed-collections.html#sec- > isconcatspreadable > [3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray > [4] https://webidl.spec.whatwg.org/#es-observable-array From https://webidl.spec.whatwg.org/#observable-array-exotic-object: ``` An observable array exotic object is a specific type of ECMAScript Proxy exotic object which is created using the proxy traps defined in this section. They are defined in this manner because the ECMAScript specification includes special treatment for Proxy exotic objects that have Array instances as their proxy target, and we want to ensure that observable array types are exposed to ECMAScript code with this special treatment intact. ``` So I believe concat should indeed spread the observable array here. However, our implementation currently relies on isJSArray(), not isArray() (which would be what the spec says). 1940910 7 cdumez 2023-03-13 09:11:43 -0700 (In reply to Chris Dumez from comment #6) > (In reply to Chris Dumez from comment #5) > > Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2]. > > IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then > > fallback to calling IsArray() [3]. > > > > ObservableArray is defined here [4]. > > > > [1] > > https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array. > > prototype.concat > > [2] > > https://tc39.es/ecma262/multipage/indexed-collections.html#sec- > > isconcatspreadable > > [3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray > > [4] https://webidl.spec.whatwg.org/#es-observable-array > > From https://webidl.spec.whatwg.org/#observable-array-exotic-object: > ``` > An observable array exotic object is a specific type of ECMAScript Proxy > exotic object which is created using the proxy traps defined in this > section. They are defined in this manner because the ECMAScript > specification includes special treatment for Proxy exotic objects that have > Array instances as their proxy target, and we want to ensure that observable > array types are exposed to ECMAScript code with this special treatment > intact. > ``` > > So I believe concat should indeed spread the observable array here. However, > our implementation currently relies on isJSArray(), not isArray() (which > would be what the spec says). I have tried several approaches here but I keep hitting assertions. Seems existing code is not happy dealing with a JSObservableArray, even though it is a JSArray subclass. 1940942 8 cdumez 2023-03-13 10:42:24 -0700 Pull request: https://github.com/WebKit/WebKit/pull/11449 1941018 9 webkit-bug-importer 2023-03-13 15:24:35 -0700 <rdar://problem/106667776> 1941022 10 ysuzuki 2023-03-13 15:29:52 -0700 Pull request: https://github.com/WebKit/WebKit/pull/11465 1941060 11 ews-feeder 2023-03-13 17:30:02 -0700 Committed 261604@main (9d30cd47aaab): <https://commits.webkit.org/261604@main> Reviewed commits have been landed. Closing PR #11465 and removing active labels.