Closed
Bug 1444872
Opened 7 years ago
Closed 7 years ago
Remove processing for document.open()'s type parameter
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: annevk, Assigned: bzbarsky)
References
Details
(Keywords: site-compat)
Attachments
(2 files)
(deleted),
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
It's not implemented in Chrome, Edge, or Safari.
HTML Standard: https://github.com/whatwg/html/pull/3559.
Reporter | ||
Comment 1•7 years ago
|
||
Tests are at https://github.com/w3c/web-platform-tests/pull/9978.
Reporter | ||
Comment 2•7 years ago
|
||
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.)
Priority: -- → P3
Assignee | ||
Comment 3•7 years ago
|
||
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?
Flags: needinfo?(annevk)
Assignee | ||
Comment 4•7 years ago
|
||
OK, just tested, and https://jsfiddle.net/s7Lhdkxh/ shows that Edge and IE both don't support it. OK, let's nix it.
Assignee: nobody → bzbarsky
Flags: needinfo?(annevk)
Assignee | ||
Comment 5•7 years ago
|
||
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.
Flags: needinfo?(d)
Flags: needinfo?(annevk)
Assignee | ||
Comment 6•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8962879 -
Flags: review?(kyle)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8962879 [details] [diff] [review]
Remove support for the 'type' parameter to document.open
That has test failures....
Attachment #8962879 -
Flags: review?(kyle)
Comment 9•7 years ago
|
||
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.
Attachment #8962879 -
Flags: review+
Assignee | ||
Comment 10•7 years ago
|
||
> 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•7 years ago
|
||
Ok, that's what I was missing, thanks!
Assignee | ||
Comment 12•7 years ago
|
||
I guess the test failures are all for the tests for bug 659763.
Reporter | ||
Comment 13•7 years ago
|
||
> 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.
Reporter | ||
Comment 14•7 years ago
|
||
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.)
Flags: needinfo?(d)
Flags: needinfo?(annevk)
Assignee | ||
Comment 15•7 years ago
|
||
Just like the previous version, exept for the test fixes
Attachment #8963137 -
Flags: review?(kyle)
Assignee | ||
Comment 16•7 years ago
|
||
> 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.
Updated•7 years ago
|
Attachment #8963137 -
Flags: review?(kyle) → review+
Comment 17•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cdb625f0ce
Remove support for the 'type' parameter to document.open. r=qdot
Reporter | ||
Comment 18•7 years ago
|
||
> 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 20•7 years ago
|
||
> 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.
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10305 for changes under testing/web-platform/tests
Comment 22•6 years ago
|
||
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/
Keywords: site-compat
Updated•6 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•