Skip to content

Conversation

@Kassy2048
Copy link
Contributor

@Kassy2048 Kassy2048 commented Jun 29, 2023

Fullscreen/pointerlock can be requested during transient activation not just while handling an input event, so check for that on browsers that support it (all but Firefox it seems).

These changes make it possible for the fullscreen switch to happen instantaneously on apps that treat the input events outside the event handler (like SDL2 for instance).

Fullscreen can be requested during transient activation
not just while handling an input event, so check for that
on browsers that support it.
@Kassy2048 Kassy2048 changed the title Use dedicated API to know if fullscreen can be requested Use dedicated API to know if fullscreen/pointerlock can be requested Jun 30, 2023
@sbc100
Copy link
Collaborator

sbc100 commented Jun 30, 2023

@juj WDYT?

I'm not really an html5 expert but it seems reasonable. Also seems hard to write a test for :(

Comment on lines 115 to 118
if (navigator.userActivation) {
// Use transient activation status for browsers that support it
return navigator.userActivation.isActive;
}
Copy link
Collaborator

@juj juj Jun 30, 2023

Choose a reason for hiding this comment

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

How about

  canPerformEventHandlerRequests: function() {
  // Verify against transient activation status from UserActivation API
  // whether it is possible to perform a request here without needing to defer. See
  // https://developer.mozilla.org/en-US/docs/Web/Security/User_activation#transient_activation
  // and https://caniuse.com/mdn-api_useractivation
  // At the time of writing, Firefox does not support this API: https://bugzilla.mozilla.org/show_bug.cgi?id=1791079
  return navigator.userActivation?.isActive
    || (JSEvents.inEventHandler && JSEvents.currentEventHandler.allowsDeferredCalls);
}

It would be interesting to experiment with a micro code-size improvement to remove JSEvents.inEventHandler and JSEvents.currentEventHandler.allowsDeferredCalls altogether when targeting only browsers that are guaranteed to support UserActivation API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't generally allow optional chaining in the generated JS yet, but perhaps we should. If we can show that closure that lower it to ES5 as needed that would be great... let me see if I can confirm that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the comment lines. Let me know when you've decided about the optional chaining operator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can land this now and consider it as part of the motivation for landing #19772

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure I understand. Do you mean using the optional chaining in the PR and wait for #19772 completion to merge this PR? Or the opposite (not using the optional chaining here)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that I think we should land this PR now, as is

@juj
Copy link
Collaborator

juj commented Jun 30, 2023

LGTM modulo that comment above.

Maybe this will further enable a nice simplification of otherwise hairy logic in the HTML5 API by enabling us to tear down the previous manual check logic.

@sbc100 sbc100 enabled auto-merge (squash) July 7, 2023 23:57
@sbc100 sbc100 merged commit a6b8143 into emscripten-core:main Jul 9, 2023
@Kassy2048 Kassy2048 deleted the fix-fullscreen branch July 9, 2023 19:09
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