Platform: remove deprecated Emscripten event target behavior support.

This makes the minimal supported Emscripten version 1.39.5. With some
more effort this could be changed to 1.38.27, but I don't think anybody
needs that.

doc/changelog.dox

@@ -48,6 +48,9 @@ See also:
 -   Minimal supported CMake version for Emscripten WebGL 2 compilation is now
     3.13, in order to be able to set linker options for enabling WebGL 2
     without having to pollute the global `CMAKE_EXE_LINKER_FLAGS` variable.
+-   Minimal supported Emscripten version is now 1.39.5 which implicitly sets
+    `-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1`, as the deprecated
+    code paths were removed.
 
 @subsection changelog-latest-new New features
 

doc/platforms-html5.dox

@@ -620,8 +620,6 @@ down the build significantly or being very recent:
     (new since 1.38.27) --- note that, while this leads to significant size
     savings, it currently requires significant amount of work on the
     application side and Magnum was not yet tested with this option enabled
--   [`-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1`](https://github.com/emscripten-core/emscripten/pull/7977)
-    (new since 1.38.27) --- leads to minor code size savings as well
 
 On the other hand, if you need shorter compile times and don't care much about
 code size (for example when testing on the CIs), consider *removing* some of

src/Magnum/Platform/CMakeLists.txt

@@ -182,6 +182,16 @@ if(MAGNUM_WITH_ANDROIDAPPLICATION)
     add_library(Magnum::AndroidApplication ALIAS MagnumAndroidApplication)
 endif()
 
+if(CORRADE_TARGET_EMSCRIPTEN AND (MAGNUM_WITH_EMSCRIPTENAPPLICATION OR MAGNUM_WITH_SDL2APPLICATION))
+    # This could also set -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1
+    # always and require only 1.38.27 (where this option got added), but that
+    # version is from February 2019 and I don't think anyone actively uses it
+    # anymore.
+    if(EMSCRIPTEN_VERSION VERSION_LESS 1.39.5)
+        message(SEND_ERROR "Emscripten 1.39.5+, which enables DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR by default, is required to build EmscriptenApplication and Sdl2Application. Found version ${EMSCRIPTEN_VERSION}.")
+    endif()
+endif()
+
 # Emscripten application
 if(MAGNUM_WITH_EMSCRIPTENAPPLICATION)
     if(NOT CORRADE_TARGET_EMSCRIPTEN)

src/Magnum/Platform/EmscriptenApplication.cpp

@@ -30,6 +30,7 @@
 #include <emscripten/emscripten.h>
 #include <emscripten/html5.h>
 #include <Corrade/Containers/ArrayView.h>
+#include <Corrade/Containers/ScopeGuard.h>
 #include <Corrade/Containers/StringView.h>
 #include <Corrade/Utility/Arguments.h>
 #include <Corrade/Utility/Debug.h>
@@ -42,13 +43,6 @@
 #include "Magnum/GL/Version.h"
 #endif
 
-/** @todo drop once we don't support < 1.38.27 anymore */
-#ifndef EMSCRIPTEN_EVENT_TARGET_DOCUMENT
-#define EMSCRIPTEN_EVENT_TARGET_DOCUMENT reinterpret_cast<const char*>(1)
-#define EMSCRIPTEN_EVENT_TARGET_WINDOW reinterpret_cast<const char*>(2)
-#define EMSCRIPTEN_EVENT_TARGET_SCREEN reinterpret_cast<const char*>(3)
-#endif
-
 namespace Magnum { namespace Platform {
 
 using namespace Containers::Literals;
@@ -190,7 +184,7 @@ namespace {
             ERROR - [JSC_LANGUAGE_FEATURE] This language feature is only
             supported for ECMASCRIPT6 mode or better: const declaration. */
         char* id = reinterpret_cast<char*>(EM_ASM_INT({
-            var id = Module['canvas'].id;
+            var id = '#' + Module['canvas'].id;
             var bytes = lengthBytesUTF8(id) + 1;
             var memory = _malloc(bytes);
             stringToUTF8(id, memory, bytes);
@@ -199,24 +193,6 @@ namespace {
         #pragma GCC diagnostic pop
         return Containers::String{id, [](char* data, std::size_t) { std::free(data); }};
     }
-
-    bool checkForDeprecatedEmscriptenTargetBehavior() {
-        /* Emscripten 1.38.27 changed to generic CSS selectors from element IDs
-        depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 being
-        set.
-        https://github.com/emscripten-core/emscripten/pull/7977
-        There is no simple way to check for compiler options so check
-        whether the new CSS selectors are being used. If so, it should find
-        canvas#[id] which is any canvas with the ID of Module.canvas.
-        The old target behavior will look for an element with id="canvas#[id]"
-        which could theoretically exist but that's highly unlikely. */
-        bool deprecated = true;
-        Vector2d tempSize;
-        if(emscripten_get_element_css_size(("canvas#" + canvasId()).data(), &tempSize.x(), &tempSize.y()) >= 0) {
-            deprecated = false;
-        }
-        return deprecated;
-    }
 }
 
 EmscriptenApplication::EmscriptenApplication(const Arguments& arguments): EmscriptenApplication{arguments, Configuration{}} {}
@@ -317,12 +293,7 @@ bool EmscriptenApplication::tryCreate(const Configuration& configuration) {
 
     std::ostream* verbose = _verboseLog ? Debug::output() : nullptr;
 
-    _deprecatedTargetBehavior = checkForDeprecatedEmscriptenTargetBehavior();
+    _canvasTarget = canvasId();
-    if(_deprecatedTargetBehavior) {
-        Debug{verbose} << "Platform::EmscriptenApplication::tryCreate(): using old Emscripten target behavior";
-    }
-
-    _canvasTarget = (_deprecatedTargetBehavior ? canvasId() : "#"_s + canvasId());
 
     /* Get CSS canvas size and cache it. This is used later to detect canvas
        resizes in emscripten_set_resize_callback() and fire viewport events,
@@ -403,16 +374,9 @@ bool EmscriptenApplication::tryCreate(const Configuration& configuration, const
     _devicePixelRatio = Vector2{Float(emscripten_get_device_pixel_ratio())};
     Debug{verbose} << "Platform::EmscriptenApplication: device pixel ratio" << _devicePixelRatio.x();
 
-    /* Find out which element target strings Emscripten expects. This depends on
-       the DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR compiler option.  */
-    _deprecatedTargetBehavior = checkForDeprecatedEmscriptenTargetBehavior();
-    if(_deprecatedTargetBehavior) {
-        Debug{verbose} << "Platform::EmscriptenApplication::tryCreate(): using old Emscripten target behavior";
-    }
-
     /* Get the canvas ID from Module.canvas, either set by EmscriptenApplication.js
        or overridden/manually set by the user. */
-    _canvasTarget = (_deprecatedTargetBehavior ? canvasId() : "#"_s + canvasId());
+    _canvasTarget = canvasId();
 
     /* Get CSS canvas size and cache it. This is used later to detect canvas
        resizes in emscripten_set_resize_callback() and fire viewport events,
@@ -534,7 +498,7 @@ void EmscriptenApplication::setupCallbacks(bool resizable) {
        changes. Better than polling for this change in every frame like
        Sdl2Application does, but still not ideal. */
     if(resizable) {
-        const char* target = _deprecatedTargetBehavior ? "#window" : EMSCRIPTEN_EVENT_TARGET_WINDOW;
+        const char* target = EMSCRIPTEN_EVENT_TARGET_WINDOW;
         auto cb = [](int, const EmscriptenUiEvent* event, void* userData) -> Int {
             static_cast<EmscriptenApplication*>(userData)->handleCanvasResize(event);
             return false; /** @todo what does ignoring a resize event mean? */
@@ -559,9 +523,8 @@ void EmscriptenApplication::setupCallbacks(bool resizable) {
     emscripten_set_mousemove_callback(_canvasTarget.data(), this, false,
         ([](int, const EmscriptenMouseEvent* event, void* userData) -> Int {
             auto& app = *static_cast<EmscriptenApplication*>(userData);
-            /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is
+            /* Relies on the target being the canvas, which should be always
-               not initialized, so we have to rely on the target being the
+               true for mouse events */
-               canvas. That's always true for mouse events. */
             Vector2i position{Int(event->targetX), Int(event->targetY)};
             MouseMoveEvent e{*event,
                 /* Avoid bogus offset at first -- report 0 when the event is
@@ -586,22 +549,20 @@ void EmscriptenApplication::setupCallbacks(bool resizable) {
        EMSCRIPTEN_EVENT_TARGET_DOCUMENT and EMSCRIPTEN_EVENT_TARGET_WINDOW.
        As the lookup happens with the passed parameter and arrays support
        element lookup via strings, we can unify the code by returning string of
-       1 or 2 if the target is document or window. This changed in Emscripten
+       1 or 2 if the target is document or window. */
-       1.38.27 depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1
-       but we don't want to force this flag on the users so the behavior
-       handles both. */
     /* Note: can't use let or const, as that breaks closure compiler:
         ERROR - [JSC_LANGUAGE_FEATURE] This language feature is only
         supported for ECMASCRIPT6 mode or better: const declaration. */
-    const char* keyboardListeningElement = reinterpret_cast<const char*>(EM_ASM_INT({
+    char* const keyboardListeningElement = reinterpret_cast<char*>(EM_ASM_INT({
         var element = Module['keyboardListeningElement'] || document;
 
         if(element === document) return 1; /* EMSCRIPTEN_EVENT_TARGET_DOCUMENT */
         if(element === window) return 2; /* EMSCRIPTEN_EVENT_TARGET_WINDOW */
         if('id' in element) {
-            var bytes = lengthBytesUTF8(element.id) + 1;
+            var id = '#' + element.id;
+            var bytes = lengthBytesUTF8(id) + 1;
             var memory = _malloc(bytes);
-            stringToUTF8(element.id, memory, bytes);
+            stringToUTF8(id, memory, bytes);
             return memory;
         }
 
@@ -609,20 +570,18 @@ void EmscriptenApplication::setupCallbacks(bool resizable) {
     }));
     #pragma GCC diagnostic pop
 
-    Containers::String keyboardListeningElementStorage;
+    /* If the element is a heap-allocated string, ensure it gets properly freed
-    if(keyboardListeningElement == EMSCRIPTEN_EVENT_TARGET_DOCUMENT) {
+       after */
-        keyboardListeningElement = _deprecatedTargetBehavior ? "#document" : keyboardListeningElement;
+    Containers::ScopeGuard keyboardListeningElementStorage{NoCreate};
-    } else if(keyboardListeningElement == EMSCRIPTEN_EVENT_TARGET_WINDOW) {
+    if(keyboardListeningElement &&
-        keyboardListeningElement = _deprecatedTargetBehavior ? "#window" : keyboardListeningElement;
+       keyboardListeningElement != EMSCRIPTEN_EVENT_TARGET_DOCUMENT &&
-    } else if(keyboardListeningElement && !_deprecatedTargetBehavior) {
+       keyboardListeningElement != EMSCRIPTEN_EVENT_TARGET_WINDOW)
-        keyboardListeningElementStorage = "#"_s + Containers::StringView{keyboardListeningElement};
+    {
-        std::free(const_cast<char*>(keyboardListeningElement));
+        keyboardListeningElementStorage = Containers::ScopeGuard{keyboardListeningElement, std::free};
-        keyboardListeningElement = keyboardListeningElementStorage.data();
     }
 
-    /* Happens only if keyboardListeningElement was set, but did not have an
+    /* Happens only if keyboardListeningElement was set, but wasn't a document
-       `id` attribute. Instead it should be either null or undefined, a DOM
+       or a window and did not have an `id` attribute. */
-       element, `window` or `document`. */
     CORRADE_ASSERT(keyboardListeningElement,
         "EmscriptenApplication::setupCallbacks(): invalid value for Module['keyboardListeningElement']", );
 
@@ -852,9 +811,8 @@ EmscriptenApplication::MouseEvent::Button EmscriptenApplication::MouseEvent::but
 }
 
 Vector2i EmscriptenApplication::MouseEvent::position() const {
-    /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not
+    /* Relies on the target being the canvas, which should be always true for
-       initialized, so we have to rely on the target being the canvas. That's
+       mouse events */
-       always true for mouse events. */
     return {Int(_event.targetX), Int(_event.targetY)};
 }
 
@@ -872,9 +830,8 @@ EmscriptenApplication::MouseMoveEvent::Buttons EmscriptenApplication::MouseMoveE
 }
 
 Vector2i EmscriptenApplication::MouseMoveEvent::position() const {
-    /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not
+    /* Relies on the target being the canvas, which should be always true for
-       initialized, so we have to rely on the target being the canvas. That's
+       mouse events */
-       always true for mouse events. */
     return {Int(_event.targetX), Int(_event.targetY)};
 }
 
@@ -901,9 +858,8 @@ Vector2 EmscriptenApplication::MouseScrollEvent::offset() const {
 }
 
 Vector2i EmscriptenApplication::MouseScrollEvent::position() const {
-    /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not
+    /* Relies on the target being the canvas, which should be always true for
-       initialized, so we have to rely on the target being the canvas. That's
+       mouse events */
-       always true for mouse events. */
     return {Int(_event.mouse.targetX), Int(_event.mouse.targetY)};
 }
 

src/Magnum/Platform/EmscriptenApplication.h

@@ -908,7 +908,6 @@ class EmscriptenApplication {
         Flags _flags;
         Cursor _cursor = Cursor::Arrow;
 
-        bool _deprecatedTargetBehavior{};
         Containers::String _canvasTarget;
 
         #ifdef MAGNUM_TARGET_GL

src/Magnum/Platform/Sdl2Application.cpp

@@ -414,13 +414,6 @@ bool Sdl2Application::tryCreate(const Configuration& configuration) {
     /** @todo don't hardcode "#canvas" here, make it configurable from outside */
     {
         Vector2d canvasSize;
-        /* Emscripten 1.38.27 changed to generic CSS selectors from element
-           IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1
-           being set (which we can't detect at compile time). Fortunately,
-           using #canvas works the same way both in the previous versions and
-           the current one. Unfortunately, this is also the only value that
-           works the same way for both. Further details at
-           https://github.com/emscripten-core/emscripten/pull/7977 */
         emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y());
         _lastKnownCanvasSize = Vector2i{canvasSize};
     }
@@ -650,13 +643,6 @@ bool Sdl2Application::tryCreate(const Configuration& configuration, const GLConf
     /** @todo don't hardcode "#canvas" here, make it configurable from outside */
     {
         Vector2d canvasSize;
-        /* Emscripten 1.38.27 changed to generic CSS selectors from element
-           IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1
-           being set (which we can't detect at compile time). Fortunately,
-           using #canvas works the same way both in the previous versions and
-           the current one. Unfortunately, this is also the only value that
-           works the same way for both. Further details at
-           https://github.com/emscripten-core/emscripten/pull/7977 */
         emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y());
         _lastKnownCanvasSize = Vector2i{canvasSize};
     }
@@ -884,12 +870,9 @@ bool Sdl2Application::mainLoopIteration() {
        size here. But only if the window was requested to be resizable, to
        avoid resizing the canvas when the user doesn't want that. Related
        issue: https://github.com/kripken/emscripten/issues/1731 */
+    /** @todo don't hardcode "#canvas" here, make it configurable from outside */
     if(_flags & Flag::Resizable) {
         Vector2d canvasSize;
-        /* Emscripten 1.38.27 changed to generic CSS selectors from element
-           IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1
-           being set (which we can't detect at compile time). See above for the
-           reason why we hardcode #canvas here. */
         emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y());
 
         const Vector2i canvasSizei{canvasSize};

src/Magnum/Platform/Test/CMakeLists.txt

@@ -50,11 +50,8 @@ if(MAGNUM_WITH_EMSCRIPTENAPPLICATION)
     add_executable(PlatformEmscriptenApplicationTest EmscriptenApplicationTest.cpp)
     target_link_libraries(PlatformEmscriptenApplicationTest PRIVATE
         MagnumEmscriptenApplication MagnumGL)
+    # Enable memory runtime checks
     target_link_options(PlatformEmscriptenApplicationTest PRIVATE
-        # Test that the canvas and keylistener can be found with both the old
-        # and the new Emscripten target behaviors
-        "SHELL:-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=0"
-        # Enable memory runtime checks
         "SHELL:-s SAFE_HEAP=1")
     # TODO Emscripten's own event handling generates float-to-int conversion
     # errors with ASSERTIONS=2, using ASSERTIONS=1 until fixed:
@@ -78,11 +75,8 @@ if(MAGNUM_WITH_EMSCRIPTENAPPLICATION)
     add_executable(PlatformMultipleEmscriptenApplicationTest EmscriptenApplicationTest.cpp)
     target_link_libraries(PlatformMultipleEmscriptenApplicationTest PRIVATE
         MagnumEmscriptenApplication MagnumGL)
+    # Enable memory runtime checks
     target_link_options(PlatformMultipleEmscriptenApplicationTest PRIVATE
-        # Test that the canvas and keylistener can be found with both the old
-        # and the new Emscripten target behaviors
-        "SHELL:-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1"
-        # Enable memory runtime checks
         "SHELL:-s SAFE_HEAP=1")
     # TODO Emscripten's own event handling generates float-to-int conversion
     # errors with ASSERTIONS=2, using ASSERTIONS=1 until fixed: