Skip to content

Conversation

@Grey41
Copy link

@Grey41 Grey41 commented Dec 21, 2025

Description

In the old code, we tried to detect whether the canvas size was controlled with css by setting the canvas size to 1, then using emscripten_set_element_css_size to see if anything changed.

I suggest we leave the canvas size alone, except when the SDL_WINDOW_FILL_DOCUMENT flag is set. This means the canvas doesn't unexpectedly mutate (or its css). Also, on destroy, it's best just to clean up and not to mutate the canvas.

Finally, when SDL_WINDOW_HIGH_PIXEL_DENSITY exists, work backwards from the canvas size. For example:

window.width = canvas.width / pixelRatio
window.height = canvas.height / pixelRatio

This is intuitive because the canvas is setting the precedent for the window size, instead of SDL trying to guess by peering into the css.

@slouken slouken requested a review from icculus December 21, 2025 19:52
@slouken slouken added this to the 3.4.0 milestone Dec 21, 2025
@slouken
Copy link
Collaborator

slouken commented Dec 21, 2025

Conceptually this seems like a good idea. @icculus?

@icculus
Copy link
Collaborator

icculus commented Dec 21, 2025

Not against it, but I'm real nervous to apply this right before 3.4.0 ships.

@slouken slouken modified the milestones: 3.4.0, 3.4.2 Dec 21, 2025
@slouken
Copy link
Collaborator

slouken commented Dec 21, 2025

I'll bump this to the 3.4.2 milestone so it can get some more testing.

Copy link
Collaborator

@icculus icculus left a comment

Choose a reason for hiding this comment

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

It's totally possible I'm misunderstanding this PR, but it feels like it's cutting way too deep for the thing it's trying to accomplish. I'm not sure we want to merge this after all, but I'm going to come back to this later and re-read it all again before I make any final decisions on it.

const char *selector;

bool fill_document = ((window->flags & SDL_WINDOW_FILL_DOCUMENT) != 0);
if (fill_document && Emscripten_fill_document_window) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the check for multiple fill-document windows removed here?

Copy link
Author

Choose a reason for hiding this comment

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

I might be missing something... I found that Emscripten_SetWindowFillDocument already checks for multiple windows and raises an error.

static bool Emscripten_SetWindowFillDocument(SDL_VideoDevice *_this, SDL_Window *window, bool fill)
{
    SDL_WindowData *wdata = window->internal;
    if (fill && Emscripten_fill_document_window && (Emscripten_fill_document_window != window)) {
        return SDL_SetError("Only one fill-document window allowed at a time.");
    }


Emscripten_UnregisterEventHandlers(data);

// We can't destroy the canvas, so resize it to zero instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to change this behavior.

Copy link
Author

@Grey41 Grey41 Jan 3, 2026

Choose a reason for hiding this comment

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

Fair enough. One thing I didn't understand... why do we change the css size on quit when there's a high pixel ratio?

if (data->pixel_ratio != 1.0f) {
    emscripten_set_element_css_size(data->canvas_id, 1, 1);
}

In my project, I reuse the canvas many times. It was frustrating when my canvas suddenly disappeared, but only on high dpi devices. This means I have to manually revert the css size after SDL quits.

@Grey41
Copy link
Author

Grey41 commented Jan 3, 2026

@icculus You make good points. I was hoping to make the canvas resizing cleaner in the browser. Also, sometimes it might be ideal to preserve the canvas and let the SDL user handle the canvas clearing.

In the recording below:
▶️ calls SDL_Init
⏹️ calls SDL_Quit

Screen.Recording.2026-01-03.at.11.57.20.mov

I'd understand if this goes against good SDL cleanup practice.

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.

3 participants