Servo: Make Window::set_fullscreen pass a non-None value when entering fullscreen

[jdm]

I expect we can call window.get_primary_monitor() to get a monitor id, then pass that in a Some value when state is true. This will hopefully make Servo _actually_ enter full screen mode.

It looks like we will also need to change the use of SetFullscreenState in exit_fullscreen in document.rs to pass the value false, or we will never be able to exit fullscreen mode.

https://www.joshmatthews.net/webgl2.html should be able to verify if these changes are behaving as expected.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. :smile:

[kamal-umudlu]

@highfive assign me. I am new on this project. Would like to start with this issue

[highfive]

Hey @kamal-umudlu! Thanks for your interest in working on this issue. It's now assigned to you!

[kamal-umudlu]

Hi,
Currently I am working on this bug. Please correct me if I am wrong. After my fix, fullscreen-region-content-001.html should go to the full screen and exit from full screen mode by pressing esc. Is that all I need to fix for this bug?

[kamal-umudlu]

I tried to pass get_primary_monitor() in Some value under get_allow_fullscreen() but it looks like there is no window.get_primary_monitor() function in windows.rs file.

I am new in this project maybe I am wrong. Could you please give a little clarification about issue?
Thanks )

[jdm]

Sorry, I should have been more specific about the code I was looking at:

• https://github.com/servo/servo/blob/cc131be2e0f48924ffc8cdfc017fc23c361f1140/ports/servo/glutin_app/window.rs#L326
• https://github.com/servo/servo/blob/cc131be2e0f48924ffc8cdfc017fc23c361f1140/components/script/dom/document.rs#L3228

[kamal-umudlu]

Hi Josh,
So I have changed code to window.set_fullscreen(Some(window.get_primary_monitor())); and build the project to see my changes in fullscreen-region-content-001.html. After my change, when I press "Go full screen" button the whole window (not only video part) goes to full screen mode and I can't exit from fullscreen mode by pressing "ESC" button.

Is there any way to make only video to become full screen or am I running wrong html file?

[jdm]

Our existing fullscreen support (inside of a window, rather than actually making the window fullscreen) is supposed to do what you describe. Have you tried https://www.joshmatthews.net/webgl2.html?

[jdm]

As for leaving fullscreen, I would expect fixing the code in document.rs to avoid that problem.

[kamal-umudlu]

Yeah I understood fullscreen support and also tried https://www.joshmatthews.net/webgl2.html. I was expecting the same behaviour from fullscreen-region-content-001.html but maybe I am missing something that doesn't make video element to become full size.

My expectation was after passing window.set_fullscreen(Some(window.get_primary_monitor())), it will get video element and make it full screen which doesn't.

[jdm]

That may be related to #22358. We have observed fullscreen support not working for videos in particular.

[kamal-umudlu]

Under #22358 , it says issue may be relevant to this issue. That is why I thought maybe my fix also needs to fix #22358 . Am I wrong?

[jdm]

I wouldn't expect the changes for this issue to affect #22358.

[kamal-umudlu]

Oh sure. Is there any other fix I need to do other than passing monitor_id and changing SetFullscreenState to true in exit_fullscreen?

[jdm]

If those make the window enter fullscreen mode and leave it, then no :)

[kamal-umudlu]

The only issue I have right now is when I press "ESC" keyboard button, it close the whole servo app (doesn't make difference if it is in fullscreen mode or not). Is it also issue that I need to fix? If yes, is there any way to map "ESC" button to exit_fullscreen() function?

[jdm]

Ooh, that's a very good point. So, the decision to quit Servo happens in handle_key_from_window in ports/servo/browser.rs. This is called before web content is allowed to handle it; I think we should add a check for whether self.window is currently fullscreen and send a message to the constellation to exit fullscreen (which will get passed to the appropriate script thread and invoke document.exit_fullscreen()). Would you like to work on that?

[kamal-umudlu]

Sure I can try :)

But the only thing I just need to make sure is it is feasible to fix it until Friday noon.
I just need the steps that I can follow and I also have couple questions.

1. If the window is not in fullscreen mode, should it quit from the app when "ESC" button is pressed?

2. How I can send a message to the constellation? Do I have to add a script like WindowEvent::ExitFullScreen in servo/components/servo/lib.rs file?

