:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

https://www.w3.org/Bugs/Public/show_bug.cgi?id=19574

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 11 years ago

Should be pretty simple, but the W3C bug makes no sense to me.  How does making this limited enumerated address the "" bits of https://www.w3.org/Bugs/Public/show_bug.cgi?id=19574#c1 ?

	Mounir Lamouri (:mounir)
	Comment 2 • 11 years ago

The attribute has an "invalid value default" being "Anonymous" so the reflection rules say that you have to use the keyword of the associated state when the current value is invalid. "" is an invalid value such as "foobar" would be.

This said, I am not a big fan of behavioural difference when an attribute is not set or set to the empty string (or even some garbage). We should try to keep the behaviour the same in all those situations otherwise, I am afraid that developers might spend a long time trying to understand what is happening simply because they switched crossorigin="use-credentials" to crossorigin="" expecting this to remove cors restrictions. Though, Hixie does not agree at all with that vision.

	Mounir Lamouri (:mounir)
	Comment 3 • 11 years ago

Also FWIW, no engine seems to implement this bit of the specs. Everyone seems to be reflecting the attribute as a simple DOMString. I haven't tested Trident though.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

Well, this bit of the spec is new.  It didn't use to be that way...

I think the "" behavior is there so that <img crossorigin src="whatever"> will work.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 10 years ago

OK, the spec has been updated (see <http://html5.org/r/8727>) so the missing-value behavior is to return null, and assigning null removes the attribute.  This actually kinda makes sense now.

	Simon Pieters [:zcorpan]
	Comment 6 • 10 years ago

(In reply to Mounir Lamouri (:mounir) from comment #3)
> Also FWIW, no engine seems to implement this bit of the specs. Everyone
> seems to be reflecting the attribute as a simple DOMString. I haven't tested
> Trident though.

Blink has it limited to known values now AFAICT. null thing not implemented yet (https://code.google.com/p/chromium/issues/detail?id=409524 )

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 10 years ago

In case it wasn't clear, I'm actively working on this.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 10 years ago

Comment on attachment 8485098 [details] [diff] [review]
part 3.  Change crossOrigin reflection to allow null values and be a limited enumerated attribute

>   /**
>+   * Unset an attribute.
>+   */
>+  void UnsetAttr(nsIAtom* aAttr, ErrorResult& aError)
>+  {
>+    aError = UnsetAttr(kNameSpaceID_None, aAttr, true);
>+  }
>+
>+  /**
>+   * Set an attribute in the simplest way possible.
>+   */
>+  void SetAttr(nsIAtom* aAttr, const nsAString& aValue, ErrorResult& aError)
>+  {
>+    aError = SetAttr(kNameSpaceID_None, aAttr, aValue, true);
>+  }
These don't cause any "foobar(...) hiding someother foobar(...)" warnings? I guess not.


>+
>+  /**
>+   * Set a content attribute via a reflecting nullable string IDL
>+   * attribute (e.g. a CORS attribute).
>+   */
>+  void SetNullableStringAttr(nsIAtom* aName, const nsAString& aValue,
>+                             ErrorResult& aError);
I think the comment should say that setting null string removes the attribute.
And perhaps the name should be SetOrRemoveNullableAttr or
SetOrRemoveNullableStringAttr (that is a bit long, but at least it says something about removal).


>+[uuid(ec18e71c-4f5c-4cc3-aa36-5273168644dc)]
> interface nsIDOMHTMLImageElement : nsISupports
> {
>            attribute DOMString        alt;
>            attribute DOMString        src;
>            attribute DOMString        srcset;
>            attribute DOMString        sizes;
>-           attribute DOMString        crossOrigin;
Not sure why we want to remove all these, but fine.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 10 years ago

> These don't cause any "foobar(...) hiding someother foobar(...)" warnings? I guess not.

Right, because Element already declares overrides of UnsetAttr and SetAttr.

> I think the comment should say that setting null string removes the attribute.
> And perhaps the name should be SetOrRemoveNullableAttr or
> SetOrRemoveNullableStringAttr 

SetOrRemoveNullableStringAttr it is.

> Not sure why we want to remove all these

Because then I don't have to try to implement XPCOM getters/setters with the right behavior.  It was just less work than implementing those.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 10 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cdfb7370315
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb7ecf90281
   https://hg.mozilla.org/integration/mozilla-inbound/rev/388133d2384e

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 10 years ago

And https://hg.mozilla.org/integration/mozilla-inbound/rev/651d43dca047 to fix the script reflection tests, which I'd failed to find.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 10 years ago

And https://hg.mozilla.org/integration/mozilla-inbound/rev/93b368055aa8 for the web-platform-test reflection tests (though those seem pretty wonky for <img>!).

	Carsten Book [:Tomcat]
	Comment 16 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/6cdfb7370315
https://hg.mozilla.org/mozilla-central/rev/ecb7ecf90281
https://hg.mozilla.org/mozilla-central/rev/388133d2384e
https://hg.mozilla.org/mozilla-central/rev/651d43dca047
https://hg.mozilla.org/mozilla-central/rev/93b368055aa8