Skip to content

Conversation

@WojciechNowak
Copy link

Hi.

Test scenario:

  1. load page with <script>document.addEventListener("DOMContentLoaded", function() {})</script>
  2. close window.

Problem: On window close, document implementation fires DOMContentLoaded event, which in turn executes function added within addEventListener.

Proposed solution: clear document eventListeners before setting it to readyState.

Tests added to test/window/script.js. If pull request is accepted and there is a requirement to write tests in Mocha please tell me in which file it should be rewritten.

Best regards,
Wojciech Nowak

@domenic
Copy link
Member

domenic commented May 10, 2016

Hmm. window.close() already handles this use case. But document.close() should not blow away event listeners (see spec).

This seems wrong to me.

@WojciechNowak
Copy link
Author

Event listeners added to a window are in fact cleared and the problem doesn't exist. However we can see a comment in close function in document-impl.js:
// Set the readyState to 'complete' once all resources are loaded.
// As a side-effect the document's load-event will be dispatched.
...
ev.initEvent("DOMContentLoaded", false, false);
this.dispatchEvent(ev);

Is emmiting this event defined by spec or is it some kind of jsdom hack? Maybe we should consider removing only DOMContentLoaded from eventListeners?

@domenic
Copy link
Member

domenic commented May 10, 2016

document.close() should definitely never remove any event listeners. If you are trying to close the window and stop event listeners, use window.close(), not document.close().

It sounds like it might be a separate issue where document.close() should not be what fires the DOMContentLoaded event. Instead, the parser probably should.

@WojciechNowak
Copy link
Author

I call window.close which in turn calls document.close which causes DOMContentLoaded to be fired. If proposed solution to this problem is incorrect what should be done next? Shall I create an issue to track this bug?

@domenic
Copy link
Member

domenic commented May 11, 2016

Yes. window.close() needs to clear the event handlers; if it is not doing so then something is wrong. But document.close() should not clear event handlers.

@WojciechNowak WojciechNowak force-pushed the Remove_Document_DOMContentLoaded_Listeners branch from 452c29a to 102fd84 Compare May 11, 2016 20:54
@WojciechNowak
Copy link
Author

I checked window.close() function and I think I found the bug. I have provided a new solution. What do you think about it?

@domenic
Copy link
Member

domenic commented May 11, 2016

Oh, wow, excellent find, thank you! Yes, I'll include this in the next release (probably this weekend).

@WojciechNowak
Copy link
Author

Great, I'm glad I could help.

@domenic
Copy link
Member

domenic commented May 15, 2016

Merged as 868560b; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants