Don’t remove JS event handlers on canvas on deleteContext

[msorvig]

We are done with the context, but might want to continue to use the canvas.

[welcome]

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

[msorvig]

See for QTBUG-74850 for context - this kept us scratching our heads for a while :)

[kripken]

Thanks @msorvig! That does look like a bug, good find :)

Looks like this broke a test on CI, browser.test_html5_webgl_destroy_context. I believe the test checks that we don't get a context_lost or context_restored callback for a destroyed context, so what I think is happening is that we should be deleting those two events (which relate to the context, and not the canvas), but not all of them (because as you said, we may want to keep using the canvas).

Please also improve the test to check for the bug you are fixing here. I think we can replace the final emscripten_async_call with sending an event to the canvas, like a mouse event or a key event, and in the handler we can call that finish function used there.

Let me know if you have any questions about our test suite!

[msorvig]

Yep, I suspected there was more to this. Thanks for the update! I'll see if I can get the tests running.

[msorvig]

Was able to run the test (test_html5_webgl_destroy_context) now.

However, I'm not sure if works as intended:

• It does not appear to actually check for context_lost/restored. It refers to context_lost_desired() in comments, but there is no such function.
• It calls emscripten_set_webglcontextlost_callback("#canvas") to install an event handler on the canvas, and the experts that calling emscripten_webgl_destroy_context() will uninstall it. emscripten_webgl_destroy_context() should certainly clean up internal state, but I'm not sure if it should remove externally installed event handlers.
• As far as I can see it is not possible to uninstall event handlers installed by emscripten_set_webglcontextlost_callback() only, since EventTarget.removeEventListener() removes all matching event listeners. (as documented at https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#Matching_event_listeners_for_removal)

Does this make sense to you? Let me know :)

[juj]

* It does not appear to actually check for context_lost/restored. It refers to context_lost_desired() in comments, but there is no such function.

It looks like the comment

  // When we force a context loss, we should get an event, i.e. context_lost_desired() should get called.

is something old/lost in refactor. Let's remove that line altogether.

* As far as I can see it is not possible to uninstall event handlers installed by emscripten_set_webglcontextlost_callback() only

Let's add a new function

    // Removes all event handlers of specific types on the given DOM element. Call with e.g. _JSEvents_removeSpecificHandlersOnTarget(canvas, ['webglcontextlost', 'webglcontextrestored'])
    JSEvents_removeSpecificHandlersOnTarget__deps: ['$JSEvents'],
    JSEvents_removeSpecificHandlersOnTarget: function(target, eventTypesList) {
      for(var i = 0; i < JSEvents.eventHandlers.length; ++i) {
        if (JSEvents.eventHandlers[i].target == target && 
            eventTypesList.indexOf(JSEvents.eventHandlers[i].eventTypeString) != -1) {
            JSEvents._removeHandler(i--);
         }
      }
    },

that enables removing events of specific types. Note that this function should not live inside the JSEvents object, as that object was bad design practice - a standalone function is better for DCE.

In library_html5.js emscripten_webgl_destroy_context__deps, let's add a new dependency to JSEvents_removeSpecificHandlersOnTarget in the list, and that function can call _JSEvents_removeSpecificHandlersOnTarget(GL.contexts[contextHandle].canvas, ['webglcontextlost', 'webglcontext'restored']);` to erase the event handlers.

[sbc100]

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

[Evengard]

Any news on this?

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.