The crossOrigin attribute is described here: http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#cors-settings-attribute It says: "The empty string is also a valid keyword, and maps to the Anonymous state. The attribute's invalid value default is the Anonymous state. The missing value default, used when the attribute is omitted, is the No CORS state." The behavior currently implemented in bug 664299 is that the invalid-value-default is No CORS. So we need to change that to Anonymous, but without changing the missing-value-default. Can you help? It's not straightforward, because our facilities for implementing an enum attribute, such as NS_IMPL_ENUM_ATTR_DEFAULT_VALUE, don't seem to distinguish between an invalid value and a missing value. Also, this really matters in practice, see bug 643651.

Note: the code is here: http://hg.mozilla.org/mozilla-central/file/1dddaeb1366b/content/html/content/src/nsHTMLImageElement.cpp#l230 http://hg.mozilla.org/mozilla-central/file/1dddaeb1366b/content/html/content/src/nsHTMLImageElement.cpp#l355 http://hg.mozilla.org/mozilla-central/file/1dddaeb1366b/content/html/content/src/nsHTMLImageElement.cpp#l654

NS_IMPL_ENUM_ATTR_DEFAULT_VALUE will return the value stored in the nsAttrValue if that's an enumerated value. If there is no nsAttrValue or the nsAttrValue isn't enumerated, it will return the last argument to the macro. So in this case, it seems like we should just make the last argument to the macro "no cors" and make sure that as long as the attribute is set the nsAttrValue is enumerated and has the right enum value. That means that in ParseAttribute we need to ParseEnumValue and if that returns false go ahead and set the nsAttrValue to the "anonymous CORS" value. Unfortunately, it looks like we have no public API on nsAttrValue for setting it to an enumerated value. Jonas, can we just change ParseEnumValue to take an optional default value if the value doesn't match anything in the enumeration?

Sure, I'd be fine with that. Though we have to make sure that the string value that we store is whatever value was actually passed in.

Yes, we do that anyway.

So is either of you taking this bug? We really would like to have this in Firefox 8. A big major WebGL site coming out mid-August is relying on anonymous CORS and apparently is doing crossOrigin="" i.e. relying on the invalid-value-default.

If you need this in Fx8, I'm the wrong person, since I'm going on vacation from tomorrow until after Fx8 branches.

Or more precisely I _could_ try to write up a patch this afternoon, maybe, but someone else would obviously need to drive it in....

OK no problem. Jonas: can you do the part about the nsAttrValue API and give me directions for the rest? Or can you do everything?

Sorry, I won't have time to do this.

I'm also not likely to have time to write tests and whatnot, even if I can get the code done. I should have read the spec way better in bug 664299, by the way. For example, crossorigin="aNONymous" should give img.crossOrigin == "aNONymous" whereas we canonicalize it. That should be in tests too.

I can write the tests :-)

Comment on attachment 551524 [details] [diff] [review] This should do the trick r=me assuming tests pass

This also renames "leave-default-crossOrigin-attribute" to "missing-value-default" because that's really what it meant.

(With Boris's patch and that test patch, it passes. With only Boris's patch and without the test patch, it fails)

Comment on attachment 551537 [details] [diff] [review] test that crossOrigin="" or invalid value has the behavior of "anonymous" Anyone can review this?

Comment on attachment 551537 [details] [diff] [review] test that crossOrigin="" or invalid value has the behavior of "anonymous" Hmm.. these tests are complicated enough that I think someone that know them better than me should review them.

I'll review the tests tomorrow, but I'll have to review bug 664299 before that. ...unless Boris has time to look at the test changes.

Comment on attachment 551537 [details] [diff] [review] test that crossOrigin="" or invalid value has the behavior of "anonymous" Jeff said he'd review this because he's a sucker.

and this one too, which he asked for. testing that <img crossOrigin> really behaves like <img crossOrigin="">.

Comment on attachment 551537 [details] [diff] [review] test that crossOrigin="" or invalid value has the behavior of "anonymous" This looks fine as far as it goes, so r=me on these changes, but we should also add a test for the reflection stuff in comment 10. I seem to recall we have a DOM attribute reflection test framework in content/html/content/test/reflect.js; might be worth looking at other tests that use it.

