1444872 - Remove processing for document.open()'s type parameter

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Anne (:annevk)]

It's not implemented in Chrome, Edge, or Safari.

HTML Standard: https://github.com/whatwg/html/pull/3559.

Comment 1

[Anne (:annevk)]

Tests are at https://github.com/w3c/web-platform-tests/pull/9978.

Comment 2

[Anne (:annevk)]

bz, this is a simplification we could make of document.open() that doesn't touch the bits we're less clear on. (I do still plan on researching those more.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  At one point this was definitely used.  Worse yet, things that do use it can end up with security bugs due to a change from things being treated as text to things being treated as HTML...

Are we _very_ very sure no one else supports this?  IE used to, but they removed it in edge?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, just tested, and https://jsfiddle.net/s7Lhdkxh/ shows that Edge and IE both don't support it.  OK, let's nix it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So this introduces a new parsing mode that I think didn't use to exist in the spec: parsing an entire input document as text/html while a document is text/plain.

Testcase: https://jsfiddle.net/s7Lhdkxh/3/ or http://web.mit.edu/bzbarsky/www/testcases/document/document-open-2.html

And there's no wpt test coverage around this, afaict.... nor around what happens if you call document.write() in a text/plain document that is still loading.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I am going to take a guess that document.write() should continue parsing as plaintext when done during the load of a plaintext document, and proceed on that basis.  Someone needs to write a test for that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove support for the 'type' parameter to document.open

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962879 [details] [diff] [review]
Remove support for the 'type' parameter to document.open

That has test failures....

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8962879 [details] [diff] [review]
Remove support for the 'type' parameter to document.open

Review of attachment 8962879 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +12266,5 @@
>    }
>  
>    mCachedEncoder = nullptr;
>    mContentType = aType;
> +  mContentTypeForWriteCalls = aType;

nit: Kinda confused by this addition. The only place mContentTypeForWriteCalls is used appears to be after the mContentTypeForWriteCalls.AssignLiteral in nsHTMLDocument::Open (which happens after the call to SetContentTypeInternal), so isn't this just going to get reset no matter what? Just trying to make sure I understand the logic flow here, don't think it affects anything though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> after the mContentTypeForWriteCalls.AssignLiteral in nsHTMLDocument::Open

It's used in Close() and Write().  You can do Write() without doing Open().  That's the whole point: I didn't want to hardcode Write() always parsing as HTML, in case someone does a Write() mid-load on a text document.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, that's what I was missing, thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

I guess the test failures are all for the tests for bug 659763.

Comment 13

[Anne (:annevk)]

> I am going to take a guess that document.write() should continue parsing as plaintext when done during the load of a plaintext document, and proceed on that basis.

It's unclear to me how this can be tested. Won't the insertion point be undefined? I tried to trickle a text/plain load and then document.write() into it, but that'll always replace the contents of the document.

I'll add some tests about invoking document.write() without having invoked document.open() though. That should generally do the same as if you had invoked document.open() first, but it'd be good to test.

As for document's content type, should we consider updating it in the "document open steps" to "text/html"? All browsers agree on it staying "text/plain" today, but their behavior is not aligned with that.

Comment 14

[Anne (:annevk)]

I created https://github.com/w3c/web-platform-tests/pull/10209 to test document.write() on an image/png and text/plain document. For some reason it fails in Firefox (times out on loading some resources), but works fine in Chrome and Safari (who do fail rather vaguely on the image/png case but at least without a timeout). Hope this is what comment 5 needed.

(Note that for now I just tested that contentType remains untouched. We could change that if we wanted to, but since nothing really depends on what it says, I'm not sure it's worth it.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove support for the 'type' parameter to document.open

Just like the previous version, exept for the test fixes

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I created https://github.com/w3c/web-platform-tests/pull/10209 

Thank you.  That tests the behavior when doing a write-after-load (with an implicit open).  It would be interesting, if possible, to test a write-during-load... that's the one I could not figure out how to test.

> For some reason it fails in Firefox (times out on loading some resources)

Because the test doesn't document.close() the documents it opens, so they're never done loading.

> but since nothing really depends on what it says

Last I checked, there were parts of the browser UI in various browsers that were enabled or not depending on the content type of the thing being viewed.  But I last checked a while ago.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cdb625f0ce
Remove support for the 'type' parameter to document.open.  r=qdot

Comment 18

[Anne (:annevk)]

> It would be interesting, if possible, to test a write-during-load...

I don't know if you saw, but I speculated a bit about this on IRC and I don't think it's possible. The only way to get an insertion point is with a script end tag or document.open() call.

And if you move an element then I think it's still the original parser in control, though I could not really get Firefox to execute the second script here using Live DOM Viewer so that avenue of testing is also closed (probably worthy of further study though):

  <iframe src=image></iframe>
  <div>
   <script>frames[0].document.body.appendChild(document.currentScript.parentNode);</script>
   <script>document.write("test")</script>
  </div>

> Last I checked, there were parts of the browser UI in various browsers that were enabled or not depending on the content type of the thing being viewed.

Do you remember where? Right click does not really show anything for

  http://software.hixie.ch/utilities/js/live-dom-viewer/?%3Ciframe%20src%3Dimage%3E%3C%2Fiframe%3E

However, given that you can also manipulate the page through DOM manipulation I no longer see the point of resetting document's content type for this legacy API.

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/72cdb625f0ce

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Do you remember where? 

There's various bits in the context menu and find code in Firefox, but it mostly seems to be about text/* vs not, so treats HTML and plaintext identically.  See https://searchfox.org/mozilla-central/search?q=mimeTypeIsTextBased

In other browsers I recall issues around whether devtools were enabled and such, but I don't recall details and they might have changed behavior.

The main worry here, of course, is sites effectively disabling parts of the browser UI in user-hostile ways.  And yes, the fact that you can just inject stuff via direct DOM manipulation does make this less of a concern, I agree.

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10305 for changes under testing/web-platform/tests

Comment 22

[Kohei Yoshino [:kohei]]

Posted the site compatibility note just in case: https://www.fxsitecompat.com/en-CA/docs/2018/non-standard-type-argument-for-document-open-is-no-longer-supported/