search

mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.25k stars 2.94k forks source link

Handle fullscreen in WebEngine #18347

Open data-sync-user opened 10 months ago

data-sync-user commented 10 months ago

┆Issue is synchronized with this Jira Task

data-sync-user commented 8 months ago

➤ Laurie Marceau commented:

Turns out this isn’t just a WebEngine task, as there exists a call to exitFullScreenVideo in Focus iOS. So we need an a public API call to so this on the WebEngine session.

data-sync-user commented 8 months ago

➤ Matthew Reagan commented:

Laurie MarceauA couple notes:

{quote}* How does the implementation differs from Firefox to Focus?{quote}

  1. Specific callsites and how the scripts are injected differ between Firefox and Focus (added a couple references to Jira description), enough that consolidating the JS injection code completely will probably be a nontrivial task (though I think we can find a shared solution just for Fullscreen)
  2. Focus -- as part of applicationWillResignActive -- calls the closeFullScreen JS func which is unique to the Focus fullscreen script (but is the only difference between the scripts)

{quote}* Can we align on how this is handled? If not, maybe we can use the nicer/most recent script of the two for both products.{quote}

I’m not entirely clear why Firefox doesn’t have a similar closeFullScreen and at least initially it seems like we could consolidate the two by sharing an identical script (using the Focus script and adding the hook to call closeFullScreen in Firefox, if needed, or just skipping that for parity with the existing client).

{quote}Turns out this isn’t just a WebEngine task, as there exists a call to exitFullScreenVideo in Focus iOS. So we need an a public API call to so this on the WebEngine session.{quote}

I might be wrong or misunderstanding but I don’t think that this needs to be an explicit API on WebEngine; the clients know about the closeFullScreen() func and can just call something equivalent to evaluateJavaScript("closeFullScreen()") , which is essentially what Focus does currently, as long as we expose an API for executing JS on the EngineSession (if we haven’t already). This might be something that is worth syncing up on however, I don’t have full context on how you had intended this to work. LMK if you want to chat further.

data-sync-user commented 8 months ago

➤ Matthew Reagan commented:

Laurie Marceau Did some more digging and the closeFullScreen func (which is the only thing unique/specific to Focus' fullscreen JS vs. Firefox) was added here: https://github.com/mozilla-mobile/focus-ios/pull/3679/files ( https://github.com/mozilla-mobile/focus-ios/pull/3679/files|smart-link )

as part of this ticket: https://mozilla-hub.atlassian.net/browse/FOCUSIOS-44 ( https://mozilla-hub.atlassian.net/browse/FOCUSIOS-44|smart-link )

and also a related PR: https://github.com/mozilla-mobile/focus-ios/pull/1187 ( https://github.com/mozilla-mobile/focus-ios/pull/1187|smart-link )

It looks like that last PR (which was added to fix https://github.com/mozilla-mobile/focus-ios/issues/1139 ( https://github.com/mozilla-mobile/focus-ios/issues/1139|smart-link ) ) is ultimately what led to that addition to the Fullscreen.js.

It might be useful for us to verify whether that same bug (https://github.com/mozilla-mobile/focus-ios/issues/1139 ( https://github.com/mozilla-mobile/focus-ios/issues/1139|smart-link ) ) also repro’s in Firefox, but either way it seems like we should be able to include that functionality into Firefox if/when we consolidate the implementation between both clients.

data-sync-user commented 8 months ago

➤ Laurie Marceau commented:

Matthew Reagan Awesome! Then yeah sounds like we want this to be ported to Firefox indeed. Thank you!