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

Clarify priority between blob.type and Content-Type in headers for request/response.blob() #1630

Closed
CYBAI opened this issue Apr 12, 2023 · 4 comments · Fixed by #1657
Closed
Labels
clarification Standard could be clearer

Comments

@CYBAI
Copy link
Contributor

CYBAI commented Apr 12, 2023

Currently, the spec of blob() says

return a Blob whose contents are bytes and whose type attribute is this’s MIME type.

however, it doesn't mention when the body is a Blob with type, browsers should respect the blob's type or the Content-Type in headers.

there's a test in Chromium and WebKit.

 promise_test(function(t) { 
     var req = new Request('http://localhost/', 
                           {method: 'POST', 
                            body: new Blob([''], {type: 'Text/Plain'})}); 
     req.headers.set('Content-Type', 'Text/Html'); 
     return req.blob() 
       .then(function(blob) { 
           assert_equals(blob.type, 'text/plain'); 
           assert_equals(req.headers.get('Content-Type'), 'Text/Html'); 
         }); 
   }, 
   'MIME type unchanged if headers are modified after Request() constructor'); 

Chromium and WebKit are respecting blob's type so they could pass the test now. For Gecko, it fails to run the test because it would respect the headers' content-type.

With confirming with @annevk in WebKit/WebKit#12376 (comment), it seems the blob() should always respect the header's content-type as "this’s MIME type" wins. So, I wonder maybe we would need some notes (or algorithms in the blob()) to clarify more about blob's type when it exists in the spec?

Finally, if we should respect headers' Content-Type, I will fix the test to

-           assert_equals(blob.type, 'text/plain'); 
+           assert_equals(blob.type, 'text/html'); 

(As this test exists in Chromium and WebKit separately, I will try to make a WPT PR and help to remove the related one in Chromium and WebKit 🙏)

@annevk
Copy link
Member

annevk commented Apr 21, 2023

I think the main thing we want to do here is to change "this's MIME type" to be more obviously an algorithm invocation. I think that would address any potential confusion. But this would be an editorial change as in principle this already makes the relevant requirements.

@CYBAI
Copy link
Contributor Author

CYBAI commented May 15, 2023

I'd like to try working on the clarification. 🙋

I was wondering if https://fetch.spec.whatwg.org/#concept-body-mime-type is the right place to add the algorithm invocation 🤔 However, body doesn't have an associated headers.

Thus, maybe I should update https://fetch.spec.whatwg.org/#ref-for-concept-body-mime-type%E2%91%A2 and https://fetch.spec.whatwg.org/#ref-for-concept-body-mime-type%E2%91%A3 to run the algorithm instead?

@annevk
Copy link
Member

annevk commented May 15, 2023

That's great.

I think there are two approaches here:

  1. Renaming from "MIME type" to "get the MIME type" to add clarity. The concrete classes then continue to define the actual algorithm as they do today.
  2. Make it a standalone "get the MIME type" algorithm that takes and branches on a Request/Response object to do the right thing.

The standalone algorithm makes the mixin less generic, but that's not necessarily a bad thing I think. And it's probably the most clear.

@CYBAI
Copy link
Contributor Author

CYBAI commented May 15, 2023

Thank you! Sounds good to me! I will try with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer
Development

Successfully merging a pull request may close this issue.

2 participants