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

Should we use FrozenArray instead of StyleSheetList for adoptedStyleSheets? #36

Closed
rakina opened this issue Sep 12, 2018 · 12 comments
Closed

Comments

@rakina
Copy link
Member

rakina commented Sep 12, 2018

If we do, we don't need to introduce a constructor for StyleSheetList, and can use array assignment and other functions directly on adoptedStyleSheets.

cc @domenic who pointed this out.

@domenic
Copy link
Contributor

domenic commented Sep 12, 2018

Yeah. I think this is probably the right path. The only drawback is consistency. For web developers (as opposed to implementers/spec authors) the lack of consistency would be observable in the following ways:

document.styleSheets instanceof StyleSheetList;        // true
document.adoptedStyleSheets instanceof StyleSheetList; // false

document.styleSheets.item(0);        // works, although [0] does too
document.adoptedStyleSheets.item(0); // does not work; use [0] instead

I think these are not very bad, so FrozenArray is probably better.

@tabatkins
Copy link
Contributor

I'm not sure I understand; FrozenArray is fixed-length and readonly, so you can't use array assignment and other stuff on it.

@domenic
Copy link
Contributor

domenic commented Sep 12, 2018

If the attribute is not read-only, you can use array assignment on it. See Web IDL spec.

Not sure what "other stuff" StyleSheetList has that FrozenArray does not.

@tabatkins
Copy link
Contributor

If the attribute is not read-only, you can use array assignment on it. See Web IDL spec.

That... doesn't appear to be true, or if it is, WebIDL is at least severely misleading on this point.

2.13.32 says:

A frozen array type is a parameterized type whose values are references to objects that hold a fixed length array of unmodifiable values.

3.2.26, in the "create a frozen array" algorithm, freezes the constructed array:

Perform SetIntegrityLevel(array, "frozen").

Am I totally misreading this, or missing a part somewhere else that contradicts these lines and makes the array modifiable?

Not sure what "other stuff" StyleSheetList has that FrozenArray does not.

Because it's frozen, .push() doesn't work, for instance. (Again, unless I'm drastically misreading.)

@domenic
Copy link
Contributor

domenic commented Sep 12, 2018

Attribute definitions are separate from type definitions. You can have an immutable value held by a mutable attribute. (This is similar to how you can store an immutable value in a let binding, and then still reassign the binding to point to another value.) When you assign to such an attribute it creates a new frozen array using the algorithm you referred to and starts returning that.

StyleSheetList doesn't have .push() either.

@tabatkins
Copy link
Contributor

When you assign to such an attribute it creates a new frozen array using the algorithm you referred to and starts returning that.

Yes, you can replace the whole FrozenArray if the attribute isn't readonly. That's not "array assignment", which appears to be disallowed by FrozenArray.

StyleSheetList doesn't have .push() either.

Right, that would have gotten added, as appending newly-created sheets to the list is the whole point of .adoptedStylesheets.

@domenic
Copy link
Contributor

domenic commented Sep 12, 2018

I guess I don't know what "array assignment" means if it doesn't mean "assigning an array"

@tabatkins
Copy link
Contributor

Assigning to an array, like foo[0] = bar. ^_^

@rakina
Copy link
Member Author

rakina commented Oct 3, 2018

If we do add .push() to StyleSheetList, doesn't that mean we can do document.styleSheets.push(someSheet)?

If there's no way to prevent that then that means we can't add the function, so I think the FrozenArray approach is better here since you can just assign a normal array, instead of making another object that doesn't really do anything.

@tabatkins
Copy link
Contributor

Yeah, we should stick with FrozenArray for now. I discussed this with @domenic, and we might want to upgrade it into an array-like later, but that should be consistent with FrozenArray for now. (Changing will just mean that some things that throw would start working instead, I think.)

@rakina
Copy link
Member Author

rakina commented Oct 17, 2018

Cool, let me update the spec and explainer and close this soon.

@rakina
Copy link
Member Author

rakina commented Oct 17, 2018

Spec update is merged at #40.

@rakina rakina closed this as completed Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants