crossOrigin attribute invalid-value-default should be Anonymous

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Unassigned)

Tracking

({dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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

Comment 1

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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....
(Reporter)

Comment 8

6 years ago
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 :-)
Created attachment 551524 [details] [diff] [review]
This should do the trick
Comment on attachment 551524 [details] [diff] [review]
This should do the trick

r=me assuming tests pass
Attachment #551524 - Flags: review+
Created attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

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?
Attachment #551537 - Flags: review?(jonas)
Attachment #551537 - Flags: review?(Olli.Pettay)
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.
Attachment #551537 - Flags: review?(jonas)
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.
Attachment #551537 - Flags: review?(Olli.Pettay) → review?(jmuizelaar)
Created attachment 551588 [details] [diff] [review]
test crossOrigin without value

and this one too, which he asked for. testing that <img crossOrigin> really behaves like <img crossOrigin="">.
Attachment #551588 - Flags: review?(jmuizelaar)
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.
Attachment #551537 - Flags: review+
Attachment #551588 - Flags: review+
(Reporter)

Updated

6 years ago
Attachment #551537 - Flags: review?(jmuizelaar)
(Reporter)

Updated

6 years ago
Attachment #551588 - Flags: review?(jmuizelaar)
(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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Depends on: 678077
Resolution: --- → FIXED
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 ""
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Created attachment 552507 [details] [diff] [review]
fix the crossOrigin attribute test

Updated

6 years ago
Attachment #552507 - Flags: review+
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
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: dev-doc-needed

Updated

6 years ago
Depends on: 679689
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)
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Depends on: 702578

Updated

6 years ago
Depends on: 715124
You need to log in before you can comment on or make changes to this bug.