3. Where I can call document.exit_fullscreen() that will actually exit from fullscreen mode?

[jdm]

1. Yes.

2&3.
You will need to add a new WindowEvent value, then handle it in handle_window_event in components/servo/lib.rs by sending a new ConstellationMsg value like WindowEvent::LoadUrl does. This new value will be handled by handle_request_from_compositor in components/constellation/constellation.rs, and it will send a new ConstellationControlMsg value to the script thread. You can use FromCompositorMsg::WindowSize as a model; it takes the toplevel browsing context id, gets a pipeline from it (in resize_browsing_context) and sends a ConstellationControlMsg to it that tells the script thread that a resize has happened. The new message for exiting fullscreen will need to be handled in start in components/script/script_thread.rs, and you can use the Resize message as a guide again. That will allow you to invoke the exit_fullscreen method on an actual document :D

[kamal-umudlu]

Thanks :)

I will do my best and will let you know if I have any question.

[kamal-umudlu]

Is there any public getter function to check whether self.window is in fullscreen mode or not?

[jdm]

Actually it looks like you can check self.fullscreen instead.

[kamal-umudlu]

oh I thought it is a private value, if it is public value then it is fine :)

[kamal-umudlu]

Update

I have tried to use self.window.fulscreen, but it seems like it is private value. So, I can't actually use self.window.fullscreen :(

I am actually so close to the fix, but don't know If I should add getter function for fullscreen value.

[jdm]

Like I said, self.fullscreen instead of self.window.

[kamal-umudlu]

So I was saying in browser.rs file, is there any way to access fullscreen state? I have tried self.fullscreen too but it says there is no field. Is there way to get state of fullscreen in browser.rs?

[kamal-umudlu]

.shortcut(Modifiers::empty(), Key::Escape, || { let state = **self.fullscreen**; if state { if let Some(id) = self.browser_id { let event = WindowEvent::ExitFullScreen(id); self.event_queue.push(event); } } else { self.event_queue.push(WindowEvent::Quit); } })

Here is my code in browser.rs

[jdm]

Oh, I'm sorry. I kept mixing up which file we were talking about. Yes, go ahead and add a public getter to Window that returns self.fullscreen.

[kamal-umudlu]

I added getter function and now I can get self.fulscreen value. Right now I don't have any compile error and I have tested my code on fullscreen-region-content-002.html. But it is not actually changing fullscreen mode when I pressed ESC button.

So my guess is, I am missing something in script_thread.rs file.

This is the code I added to script_thread.rs file. Am I missing something here?

fn handle_exit_fullscreen(&self, id: PipelineId) {
        let document = self.documents.borrow().find_document(id);
        if let Some(document) = document {
            document.exit_fullscreen();
            return;
        }
    }

[jdm]

That seems reasonable. Have you tried using a debugger or adding println! statements to make sure the new code is being executed?

[kamal-umudlu]

I am trying now. Just one quick question. After adding println statements, do I have to build project again?

[jdm]

Yes.

[kamal-umudlu]

So yeah I added println statements and seems like the new code is being executed. But in document.exit_fullscreen() function, after executing let promise = Promise::new(global.r()); it is crashing.

Is there any file or function that actually calls document.exit_fullscreen()?

[jdm]

When I search the code (using grep or ack, for example) I see unbind_from_tree in element.rs, which is executed when an element is removed from a document (like with node.remove() in JS). I suspect that adding this code before calling exit_fullscreen will make the crash go away:

let _ac = JSAutoCompartment::new(document.global().get_cx(), document.reflector().get_jsobject().get());

[kamal-umudlu]

Can I just add that code on my handler like this?

fn handle_exit_fullscreen(&self, id: PipelineId) {
        let document = self.documents.borrow().find_document(id);
        if let Some(document) = document {
            let _ac = JSAutoCompartment::new(document.global().get_cx(), document.reflector().get_jsobject().get());
            document.exit_fullscreen();
            return;
        }
    }

[kamal-umudlu]

I tested it. I think it worked :)

[kamal-umudlu]

Hi Josh. I have one more last question :)

Should I create two different Pull Requests for one fixing issue #22853 and the second for adding ESC button that make Servo exit full screen mode? Or should i just do one PR that covers both patches?

[jdm]

One PR for both commits is fine.