Remove document event listeners before setting readyState to complete#1479
Remove document event listeners before setting readyState to complete#1479WojciechNowak wants to merge 1 commit intojsdom:masterfrom
Conversation
|
Hmm. This seems wrong to me. |
|
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: Is emmiting this event defined by spec or is it some kind of jsdom hack? Maybe we should consider removing only DOMContentLoaded from eventListeners? |
|
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. |
|
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? |
|
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. |
… on document close.
452c29a to
102fd84
Compare
|
I checked window.close() function and I think I found the bug. I have provided a new solution. What do you think about it? |
|
Oh, wow, excellent find, thank you! Yes, I'll include this in the next release (probably this weekend). |
|
Great, I'm glad I could help. |
|
Merged as 868560b; thanks! |
Hi.
Test scenario:
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