For #24040 - App should not crash when the search widget is clicked while PIP mode is active #24951

indurs · 2022-04-25T05:45:30Z

When the search event is from the search widget while PIP is active, the search fragment opens after the screen is unlocked. This avoids the issue of the search page opening in landscape mode and also the app doesn't crash.

screenpip.mp4

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

firefoxci-taskcluster · 2022-04-25T05:45:47Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

indurs · 2022-04-27T06:19:21Z

@Mugurell , to avoid the crash when the search widget is tapped and to open the search page in portrait mode, I've modified the StartSearchIntentProcessor and added a method in BrowserStore. The app doesn't crash and the search is opened in portrait mode.
Just wanted to make sure I'm going in the right direction. Suggestions would be helpful.
In this approach, onStop() method of BrowserFragment will be called. So fullScreenFeature.onBackPressed() is called
in onStop() as well in pipmodechanged(). I will explore more if the above approach is fine

Mugurell

This seems like a fine approach to handle the race but I'm a bit worried about the result.

It seems like we will open to the fullscreen video and after a small delay we'll navigate to home to actually enter search mode. Not sure how big the delay would be but it could probably be seen by users as another issue.

Would it be possible to go the other way around, navigate to home to enter search immediately, and handle fullscreen media cleanup in the background?

Mugurell · 2022-04-27T08:40:19Z

app/src/main/java/org/mozilla/fenix/browser/BrowserStore.kt

+    block: () -> Unit
+) {
+
+    if (state.findActiveMediaTab() == null || !state.findActiveMediaTab()?.mediaSessionState?.fullscreen!!) {


Can the fullscreen check avoid the not null assertion (!!) operator for something like

state.findActiveMediaTab()?.mediaSessionState?.fullscreen? == false

(here and below. This could even be another method (extension or not) - isPlayingMediaInFullscreen())

Thank you for reviewing! I'll update the code.

@Mugurell , if we go to homefragment immediately the screen orientation will still be in landscape mode.
When the BrowserFragment receives pipmode as false, FullScreenFeature.onBackPressed() is invoked which triggers the full screen exit event which finally reaches MediaSessionFullscreenFeature. When MediaSessionFullscreenFeature finds out that the fullscreen is false only then the activity orientation is set to portrait. Will explore more..

Maybe the intent processor can also set sensor orientation?
It might be that we need all the functionality to exit fullscreen here.

Sure. We can do it that way

@Mugurell , I've updated the changes. But the screen first goes to the landscape mode and then to the portrait mode, since the activity is in landscape while it's in PIP mode.

Untitled.4.mp4

Maybe, we could just set the activity's orientation to ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED in MediaSessionFullscreenFeature, when the activity is in PIP mode. Is this a good way to solve the problem? If so I'll look deeper into the MediaSessionFullscreenFeature class since it's used by several classes.

private fun processFullscreen(sessionStates: List<SessionState>) { /* there should only be one fullscreen session */ val activeState = sessionStates.firstOrNull() if (activeState == null || activeState.mediaSessionState?.fullscreen != true) { activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER return } when (activeState.mediaSessionState?.elementMetadata?.portrait) { true -> activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER_PORTRAIT false -> if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && activity.isInPictureInPictureMode) { activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED }else{ activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_SENSOR_LANDSCAPE } else -> activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER } }

When we search from search widget

fromsearch.mp4

Also when we enter from PIP (from home)

frompip2.mp4

Mugurell · 2022-04-27T08:52:52Z

app/src/main/java/org/mozilla/fenix/browser/BrowserStore.kt

+import mozilla.components.feature.media.ext.findActiveMediaTab
+import mozilla.components.lib.state.Store
+
+fun BrowserStore.waitForPIPToClose(


Not sure on whether we need this as an extension function - for this method to be added to the BrowserStore namespace and use the lower level Subscription api but looks like it may be useful in other scenarios also and it seems we already do something similarly with BrowserStore.waitForSelectedOrDefaultSearchEngine.
Only please add the mozilla license and documentation and move this rather to fenix/ext where other extensions are.
If you could also add some tests (in the likes of the ones for waitForSelectedOrDefaultSearchEngine would be great!

Yes.. Actually after going through waitForSelectedOrDefaultSearchEngine in BrowserStore, I thought that we can implement similar solution to avoid the race. Will move extension to fenix/ext and also add tests.

Mugurell · 2022-04-27T08:53:22Z

app/src/main/java/org/mozilla/fenix/browser/BrowserStore.kt

+        }
+    }
+    subscription.resume()
+}


nit: missing newline.

Mugurell · 2022-04-27T09:31:33Z

app/src/main/java/org/mozilla/fenix/home/intent/StartSearchIntentProcessor.kt

            }
+            return processed


Would this return false if fullscreen media is currently playing and the block of waitForPIPToClose wasn't yet executed?
Seeing that in the current implementation true is returned if event != null and we're already in this case it seems to be okay to return here true without the need of the new processed property.
We return that this processor will act upon the Intent, just that we now introduce a small delay.

Would this return false if fullscreen media is currently playing and the block of waitForPIPToClose wasn't yet executed?
-- No. It should not return false

Seeing that in the current implementation true is returned if event != null and we're already in this case it seems to be okay to return here true without the need of the new processed property.
We return that this processor will act upon the Intent, just that we now introduce a small delay.
-- I understand now that we can return true

val event = intent.extras?.getString(HomeActivity.OPEN_TO_SEARCH) return if (event != null) { store.waitForPIPToClose { openSearch(event,navController,out) } return true } else { false }

mergify · 2022-05-03T09:48:27Z

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

indurs · 2022-05-04T06:22:44Z

@Mugurell , setting the orientation as ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED in MediaSessionFullscreenFeature when the activity is in PIP mode, might solve the orientation issue. Full screen media cleanup will be handled in the onStop() method of BaseBrowserFragment. In this scenario we don't have to change the orientation in StartSearchIntentProcessor.
Please see the attached videos above.

Mugurell · 2022-05-05T12:09:22Z

@indurs Videos look great!
I like your suggestion about setting SCREEN_ORIENTATION_UNSPECIFIED when we are in PIP.
When we exit pip to re-enter the app we should exit fullscreen either way to there's no need to still be in landscape as "fullscreen media playing" could require.
So if we need that android-components change to get that UX shown in the above videos I think we should do it. (hoping there's no race there also 🤔)
(All work should be done based on a ticket defining the problem to be resolved, you can open a ticket on AC for this or we help also with this.)
Nice work!

indurs · 2022-05-06T06:09:51Z

@Mugurell , Thank you !
For this issue I'll just update the BaseBrowserFragment and commit.

private fun pipModeChanged(session: SessionState) {
       if (!session.content.pictureInPictureEnabled && session.content.fullScreen && isAdded) {
            onBackPressed()
            fullScreenChanged(false)
        }
    }

I think there are several ways a user can navigate away from PIP mode. So far I've looked into search widget, share from focus to fenix, share from fenix to focus, home shortcut... and more
To tackle the orientation issue when the user navigates away while in pip mode ,the code changes will involve MediaSessionFullscreenFeature and probably involve other classes as well. As you've mentioned we need to take care of race conditions as well.
mozilla-mobile/focus-android#6862 (comment) is also related
Could you please create a PR on AC and assign it to me? Thank You

Mugurell · 2022-05-06T12:26:22Z

@indurs I've created mozilla-mobile/android-components#12118 for this.
You have to leave a comment there to then be able to have the ticket assigned to you.

indurs · 2022-05-13T05:01:25Z

@Mugurell, this PR is just for the change made to avoid the crash when the search widget is clicked.

Mugurell

@indurs There are some CI tasks failing related to code style, please check.
Also please squash the commits.
Would be great to add some tests to ensure the BrowserStore.waitForPIPToClose functionality will not regress in the future.

Tested this for myself now and I see an important issue remaining: no crash but although the device was always kept in portrait upon clicking the search widget while in PIP the app and the search UX will start and remain blocked in landscape.

SearchWidgetOpensFenixInLandscape.mp4

If a more complex solution with support from AC is needed maybe for now just catching the IllegalStateException would be a quick fix to prevent the crash.

indurs · 2022-05-13T12:55:11Z

@Mugurell, this PR is only for avoiding the crash when the search widget is tapped while in PIP mode. So the orientation will be locked to landscape as before.
Since we've opened an issue in AC for orientation mozilla-mobile/android-components#12118 I thought that I've to create a different PR addressing that issue in AC. I've implemented the changes for orientation but not yet committed the changes in AC.

Mugurell · 2022-05-13T13:00:01Z

@Mugurell, this PR is only for avoiding the crash when the search widget is tapped while in PIP mode. So the orientation will be locked to landscape as before. Since we've opened an issue in AC for orientation mozilla-mobile/android-components#12118 I thought that I've to create a different PR addressing that issue in AC. I've implemented the changes for orientation but not yet committed the changes in AC.

Having the device locked to landscape means users would have a hard time going back to portrait.
Having them locked in landscape and forcing this way of interacting with the browser while the device is in portrait seems like a still broken experience, that's why I suggested just catching the exception as a quick and easy crash fix while the work for ensuring the app will behave nicely can continue separately.

indurs · 2022-05-13T13:12:22Z

Got it. Will just catch the IllegalStateException and commit.

indurs · 2022-05-16T08:56:15Z

@Mugurell , after updating the MediaSessionFullscreenFeature for fixing the orientation issue, I'm not using the BrowserStore.waitForPIPToClose functionality. Shall I delete that extension from BrowserStore?

Mugurell · 2022-05-16T10:55:25Z

@Mugurell , after updating the MediaSessionFullscreenFeature for fixing the orientation issue, I'm not using the BrowserStore.waitForPIPToClose functionality. Shall I delete that extension from BrowserStore?

Agree, thank you!
Generally we don't keep unused code around.

So I understand you plan to base this fix on mozilla-mobile/android-components#12169 ?
(The result looks great 👍)
If so that will have to be merged first and then we can land this also (commits needs to be squashed).

indurs · 2022-05-16T11:11:47Z

Thank You! Yes. We've to merge the MediaSessionFullscreenFeature changes in AC first and then this fix in Fenix. I'll squash the changes.

indurs · 2022-05-24T12:42:06Z

@Mugurell, when I squash the commits, I get the error "Unable to reorder. Reordering replays all commits up to the last one required for the reorder. A merge commit cannot exist among those commits." Few days back I updated the branch couple of times by selecting "update with merge commit". Could you please let me know how to fix this issue?

Mugurell · 2022-05-24T13:24:34Z

All solutions

@Mugurell, when I squash the commits, I get the error "Unable to reorder. Reordering replays all commits up to the last one required for the reorder. A merge commit cannot exist among those commits." Few days back I updated the branch couple of times by selecting "update with merge commit". Could you please let me know how to fix this issue?

Sorry but I don't know of any simple solution.
Tried to quickly fixup all commits but I still get conflicts to resolve which may not worth the hassle since the changes are small in the first place.
I would probably start fresh on a new branch and copy the changes from https://github.com/mozilla-mobile/fenix/pull/24951/files then force push to this branch with something like git push origin <new_branch>:issuefix24040 -f.

While making small changes to existing commits I do prefer comiting with --amend or creating new fixup commits later to be rebased to ensure a clean, linear history and avoid merges in favor of rebasing.

indurs · 2022-05-24T13:48:59Z

All solutions

@Mugurell, when I squash the commits, I get the error "Unable to reorder. Reordering replays all commits up to the last one required for the reorder. A merge commit cannot exist among those commits." Few days back I updated the branch couple of times by selecting "update with merge commit". Could you please let me know how to fix this issue?

Sorry but I don't know of any simple solution. Tried to quickly fixup all commits but I still get conflicts to resolve which may not worth the hassle since the changes are small in the first place. I would probably start fresh on a new branch and copy the changes from https://github.com/mozilla-mobile/fenix/pull/24951/files then force push to this branch with something like git push origin <new_branch>:issuefix24040 -f.

Thank You. Will try that approach.

While making small changes to existing commits I do prefer comiting with --amend or creating new fixup commits later to be rebased to ensure a clean, linear history and avoid merges in favor of rebasing.

Sure. I'll follow for future commits.

indurs · 2022-05-25T05:57:09Z

All solutions
I would probably start fresh on a new branch and copy the changes from https://github.com/mozilla-mobile/fenix/pull/24951/files then force push to this branch with something like git push origin <new_branch>:issuefix24040 -f.

@Mugurell , as you suggested I tried the above method and there is now only one commit. I think it should be fine now. Please let me know if I need to make any changes. Thank You

Mugurell

Tested locally and it works great!
Just a few questions about some decisions in code.

Mugurell · 2022-05-26T12:41:33Z

app/src/main/java/org/mozilla/fenix/HomeActivity.kt

@@ -503,6 +504,13 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
            )
        )

+        if (components.core.store.state.findActiveMediaTab() != null) {
+            lifecycleScope.launch(IO) {


Not sure this is really needed. Have you observed an adverse effect of running this without the coroutine?
It's just that we already filter tabs in many (like it happens with findActiveMediaTab() on the Main thread without a known impact and there is also an inherent cost for the context switching here so I'm not sure of the benefits.

I didn't observe any improvement using the coroutine. You're right regarding the performance penalty with context-switching. I'll remove the coroutine.

Mugurell · 2022-05-26T12:52:19Z

app/src/main/java/org/mozilla/fenix/home/intent/StartSearchIntentProcessor.kt

-                    MetricsUtils.Source.SHORTCUT
-                }
-                else -> null
+            return openSearch(event, navController, out)


Comparing before and after it seems to me that openSearch contains the exact same code as before just extracted now in a new method?
If so this change probably won't be needed with preserving git history being more important.

The structure of the code changed when I modified StartSearchIntentProcessor. I removed the changes but I left the modified structure. I'll revert back to the original code structure. Thank you for pointing that. I'll keep that in mind when I do future changes.

… is clicked while PIP mode is active When the search event is from the search widget while PIP is active, the search fragment opens after the screen is unlocked. This avoids the issue of the search page opening in landscape mode and also the app doesn't crash. Co-Authored-By: Mugurell <[email protected]>

indurs · 2022-05-27T06:39:14Z

@Mugurell , I've committed the changes.

Mugurell

LGTM.
Thank you for all the work on this!

indurs · 2022-05-27T10:39:43Z

Thank You @Mugurell !

Mugurell · 2022-05-27T13:44:45Z

Patch merged in #25410.

indurs mentioned this pull request Apr 25, 2022

Crash when tapping the search widget while in PIP #24040

Closed

indurs changed the title ~~For #24040 - Open search page in portrait mode~~ For #24040 - App should not crash when search widget is clicked Apr 25, 2022

indurs changed the title ~~For #24040 - App should not crash when search widget is clicked~~ For #24040 - App should not crash when the search widget is clicked while PIP mode is active Apr 25, 2022

Mugurell reviewed Apr 27, 2022

View reviewed changes

Mugurell mentioned this pull request May 6, 2022

Set default screen orientation if playing media in pip mozilla-mobile/android-components#12118

Closed

indurs marked this pull request as ready for review May 13, 2022 05:01

indurs requested review from a team as code owners May 13, 2022 05:01

Mugurell suggested changes May 13, 2022

View reviewed changes

Mugurell mentioned this pull request May 17, 2022

For #12118 : Set default screen orientation if playing media in pip mozilla-mobile/android-components#12169

Closed

4 tasks

indurs force-pushed the issuefix24040 branch from d08bab2 to 304af9e Compare May 25, 2022 05:39

Mugurell reviewed May 26, 2022

View reviewed changes

indurs force-pushed the issuefix24040 branch from 304af9e to 830db16 Compare May 27, 2022 06:26

Mugurell approved these changes May 27, 2022

View reviewed changes

Mugurell closed this May 27, 2022

indurs deleted the issuefix24040 branch May 28, 2022 00:37

For #24040 - App should not crash when the search widget is clicked while PIP mode is active #24951

For #24040 - App should not crash when the search widget is clicked while PIP mode is active #24951

Conversation

indurs commented Apr 25, 2022 • edited Loading

Pull Request checklist

To download an APK when reviewing a PR:

firefoxci-taskcluster bot commented Apr 25, 2022

indurs commented Apr 27, 2022

Mugurell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indurs Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 3, 2022

indurs commented May 4, 2022 • edited Loading

Mugurell commented May 5, 2022 • edited Loading

indurs commented May 6, 2022 • edited Loading

Mugurell commented May 6, 2022

indurs commented May 13, 2022

Mugurell left a comment

Choose a reason for hiding this comment

indurs commented May 13, 2022 • edited by Mugurell Loading

Mugurell commented May 13, 2022 • edited Loading

indurs commented May 13, 2022 • edited Loading

indurs commented May 16, 2022

Mugurell commented May 16, 2022

indurs commented May 16, 2022

indurs commented May 24, 2022

Mugurell commented May 24, 2022

indurs commented May 24, 2022

indurs commented May 25, 2022

Mugurell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indurs commented May 27, 2022

Mugurell left a comment

Choose a reason for hiding this comment

indurs commented May 27, 2022

Mugurell commented May 27, 2022

indurs commented Apr 25, 2022 •

edited

Loading

indurs Apr 28, 2022 •

edited

Loading

indurs commented May 4, 2022 •

edited

Loading

Mugurell commented May 5, 2022 •

edited

Loading

indurs commented May 6, 2022 •

edited

Loading

indurs commented May 13, 2022 •

edited by Mugurell

Loading

Mugurell commented May 13, 2022 •

edited

Loading

indurs commented May 13, 2022 •

edited

Loading