325352 - document.open doesn't clear the document

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P5)

Description

[Alexey Proskuryakov]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; ru-ru) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

This is split from WebKit bug <http://bugzilla.opendarwin.org/show_bug.cgi?id=4395>.

document.open() should clear the document, as described in
<http://www.mozilla.org/docs/dom/domref/dom_doc_ref51.html> and
<http://developer.mozilla.org/en/docs/document.open>. 

Steps to reproduce: open the attached test case. In the bottom frame, there
should be only one line "line".

The test case works as expected in WinIE 6.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Alexey Proskuryakov]

Attachment: test case

Comment 2

[Martijn Wargers (dead)]

Attachment: More minimal testcase

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

It seems like IE implicitly calls close() from open(). Should we do the same?

Comment 4

[Thomas 'PointedEars' Lahn]

We should do according to HTMLDocument::open() of W3C DOM Level 2 HTML, which in this case is what MSHTML-based UAs like IE (maybe by accident) do:

,-<http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-72161170>
|
| open
|   Open a document stream for writing. If a document exists in the target,
|   this method clears it.

Comment 5

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

The behavior of document.open() is now covered by HTML5 making this an HTML5 conformance issue. For my ongoing HTML5 parsing efforts, I have the code for clearing the document, which isn't a big deal.

The problematic thing still appears to be that removing the root element and inserting a new one confuses the scroll bar (on iframes at least).

http://livedom.validator.nu/ clears the root element and creates a new one, and the scrollbar gets confused.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It seems like IE implicitly calls close() from open(). Should we do the same?

That would make sense.  Note that the testcases for this bug can be fixed without removing the root on open(), and removing the root on open() won't fix the testcases here (because open() is currently a no-op if we have a parser).

So removing dependency on bug 78070.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I filed bug 486741 on removing the root element hackery, by the way.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm. The behavior of Gecko after bug 1489308 does not match the behavior of Chrome on attachment 210306 [details]. As far as I can tell, Gecko is following the spec as currently written. What is Chrome doing? Timothy, do you know?

Comment 10

[Timothy Gu [:TimothyGu]]

Some digging around shows that Blink clears the parser in document.close(), allowing a subsequent synchronous document.open() to work. This behavior was in fact inherited from a 2003 change in WebKit, and probably the cause of this longstanding bug.

I was not aware of this behavior difference (as I focused mostly on document.open() itself), but it might worth filing an HTML bug about it. Boris, would implementing the behavior I just described in Gecko be feasible?

Comment 11

[Boris Zbarsky [:bzbarsky]]

The issue is that not that document.open() works after document.close(). That part is interoperable across browsers. The issue is that in Blink it looks like this loop:

	for (i = 0; i < 5; i++) {
		document.open();
		document.write("There should be only one line of text.<br>");
	}

ends up resetting the DOM on each open() call, even though there is a parser (the one the previous open() call set up!), so the end result only has one line of text in the DOM, not 5 lines. This seems like squarely an issue of what document.open() does in Blink, as far as I can tell.

Comment 12

[Timothy Gu [:TimothyGu]]

Oh oops, let's try this once more.

The document open steps say:

5. If document has an active parser whose script nesting level is greater than 0, then return document.

…

14. Create a new HTML parser and associate it with document.

The definition of script nesting level says:

parsers have a script nesting level, which must be initially set to zero

In the for-loop, after the document.open() call, a new parser is created. However, when a parser is created, the script nesting level is (re)set to zero, only to be in/decremented when <script> tags are seen in the parsed HTML text. So when the next document.open() call executes, it would see an HTML parser that is not running script, and thus overwrite it.

Indeed, if I try to call document.open() again in the document.write() call in a <script> tag, like

function mainFunc() {
	document.open();
	document.write("will not be visible<br>");
	document.close();

	for (i = 0; i < 5; i++) {
		document.open();
		document.write(`
			There should be two lines of text.<br>
			<script>
				document.open();
				document.write('extra');
			</${''}script>
		`);
	}
}

Chrome ignores the extra document.open() calls and print both “There should be two lines of text” and “extra”.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

Henri, does our parser have this concept of "script nesting level" at all?

It sounds like there are no web platform tests for this script nesting level bit, unfortunately... :(

Comment 14

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #13)

Henri, does our parser have this concept of "script nesting level" at all?

I think https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/parser/html/nsHtml5Parser.h#280 is it, and the naming mismatch is thanks to the counter having been named according to previous spec concept. That is, I think I figured that a counter is needed when the spec didn't say it that way and once the spec agreed, it changed the terminology.

Comment 15

[Boris Zbarsky [:bzbarsky]]

It sounds like there are no web platform tests for this script nesting level bit, unfortunately... :(

Yep, definitely no tests, in that fixing this bug does not lead to any changes in wpt results. Need to write some tests...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 325352 part 1. Align our "defined insertion point" concept more closely with the spec's "script nesting level" concept. r=hsivonen

This also exposes an accessor for whether the parser has a nonzero script nesting level.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 325352 part 2. Don't no-op document.open if our parser has a script nesting level equal to 0. r=hsivonen

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/990c8a382cf3
part 1.  Align our "defined insertion point" concept more closely with the spec's "script nesting level" concept.  r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/264fe248bca5
part 2.  Don't no-op document.open if our parser has a script nesting level equal to 0.  r=hsivonen

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/990c8a382cf3
https://hg.mozilla.org/mozilla-central/rev/264fe248bca5

Comment 20

[Boris Zbarsky [:bzbarsky]]

I backed out part 2 to fix bug 1550524. I'll reland it, along with fixes for bug 1550524, after the next branchpoint.

Comment 21

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

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

Comment 22

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

Upstream PR merged

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c27a9d783734
part 2.  Don't no-op document.open if our parser has a script nesting level equal to 0.  r=hsivonen

Comment 24

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c27a9d783734