(In reply to Boris Zbarsky (:bz) from comment #21) > Comment on attachment 551537 [details] [diff] [review] [diff] [details] [review] > test that crossOrigin="" or invalid value has the behavior of "anonymous" > > This looks fine as far as it goes, so r=me on these changes, but we should > also add a test for the reflection stuff in comment 10. I don't even know what is the specified behavior that we would want to test here. Smaug, Jonas?

See for example comment #c10

> I don't even know what is the specified behavior Getting .crossOrigin returns exactly the string that set using setAttribute. Setting .crossOrigin causes getAttribute to return exactly the string that was set. In particular, you want to test leading/trailing whitespace and case variations.

Ah OK thanks for the detailed explanation. Filed bug 678077 about adding that test. Landed on central: http://hg.mozilla.org/mozilla-central/rev/89a9f4a88d5b http://hg.mozilla.org/mozilla-central/rev/289becc07558 http://hg.mozilla.org/mozilla-central/rev/aac29f0bdd10

Backed out: http://hg.mozilla.org/mozilla-central/rev/a1b3ba6eabf8 http://hg.mozilla.org/mozilla-central/rev/686e5ad8fa96 http://hg.mozilla.org/mozilla-central/rev/1221d45e7aca for: 65289 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "ANONYMOUS", expected "anonymous" 65293 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "ANONYMOUS", expected "anonymous" 65297 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "USE-CREDENTIALS", expected "use-credentials" 65301 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "USE-CREDENTIALS", expected "use-credentials" 65303 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | When the content attribute is set to an invalid value, the default value should be returned. - got "foobar", expected "" 65305 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | When the value is set to an invalid value, the default value should be returned. - got "foobar", expected ""

Yeah, that test is just testing things that don't match the spec...

Good news/bad news time. Good news: the test that Boris asked for in comment 10 already exists, Joe wrote it in bug 664299. It uses the reflect.js framework as mentioned in comment 21. Bad news: That's the test that's failing in comment 26. As far as I understand, the failures fall into 2 categories: > 65289 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | Enumerated attributes > should be case-insensitive. - got "ANONYMOUS", expected "anonymous" > 65293 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | Enumerated attributes > should be case-insensitive. - got "ANONYMOUS", expected "anonymous" > 65297 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | Enumerated attributes > should be case-insensitive. - got "USE-CREDENTIALS", expected > "use-credentials" > 65301 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | Enumerated attributes > should be case-insensitive. - got "USE-CREDENTIALS", expected > "use-credentials" This seems to be a real regression, right? But looking at Boris' patch I don't understand how it can cause this regression. Anyone? > 65303 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | When the content > attribute is set to an invalid value, the default value should be returned. > - got "foobar", expected "" > 65305 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_bug664299.html | When the value is set > to an invalid value, the default value should be returned. - got "foobar", > expected "" Here I'm afraid that it's the reflect.js framework that is not prepared to distinguish between invalid-value-default and missing-value-default, right?

(In reply to Boris Zbarsky (:bz) from comment #27) > Yeah, that test is just testing things that don't match the spec... The 2 last out of these 6 failures indeed don't seem to match the spec. But the first 4? Aren't they testing exactly what you asked for in comment 10/21/24?

> This seems to be a real regression, right? It's a real behavior change, and expected. The test needs to be updated. > Here I'm afraid that it's the reflect.js framework that is not prepared The reflect.js framework is being called via reflectLimitedEnumerated here. So it's making various assertions about case treatment and whatnot that are only true for limited enumerated attributes. But this attribute is just enumerated, not limited enumerated. So this test should be using reflectString and a larger set of values to test that includes case variations and so forth.

OK thanks! I didn't realize the difference between enumerated and limited enumerated.

http://hg.mozilla.org/mozilla-central/rev/16a79c640966 http://hg.mozilla.org/mozilla-central/rev/c9bad43e7c28 http://hg.mozilla.org/mozilla-central/rev/b5189d4d6fa5 http://hg.mozilla.org/mozilla-central/rev/f262c389193e

This is documented there: https://developer.mozilla.org/en/HTML/Element/img#attr-crossorigin "If invalid, it is handled as if the enumerated keyword anonymous was used." and there: https://developer.mozilla.org/en/HTML/CORS_settings_attributes "An invalid keyword will be handled as the anonymous keyword." I don't think we need to add something in "Fx8 for developers". ->dev-doc-complete (reset to dev-doc-needed if you disagree)