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

Add web fullscreen support #1142

Merged
merged 18 commits into from
Oct 11, 2019

Conversation

ryanisaacg
Copy link
Contributor

@ryanisaacg ryanisaacg commented Sep 4, 2019

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

PR progress:

  • Request fullscreen makes a call to the web APIs
  • Fullscreen requests are aware of security requirements that fullscreen can only happen in specific event handlers
  • The canvas is resized when it enters and exits fullscreen, and resize events are sent to the user

This makes progress on #1072

Currently there is an issue that the event is not processed within a user handler, so the browser rejects the attempt to fullscreen.

Supporting fullscreen requires both the business logic (how to
determine which canvas should be fullscreen) and the backend support
(abstracting over the web methods that handle fullscreen.) This
fulfills the latter.
@ryanisaacg ryanisaacg mentioned this pull request Sep 7, 2019
10 tasks
@TannerRogalsky
Copy link

I don't think this should block this PR or feature but it's maybe worth noting that the fullscreen API works on iOS Safari on iPads only. The iPhone has no fullscreen API.

let width = window.inner_width().expect("Failed to get width").as_f64().expect("Failed to get width as f64");
let height = window.inner_height().expect("Failed to get height").as_f64().expect("Failed to get height as f64");

LogicalSize { width, height }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a backend static method: web_sys::window_size().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good point

@@ -163,5 +162,23 @@ impl<T> WindowTarget<T> {
},
});
});

let runner = self.runner.clone();
let event_canvas = canvas.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid cloning the whole canvas if we create the intended_size here and move it into the closure? We could keep there the size before entering fullscreen by reading the size of the raw canvas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need to keep track of intended_size as it changes, because it can be changed by the user at any point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of ignoring user resizes when fullscreen is enabled. Does that break something? Or would it be unintuitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works for when fullscreen is enabled but the problem comes in when you have something like:

create window 1 at size of 800x600
set window 1's size to 640x480
fullscreen window 1
un-fullscreen window 1

You probably want it to return to 640x480, which means the intended size can change during the lifetime of the program.

Copy link
Contributor

@hecrj hecrj Oct 3, 2019

Choose a reason for hiding this comment

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

Couldn't we clone raw instead and use it to read the intended_size (with the width and height methods), store it while fullscreen mode is enabled, and restore the correct size when disabled? We could wrap the raw canvas in a new type if necessary. It's just that implementing Clone for the current Canvas type feels a bit hacky because of all the event listeners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, I used that design and was able to remove the Clone impl from Canvas.

*wants_fullscreen.borrow_mut() = false;
}
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is an API limitation, but it will be a bit confusing if users are buffering events and processing them in batches. If a fullscreen request happens as a result of this event processing, it will not be applied until the user interacts again, right? This is a bit unfortunate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it is an unfortunate fact of the web fullscreen APIs. I'm not sure of a better way to work around it.

@ryanisaacg ryanisaacg marked this pull request as ready for review October 10, 2019 19:42
Copy link
Contributor

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Looks great! Nice work! 🥂

Just noticed a tiny thing that I think we should fix before merging.

intended_size
};
raw.set_width(new_size.width as u32);
raw.set_width(new_size.height as u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, the second one should be set_height I think.

@ryanisaacg ryanisaacg merged commit 3ff4834 into rust-windowing:web Oct 11, 2019
@ryanisaacg ryanisaacg deleted the add-web-fullscreen-support branch October 11, 2019 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants