-
Notifications
You must be signed in to change notification settings - Fork 2k
[Android] MediaPicker: Fix photo picker completion from child activities #35944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
59ad5ed
4ddfbdb
e793d05
f890954
339a20c
ffe6112
479823c
ab60f88
c48d72e
d5a1ae0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| using Microsoft.Maui.Media; | ||
|
|
||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 35826, "PickPhotosAsync hangs when called from a child activity", PlatformAffected.Android)] | ||
| public class Issue35826 : ContentPage | ||
| { | ||
| public Issue35826() | ||
| { | ||
| var instructions = new Label | ||
| { | ||
| AutomationId = "InstructionsLabel", | ||
| Text = "1. Tap 'Open Child Activity'\n" + | ||
| "2. In the child activity, tap 'Pick Photos'\n" + | ||
| "3. Press Back to cancel the picker\n" + | ||
| "Expected: Status shows 'Cancelled'\n" + | ||
| "Bug: Picker hangs indefinitely", | ||
| FontSize = 14 | ||
| }; | ||
|
|
||
| var openButton = new Button | ||
| { | ||
| AutomationId = "OpenChildActivityButton", | ||
| Text = "Open Child Activity", | ||
| HorizontalOptions = LayoutOptions.Fill | ||
| }; | ||
| openButton.Clicked += OnOpenChildActivityClicked; | ||
|
|
||
| var statusLabel = new Label | ||
| { | ||
| AutomationId = "StatusLabel", | ||
| Text = "Status: Ready", | ||
| FontSize = 16, | ||
| FontAttributes = FontAttributes.Bold | ||
| }; | ||
|
|
||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = 30, | ||
| Spacing = 25, | ||
| Children = { instructions, openButton, statusLabel } | ||
| }; | ||
| } | ||
|
|
||
| void OnOpenChildActivityClicked(object sender, EventArgs e) | ||
| { | ||
| #if ANDROID | ||
| var activity = Microsoft.Maui.ApplicationModel.Platform.CurrentActivity; | ||
| if (activity != null) | ||
| { | ||
| var intent = new Android.Content.Intent(activity, typeof(Issue35826ChildActivity)); | ||
| activity.StartActivity(intent); | ||
| } | ||
| #endif | ||
| } | ||
| } | ||
|
|
||
| #if ANDROID | ||
| // A plain AppCompatActivity that calls MediaPicker.PickPhotosAsync(). | ||
| // Before the fix, Platform.Init() on this activity was silently ignored by the guard in | ||
| // ActivityForResultRequest.Register(), so no launcher was registered for it and the | ||
| // picker task never completed. After the fix each activity gets its own launcher entry | ||
| // in the ConditionalWeakTable, so the result is delivered correctly. | ||
| [Android.App.Activity(Label = "Issue35826 Child Activity", Theme = "@style/Maui.SplashTheme")] | ||
| public class Issue35826ChildActivity : AndroidX.AppCompat.App.AppCompatActivity | ||
| { | ||
| Android.Widget.TextView _resultLabel; | ||
|
|
||
| protected override void OnCreate(Android.OS.Bundle savedInstanceState) | ||
| { | ||
| base.OnCreate(savedInstanceState); | ||
|
|
||
| Microsoft.Maui.ApplicationModel.Platform.Init(this, savedInstanceState); | ||
|
|
||
| var layout = new Android.Widget.LinearLayout(this) | ||
| { | ||
| Orientation = Android.Widget.Orientation.Vertical | ||
| }; | ||
| layout.SetPadding(50, 50, 50, 50); | ||
|
|
||
| _resultLabel = new Android.Widget.TextView(this) | ||
| { | ||
| Text = "Result: Ready" | ||
| }; | ||
| _resultLabel.SetPadding(0, 0, 0, 50); | ||
| SetViewIdResourceName(_resultLabel, "ChildActivityResultLabel"); | ||
|
|
||
| var pickButton = new Android.Widget.Button(this) | ||
| { | ||
| Text = "Pick Photos" | ||
| }; | ||
| SetViewIdResourceName(pickButton, "ChildActivityPickButton"); | ||
|
|
||
| pickButton.Click += async (_, _) => | ||
| { | ||
| _resultLabel.Text = "Result: Picking..."; | ||
| try | ||
| { | ||
| var result = await MediaPicker.PickPhotosAsync(); | ||
| _resultLabel.Text = result?.Count > 0 | ||
| ? $"Result: Got {result.Count} photo(s)" | ||
| : "Result: Cancelled"; | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| _resultLabel.Text = "Result: Cancelled"; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _resultLabel.Text = $"Result: Error - {ex.Message}"; | ||
| } | ||
| }; | ||
|
|
||
| layout.AddView(_resultLabel); | ||
| layout.AddView(pickButton); | ||
| SetContentView(layout); | ||
| } | ||
|
|
||
| // Sets ViewIdResourceName on a native Android view so Appium can locate it by | ||
| // resource-id (the same mechanism MAUI uses for AutomationId on Android). | ||
| void SetViewIdResourceName(Android.Views.View view, string automationId) | ||
| { | ||
| var resourceName = $"{PackageName}:id/{automationId}"; | ||
| AndroidX.Core.View.ViewCompat.SetAccessibilityDelegate(view, new AutomationIdDelegate(resourceName)); | ||
| } | ||
|
|
||
| class AutomationIdDelegate : AndroidX.Core.View.AccessibilityDelegateCompat | ||
| { | ||
| readonly string _resourceName; | ||
|
|
||
| public AutomationIdDelegate(string resourceName) => _resourceName = resourceName; | ||
|
|
||
| public override void OnInitializeAccessibilityNodeInfo(Android.Views.View host, AndroidX.Core.View.Accessibility.AccessibilityNodeInfoCompat info) | ||
| { | ||
| base.OnInitializeAccessibilityNodeInfo(host, info); | ||
| info.ViewIdResourceName = _resourceName; | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| #if ANDROID | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue35826 : _IssuesUITest | ||
| { | ||
| public Issue35826(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "PickPhotosAsync hangs when called from a child activity"; | ||
|
|
||
| const string OpenChildActivityButton = "OpenChildActivityButton"; | ||
| const string ChildActivityPickButton = "ChildActivityPickButton"; | ||
| const string ChildActivityResultLabel = "ChildActivityResultLabel"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.Essentials)] | ||
| public void PickPhotosAsyncShouldReturnFromChildActivity() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Regression Prevention — Test Coverage — This test only covers the child-activity cancel path. The fix also changes adjacent high-risk behavior for activity recreation while a picker is open and overlapping launches from the same activity, but neither scenario is exercised here. Those are the places where the new per-activity pending-request table is most likely to strand or misroute a result, so they need regression coverage or an explicit acknowledged limitation before this plumbing change is safe to merge. |
||
| { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Regression Prevention and Test Coverage — This is the only regression test added for this fix, and it covers just the child-activity launch scenario; it does not cover the config-change/rotation-while-picker-is-open scenario (see the critical finding on |
||
| // This regression only manifests on Android API 36, where the ActivityResultLauncher | ||
| // ownership rules are enforced strictly enough that using the wrong activity's launcher | ||
| // causes the result to never be delivered, hanging the task indefinitely. | ||
| if (App is AppiumApp appiumApp) | ||
| { | ||
| var apiLevel = (long?)appiumApp.Driver.Capabilities.GetCapability("deviceApiLevel") ?? 0; | ||
| if (apiLevel < 36) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Regression Prevention — The
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Regression Prevention — Test Coverage — The regression test is skipped on every Android device below API 36. Because this PR changes shared ActivityResultLauncher/TCS ownership for all Android photo-picker calls, and the available PR environment does not reproduce/run this API 36-only path, the test will not catch regressions in the gate that validates the PR. Please either add coverage that exercises the ownership bug on a CI-supported API level, add an API 36 lane for this test, or document the remaining coverage gap explicitly. |
||
| { | ||
| Assert.Ignore($"Issue #35826 only manifests on Android API 36+. Current device API: {apiLevel}."); | ||
| } | ||
| } | ||
|
Comment on lines
+25
to
+32
|
||
|
|
||
| // Verify the host page loaded | ||
| App.WaitForElement(OpenChildActivityButton); | ||
|
|
||
| // Open the child (non-MAUI AppCompatActivity) | ||
| App.Tap(OpenChildActivityButton); | ||
|
|
||
| // Verify the child activity's UI is visible | ||
| App.WaitForElement(ChildActivityPickButton); | ||
| App.WaitForElement(ChildActivityResultLabel); | ||
|
|
||
| // Tap Pick Photos — calls MediaPicker.PickPhotosAsync() from the child activity. | ||
| // Before the fix the ActivityResultLauncher was never registered for child activities | ||
| // (the guard in ActivityForResultRequest.Register() blocked it), so the task hung | ||
| // indefinitely and the result label stayed on "Picking...". | ||
| App.Tap(ChildActivityPickButton); | ||
|
|
||
| // Cancel the system photo picker by pressing Back. | ||
| // After the fix each activity has its own launcher entry so the result is delivered. | ||
| App.Back(); | ||
|
|
||
| // If the bug is present WaitForTextToBePresentInElement times out because the | ||
| // TaskCompletionSource is never resolved. With the fix it updates promptly to | ||
| // the expected cancellation state. Error indicates a launcher/ownership failure | ||
| // or another exception path and must fail this regression. | ||
| var returned = App.WaitForTextToBePresentInElement(ChildActivityResultLabel, "Cancelled", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Regression Prevention — The regression test accepts any
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Regression Prevention and Test Coverage — Treating |
||
| timeout: TimeSpan.FromSeconds(120)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[minor] Regression Prevention — A 120-second timeout for pressing Back on the photo picker is disproportionate. Cancellation from a Back press completes within 1–2 seconds under normal conditions. The test suite's typical ceiling for similar UI interactions is 30 seconds (see |
||
|
|
||
| var resultText = App.FindElement(ChildActivityResultLabel).GetText(); | ||
|
|
||
| Assert.That(returned, Is.True, | ||
| $"PickPhotosAsync must return from a child activity as a cancellation result after backing out of the picker. " + | ||
| $"Actual result label: '{resultText}'. " + | ||
| $"If this fails the result label is still showing 'Picking...' after 120 seconds or an exception path was hit."); | ||
|
|
||
| Assert.That(resultText, Does.Not.Contain("Picking"), | ||
| "PickPhotosAsync must not hang in a child activity."); | ||
|
|
||
| Assert.That(resultText, Does.Not.Contain("Error"), | ||
| $"PickPhotosAsync should cancel cleanly when backing out of the picker, not surface an exception. Actual result label: '{resultText}'."); | ||
|
|
||
| // Return to the host page | ||
| App.Back(); | ||
| App.WaitForElement(OpenChildActivityButton); | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| using Android.Content.PM; | ||
| using Android.Graphics; | ||
| using Android.Provider; | ||
| using AndroidX.Activity; | ||
| using AndroidX.Activity.Result; | ||
| using AndroidX.Activity.Result.Contract; | ||
| using Microsoft.Maui.ApplicationModel; | ||
|
|
@@ -177,11 +178,14 @@ void OnResult(Intent intent) | |
|
|
||
| async Task<FileResult> PickUsingPhotoPicker(MediaPickerOptions options, bool photo) | ||
| { | ||
| var launchingActivity = ActivityStateManager.Default.GetCurrentActivity(true) as ComponentActivity | ||
| ?? throw new InvalidOperationException("The current activity must inherit from AndroidX.Activity.ComponentActivity."); | ||
|
|
||
| var pickVisualMediaRequest = new PickVisualMediaRequest.Builder() | ||
| .SetMediaType(photo ? ActivityResultContracts.PickVisualMedia.ImageOnly.Instance : ActivityResultContracts.PickVisualMedia.VideoOnly.Instance) | ||
| .Build(); | ||
|
|
||
| var androidUri = await PickVisualMediaForResult.Instance.Launch(pickVisualMediaRequest); | ||
| var androidUri = await PickVisualMediaForResult.Instance.Launch(launchingActivity, pickVisualMediaRequest); | ||
|
|
||
| if (androidUri?.Equals(AndroidUri.Empty) ?? true) | ||
| { | ||
|
|
@@ -208,6 +212,9 @@ async Task<FileResult> PickUsingPhotoPicker(MediaPickerOptions options, bool pho | |
|
|
||
| async Task<List<FileResult>> PickMultipleUsingPhotoPicker(MediaPickerOptions options, bool photo) | ||
| { | ||
| var launchingActivity = ActivityStateManager.Default.GetCurrentActivity(true) as ComponentActivity | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Logic and Correctness — |
||
| ?? throw new InvalidOperationException("The current activity must inherit from AndroidX.Activity.ComponentActivity."); | ||
|
|
||
| // Android has a limitation that you need to use a different request for single and multiple picks. | ||
| // If the selection limit is 1, we can use the single pick method, | ||
| // otherwise we need to use the multiple pick method. | ||
|
|
@@ -230,7 +237,7 @@ async Task<List<FileResult>> PickMultipleUsingPhotoPicker(MediaPickerOptions opt | |
|
|
||
| var pickVisualMediaRequest = pickVisualMediaRequestBuilder.Build(); | ||
|
|
||
| var androidUris = await PickMultipleVisualMediaForResult.Instance.Launch(pickVisualMediaRequest); | ||
| var androidUris = await PickMultipleVisualMediaForResult.Instance.Launch(launchingActivity, pickVisualMediaRequest); | ||
|
|
||
| if (androidUris?.IsEmpty ?? true) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Platform-Specific Code Scoping —
Theme = "@style/Maui.SplashTheme"is a splash-screen theme (full-bleed image, no action bar, transition drawable) applied to a content activity that shows aTextViewand aButton. This can produce a blank/unstyled background or layout inflation quirks during testing. Use a standard content theme instead, e.g."@style/Maui.MainTheme.NoActionBar".