Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle use case when canvas size is already controlled by css #20956

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

ypujante
Copy link
Contributor

When I was checking the SDL implementation I noticed that they were doing something like this:

    emscripten_set_canvas_element_size(wdata->canvas_id, 1, 1);
    emscripten_get_element_css_size(wdata->canvas_id, &css_w, &css_h);
    wdata->external_size = SDL_floor(css_w) != 1 || SDL_floor(css_h) != 1;

Then they use this flag wdata->external_size to not rewrite the css property, for example:

            if (!window_data->external_size && window_data->pixel_ratio != 1.0f) {
                emscripten_set_element_css_size(window_data->canvas_id, w, h);
            }

I changed the glfw code to use the same "trick" and implement a similar logic. This way there is no need to wrap the canvas in a <div> when the canvas needs to be full window (width: 100% / height: 100%)

- implementation is inspired by SDL implementation
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

lgtm % nits

src/library_glfw.js Outdated Show resolved Hide resolved
src/library_glfw.js Outdated Show resolved Hide resolved
src/library_glfw.js Outdated Show resolved Hide resolved
@ypujante
Copy link
Contributor Author

@sbc100 I applied your code review. Thank you.

@ypujante
Copy link
Contributor Author

All tests ran.

The primary test for this change is working fine:

  test_glfw3_hi_dpi_aware (test_browser.browser) ... cache:INFO: generating system asset: symbol_lists/0bef552e4ea38aecbf95a4972a5e9b306b78bc15.json... (this will be cached in "/root/cache/symbol_lists/0bef552e4ea38aecbf95a4972a5e9b306b78bc15.json" for subsequent builds)
cache:INFO:  - ok
ok (1.179s)

The only failing test in test-browser-firefox is

FAIL [121.660s]: test_wasmfs_fetch_backend_bigint (test_browser.browser)

  test_wasmfs_fetch_backend_bigint (test_browser.browser) ... cache:INFO: generating system asset: symbol_lists/1d95e008792744511b35b1f1168933b8b40d1d9e.json... (this will be cached in "/root/cache/symbol_lists/1d95e008792744511b35b1f1168933b8b40d1d9e.json" for subsequent builds)
cache:INFO:  - ok
[unresponsive tests: 1]
common:INFO: Restarting browser process
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
common:INFO: Launching browser: ['/root/firefox/firefox', '-headless', '-profile', '/root/tmp-firefox-profile/']
[test error (see below), automatically retrying]
Expected to find '/report_result?exit:0
' in '[no http server activity]
', diff:

--- expected
+++ actual
@@ -1 +1 @@
-/report_result?exit:0
+[no http server activity]

This failing test does not seem to be related to my changes (note that test_glfw3_hi_dpi_aware is skipped for the suite test-browser-firefox)

Thank you

@ypujante
Copy link
Contributor Author

@sbc100 I just pushed a small addition which does 2 things:

  • adds a test for the new feature (use case 8), making sure that if the canvas has a css override, then it is doing what the code change is supposed to be doing (meaning the override takes precedence)
  • store/restore the original canvas size to be a good citizen. This is not really related to my changes, and this glfw implementation has never done it before, but I figured it made sense to restore the canvas the way it was before glfwCreateWindow is called

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

test/browser/test_glfw3_hi_dpi_aware.c Outdated Show resolved Hide resolved
@sbc100 sbc100 enabled auto-merge (squash) December 21, 2023 17:36
@sbc100 sbc100 merged commit 3f299c3 into emscripten-core:main Dec 21, 2023
25 of 27 checks passed
ypujante added a commit to ypujante/emscripten that referenced this pull request Dec 22, 2023
sbc100 pushed a commit that referenced this pull request Jan 2, 2024
@ypujante ypujante deleted the check-canvas-css branch March 23, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants