880997 - Reflect crossOrigin as a limited enumerated attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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 ?

Comment 2

[Mounir Lamouri (:mounir)]

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.

Comment 3

[Mounir Lamouri (:mounir)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Simon Pieters [:zcorpan]]

(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 )

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move GetEnumAttr up to Element so I can use it in SVG code

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Fix reflect.js to be more strict when examining the value of the IDL attribute for enumerated reflection (e.g. to differentiate between null and empty string) and to allow flagging enumerated reflection as allowing null as a value

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change crossOrigin reflection to allow null values and be a limited enumerated attribute

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

   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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Carsten Book [:Tomcat]]

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