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

Reflecting USVString attributes seems to be broken #1266

Closed
Ms2ger opened this issue May 17, 2016 · 13 comments
Closed

Reflecting USVString attributes seems to be broken #1266

Ms2ger opened this issue May 17, 2016 · 13 comments
Assignees

Comments

@Ms2ger
Copy link
Member

Ms2ger commented May 17, 2016

Content attribute values can be any WTF-16 value, so you can't return them from USVString-typed IDL attributes.

We might want to try to make all content attributes valid unicode, but unless we do that, we can't use USVString for things that can return attribute values directly.

@domenic
Copy link
Member

domenic commented May 17, 2016

Hmm. We should fix the definition of Reflect to run the convert to USVString algorithm from Web IDL, I think?

@domenic
Copy link
Member

domenic commented May 17, 2016

Or just go back to DOMString. @annevk, thoughts?

@annevk
Copy link
Member

annevk commented May 17, 2016

I was thinking this could be part of Reflect. With formalized slots this becomes even easier since both the content and IDL attribute are inputs to a slot of sorts.

@domenic
Copy link
Member

domenic commented May 17, 2016

I'd like to do this before we work on [Reflect]. It sounds like you are in favor of saying something like "if reflecting a USVString, convert the result to a USVString before returning it" to the section defining reflection?

@domenic domenic self-assigned this May 17, 2016
@zcorpan
Copy link
Member

zcorpan commented May 18, 2016

What should happen on setting?

@annevk
Copy link
Member

annevk commented May 18, 2016

Yeah, when setting it ends up setting some internal slot (might even have to be a URL record for Blob URLs) which we'd define as converting to USVString and then passing it on to the URL parser.

@zcorpan
Copy link
Member

zcorpan commented May 18, 2016

OK, so the content attribute should just get set to the given value without conversion to USVString? If so it sounds like the type should still be DOMString.

@annevk
Copy link
Member

annevk commented May 18, 2016

Yeah, that sounds correct actually. Only the internal slot can be non-DOMString.

@domenic
Copy link
Member

domenic commented May 18, 2016

Wait, what? I thought we'd want to use the normal behavior when setting a USVString attribute. That is, I'd expect

const a = document.createElement("a");
a.ping = "\uDC01";
assert(a.getAttribute("ping") === "\uFFFD");
assert(a.ping === "\uFFFD");

@annevk
Copy link
Member

annevk commented May 18, 2016

Does that make sense when setAttribute() doesn't?

@domenic
Copy link
Member

domenic commented May 18, 2016

I mean, it makes as much sense as all the other non-transparent-passthrough logic reflecting does, like the numeric censoring-to-0. It seems like a similar situation to me: JavaScript has a simple unrestricted "string" or "number" type; the spec decides it wants a restricted subset of strings (USVString) or numbers (unsigned long); the setter is then responsible for censoring values outside that range, and the getter responsible for parsing/converting values until they fit in that range.

@annevk
Copy link
Member

annevk commented May 18, 2016

Fair.

@zcorpan
Copy link
Member

zcorpan commented May 19, 2016

@domenic SGTM. It wasn't clear to me what behavior we wanted for this. :-)

domenic added a commit that referenced this issue May 19, 2016
This finds all instances where URLs are part of an IDL interface,
including where they are reflecting content attributes, and changes the
type from DOMString to USVString. This has already been done in some
places in a haphazard way, but this commit finishes the process.

This allows us to clarify the rules for reflecting USVString attributes,
and remove the clause that allows DOMString attributes to reflect URLs.

Fixes #1254. Fixes #1266.
domenic added a commit that referenced this issue May 20, 2016
This finds all instances where URLs are part of an IDL interface,
including where they are reflecting content attributes, and changes the
type from DOMString to USVString. This has already been done in some
places in a haphazard way, but this commit finishes the process.

This allows us to clarify the rules for reflecting USVString attributes,
and remove the clause that allows DOMString attributes to reflect URLs.

Fixes #1254. Fixes #1266.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants