Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For # #11374 - Restore immersive mode #11375

Merged
merged 2 commits into from
Jan 6, 2022

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Dec 8, 2021

This was an issue not reproducing on all devices.
Added a new insets listener to know when they change and set again immersive mode for the fullscreen Activity. (same as Chrome uses)
Handle focus changes internally to guarantee that these events will also restore immersive mode whereas previously clients could pass any focus listener even one not knowing to restore the immersive mode.

Before
StatusBarShowingForFullscreenVideo.mp4
After
StatusBarHidingForFullscreenVideo.mp4

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

if (insets.isVisible(statusBars())) {
setAsImmersive()
}
insets
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we need insets here? As it doesn't looks as we are using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -11,6 +11,9 @@ permalink: /changelog/
* [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
* [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)

* **support-ktx**
* 🚒 Bug fixed [issue #11374](https://github.com/mozilla-mobile/android-components/issues/11374) - Restore immersive mode after interacting with other Windows. Handle this internally without clients having to pass a OnWindowFocusChangeListener.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we should mention that we have a breaking change on enterToImmersiveMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Will update!

Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

I added a couple of comments apart from them, the PR looks good!

@Mugurell Mugurell force-pushed the restoreImmersiveMode branch from f771c84 to 563780f Compare January 6, 2022 07:57
…stener

Previously we'd allow a listener being passed in and we'd hope it will
correctly handle restoring immersive mode.
Now we are handling focus changes and restoring immersive mode on ourselves
also allowing for a simpler public API.
@Mugurell Mugurell force-pushed the restoreImmersiveMode branch from 563780f to 6a54655 Compare January 6, 2022 08:12
@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Jan 6, 2022
@mergify mergify bot merged commit 3872430 into mozilla-mobile:main Jan 6, 2022
@Mugurell Mugurell deleted the restoreImmersiveMode branch January 6, 2022 10:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants