[DRAFT] Fix for #20600 #20695
Closed
[DRAFT] Fix for #20600 #20695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added Browser.isHiDPIAware flag that gets initialized from Module.isHiDPIAware - from the webgpu samples (https://webgpu.github.io/webgpu-samples/samples/helloTriangle), the gist of the logic is to scale canvas.width (resp. canvas.height) by the devicePixelRatio and set canvas.clientWidth to the width - coordinates are then scaled down to reflect what the client is expecting
- calling Browser.requestFullscreen raises "Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture"
- handle case where the canvas size is fixed (default shell) - handle case where request full screen is issued by the user (default shell)
- created issue emscripten-core#20600 to implement in a separate PR
- the rationale is that the client can simply call Module.setHiDPIAware based on the needs
- ironically, although the call was wrong, it ended up doing the right thing...
# Conflicts: # src/library_glfw.js
# Conflicts: # src/library_browser.js
- mocking devicePixelRatio allow to write a comprehensive test - added handling dynamic changes to devicePixelRatio by installing a listener
- unclear why it works locally
The issue is that requestFullscreen (resp. exitFullscreen) is an asynchronous call which can fail: as a result, the code must handle errors using the Promise mechanism.
- now requestFullscreen returns a promise (since it was returning nothing before, any calling code should not be affected)
- now exitFullscreen returns a promise or null (was returning a boolean so the SDL code
if (Browser.exitFullscreen()) {
return 1;
}
is still valid)
- requestFullscreen handles the error first and call fullscreenChange() which has the effect of removing the div that was set
- requestFullscreen then propagates the error so that the calling code can handle it as well
- glfw detects the error and act appropriately (similarly for exitFullscreen)
Note that this check in contains a bunch of logging to trace what is going on since the asynchronicity makes it hard to trace. These logs will be removed prior to submitting the PR.
- it seems that with Google Chrome the default is to hide navigation, but on Firefox it keeps it (search bar, shortcuts, etc...) which does not seem what fullscreen is all about. Adding the option makes it consistent.
- test that when the fullscreen request is denied, glfw handles the failure properly
|
Closing this draft PR as it is replaced by this other one |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am creating a draft PR for the fix for #20600 so that you can provide feedback if you want to.
Mostly the goal of the PR is to check that the tests run on CI
For the implementation of the fix itself, the key is to handle the errors coming from the promise.
Browser.requestFullscreennow handles the error and return the promise for handling by the clients (this function did not return anything before so the other calling code is not affected)Browser.exifFullscreennow handles the error and return the promise for handling by the clients (or null). This function used to return a boolean instead and as far as I can tell, the only calling code that was using the return value islibrary_sdl.js(if (Browser.exitFullscreen())) so no impact there