Calling GLFW.setWindowSize with the screen size leads to browser error

[ypujante]

This issue is created from the PR Review.

The code at issue is:

        if (width == screen.width && height == screen.height) {
          Browser.requestFullscreen();
        } else { // ...

When the calling code calls GLFW.setWindowSize and provides width and height to be the screen width and height, it ends up calling Browser.requestFullscreen. Unfortunately, this raises this error: Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture.

When Browser.requestFullscreen is called when a user clicks on a button (for example in the default shell), this works. But when it is called directly it fails.

As requested in the PR, I am reverting the changes I made in the PR and I create a separate issue for it.

I will then make another PR for this problem only, which should also allow me to write a test unique to this error.

Of note: there is another piece of code that does a similar call on window creation and I bet it triggers the same issue. I will also add a test for it.

[ypujante]

An "easy" way to reproduce the problem is with Google Chrome, because when selecting the menu "View/Enter Full screen", it goes into fullscreen mode with no chrome whatsoever (both Firefox and Safari keep the search bar, ....) and as a result, the width and height provided are exactly the size of the screen.

This leads to this outcome

and this stack trace:

imgui_raylib_cmake.js:7536 Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture.
requestFullscreen @ imgui_raylib_cmake.js:7536
imgui_raylib_cmake.js:7536 Uncaught (in promise) TypeError: Permissions check failed
    at Object.requestFullscreen (imgui_raylib_cmake.js:7536:25)
    at Object.setWindowSize (imgui_raylib_cmake.js:8612:21)
    at _glfwSetWindowSize (imgui_raylib_cmake.js:8877:59)
    at imports.<computed> (imgui_raylib_cmake.js:8975:35)
    at SetWindowSize (rcore.c:1671)
    at imgui_raylib_cmake.wasm.onCanvasSizeChanged(int, EmscriptenUiEvent const*, onCanvasSizeChanged(int, EmscriptenUiEvent const*, void*) (main.cpp:106)
    at imgui_raylib_cmake.wasm.dynCall_iiii (imgui_raylib_cmake.wasm:0x6f80ec)
    at ret.<computed> (imgui_raylib_cmake.js:9010:35)
    at imgui_raylib_cmake.js:691:14
    at imgui_raylib_cmake.js:6699:43

[sbc100]

But when you select "View/Enter Full screen" isn't that a "user gesture" ?

[ypujante]

Yes it is clearly a user triggered event, yet for some reason I do not understand it is not treated as a "user gesture".

Also, that would not cover the use-case when the client code (raylib, imgui, etc...) simply calls the API with the exact size of the screen, for example to restore a previous session, or whatever...

In my demo, I implemented it this way:

      if(ImGui::Button("Fullscreen"))
      {
        EM_ASM(
          const fs = document.getElementById("fullscreen");
          if(fs)
            fs.click();
          );
      }

And this works because it is a "click" of a button. If instead I implement a code like this:

      frame++;
      if(frame == 60 * 5)
      {
        int screenWidth = EM_ASM_INT("return screen.width;");
        int screenHeight = EM_ASM_INT("return screen.height;");
        SetWindowSize(screenWidth, screenHeight);
      }

then it triggers the problem.

Doing it this way I can reproduce it on Firefox with a slightly different error message:

Request for fullscreen was denied because Element.requestFullscreen() was not called from inside a short running user-generated event handler.

[sbc100]

In the backtrace above what is at the very top of the stack trace? i.e. what triggered the calling of Object.requestFullscreen? did you include the full backtrace that the browser gave you?

[ypujante]

Yes for the backtrace:

[sbc100]

What is on line 6701 (the first call in the stack)? Is it some kind of click handler? Why doesn't the browser show the function name there?

[ypujante]

  var registerUiEventCallback = (target, userData, useCapture, callbackfunc, eventTypeId, eventTypeString, targetThread) => {
      if (!JSEvents.uiEvent) JSEvents.uiEvent = _malloc(36);
  
      target = findEventTarget(target);
  
      var uiEventHandlerFunc = (e = event) => {
        if (e.target != target) {
          // Never take ui events such as scroll via a 'bubbled' route, but always from the direct element that
          // was targeted. Otherwise e.g. if app logs a message in response to a page scroll, the Emscripten log
          // message box could cause to scroll, generating a new (bubbled) scroll message, causing a new log print,
          // causing a new scroll, etc..
          return;
        }
        var b = document.body; // Take document.body to a variable, Closure compiler does not outline access to it on its own.
        if (!b) {
          // During a page unload 'body' can be null, with "Cannot read property 'clientWidth' of null" being thrown
          return;
        }
        var uiEvent = JSEvents.uiEvent;
        HEAP32[((uiEvent)>>2)] = e.detail;
        HEAP32[(((uiEvent)+(4))>>2)] = b.clientWidth;
        HEAP32[(((uiEvent)+(8))>>2)] = b.clientHeight;
        HEAP32[(((uiEvent)+(12))>>2)] = innerWidth;
        HEAP32[(((uiEvent)+(16))>>2)] = innerHeight;
        HEAP32[(((uiEvent)+(20))>>2)] = outerWidth;
        HEAP32[(((uiEvent)+(24))>>2)] = outerHeight;
        HEAP32[(((uiEvent)+(28))>>2)] = pageXOffset;
        HEAP32[(((uiEvent)+(32))>>2)] = pageYOffset;
// ====> 6701 is the if
        if (((a1, a2, a3) => dynCall_iiii.apply(null, [callbackfunc, a1, a2, a3]))(eventTypeId, uiEvent, userData)) e.preventDefault();
      };

[sbc100]

Strange.. that certainly looks like its happening in a user action callback.. not sure why you would be seeing that error. I guess my understanding of user gesture wrong.

[ypujante]

I know :( not an expert here either.

I am going to spend a bit of time to see if I can come up with a workable solution. Hopefully I can.

The challenge is that try/catch does not work because requestFullscreen is an async api so there is no exception thrown (which also makes writing a test a bit of a challenge).

[ypujante]

@sbc100 I have implemented a fix for this issue and was able to add a test on my personal fork/branch. I will create a PR after my other PR is approved/merged (since the changes are built on top of these other changes)

[ypujante]

@sbc100 The more I think about this issue, the more I think that the original implementation of emscripten/glfw was trying to address a problem or use case that is now causing a bigger issue.

I personally don't understand why calling glfwSetWindowSize with a size that happens to be the exact size of the current monitor automatically calls Browser.requestFullscreen. That seems like a totally unwanted side effect. If you look at both SDL and glut emscripten implementations, none of this libraries do this.

I think it would make more sense to entirely simplify the code:

From

      if (GLFW.active.id == win.id) {
        if (width == screen.width && height == screen.height) {
          Browser.requestFullscreen();
        } else {
          Browser.exitFullscreen();
          Browser.setCanvasSize(width, height);
          win.width = width;
          win.height = height;
        }
      }

to

      if (GLFW.active.id == win.id) {
          Browser.setCanvasSize(width, height);
          win.width = width;
          win.height = height;
      }

and if the client code wants to trigger fullscreen, then it can invoke Module.requestFullscreen as a click handler on a button or something like that (raylib does this for example).

That being said I understand that this change has backward compatibility implications and I assume that would be a non starter. So I am wondering if you would be open to have some conditional way of using the old way or the new way. Essentially something like:

      if (GLFW.active.id == win.id) {
        if (GLFW.autoFullscreenMode) {
          if (width == screen.width && height == screen.height) {
            Browser.requestFullscreen();
          } else {
            Browser.exitFullscreen();
            Browser.setCanvasSize(width, height);
            win.width = width;
            win.height = height;
          }
        } else {
          Browser.setCanvasSize(width, height);
          win.width = width;
          win.height = height;
        }
      }

or

      if (GLFW.active.id == win.id) {
#if GLFW_DISABLE_AUTO_FULLSCREEN_MODE
        Browser.setCanvasSize(width, height);
        win.width = width;
        win.height = height;
#else
        if (width == screen.width && height == screen.height) {
          Browser.requestFullscreen();
        } else {
          Browser.exitFullscreen();
          Browser.setCanvasSize(width, height);
          win.width = width;
          win.height = height;
        }
#endif
      }

What do you think?

[sbc100]

I think most likely the GLFW implementation was just an accident of history. I think we should just change it to match what SDL /glut is doing.

I would rather avoid adding a new setting if possible... perhaps we can do that if we find that there are folks who actually want/need it. Lets just mention this in changelog and/or on the mailing list. If anyone really wants the old behaviour we can consider adding a setting, but it seems very unlikely.

[ypujante]

@sbc100 thank you for merging my changes. Do not hesitate to ping me if there are any concerns or questions about it.