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. 😄

[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:

• servo/ports/servo/glutin_app/window.rs

  Line 326 in cc131be

  window.set_fullscreen(None);

• servo/components/script/dom/document.rs

  Line 3228 in cc131be

  let event = EmbedderMsg::SetFullscreenState(true);

[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.