diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 934fe7623e1d..c2d342fc8af4 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -200,20 +200,6 @@ git commit -m "Fix: Description of the change" - `.github/instructions/android.instructions.md` - Android handler implementation - `.github/instructions/xaml-unittests.instructions.md` - XAML unit test guidelines -### Opening PRs - -All PRs are required to have this at the top of the description: - -``` - -> [!NOTE] -> Are you waiting for the changes in this PR to be merged? -> It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! -``` - -Always put that at the top, without the block quotes. Without it, users will NOT be able to try the PR and your work will have been in vain! - - ## Custom Agents and Skills diff --git a/.github/skills/pr-finalize/SKILL.md b/.github/skills/pr-finalize/SKILL.md index 28ba6916dd8b..9932ac6534c5 100644 --- a/.github/skills/pr-finalize/SKILL.md +++ b/.github/skills/pr-finalize/SKILL.md @@ -127,16 +127,10 @@ Examples: ## Description Requirements PR description should: -1. Start with the required NOTE block (so users can test PR artifacts) -2. Include the base sections from `.github/PULL_REQUEST_TEMPLATE.md` ("Description of Change" and "Issues Fixed"). The skill adds additional structured fields (Root cause, Fix, Key insight, etc.) as recommended enhancements for better agent context. -3. Match the actual implementation +1. Include the base sections from `.github/PULL_REQUEST_TEMPLATE.md` ("Description of Change" and "Issues Fixed"). The skill adds additional structured fields (Root cause, Fix, Key insight, etc.) as recommended enhancements for better agent context. +2. Match the actual implementation ```markdown - -> [!NOTE] -> Are you waiting for the changes in this PR to be merged? -> It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! - ### Description of Change [Must match actual implementation] @@ -229,11 +223,6 @@ Example: "Before: Safe area applied by default (opt-out). After: Only views impl Use structured template only when existing description is inadequate: ```markdown - -> [!NOTE] -> Are you waiting for the changes in this PR to be merged? -> It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! - ### Root Cause [Why the bug occurred - be specific about the code path] diff --git a/.github/skills/pr-finalize/references/complete-example.md b/.github/skills/pr-finalize/references/complete-example.md index 034da2e3d09d..6f8ff08d1006 100644 --- a/.github/skills/pr-finalize/references/complete-example.md +++ b/.github/skills/pr-finalize/references/complete-example.md @@ -9,10 +9,6 @@ This example shows a PR description optimized for future agent success. ## Description ```markdown -> [!NOTE] -> Are you waiting for the changes in this PR to be merged? -> It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! - ### Root Cause In `MauiView.GetAdjustedSafeAreaInsets()` on iOS, views that don't implement `ISafeAreaView` or `ISafeAreaView2` (such as `ContentPresenter`, `Border`) were falling through to return `baseSafeArea`. This applied full device safe area insets to views that never opted into safe area handling, causing double-padding when used inside ControlTemplates. diff --git a/eng/pipelines/common/ui-tests-steps.yml b/eng/pipelines/common/ui-tests-steps.yml index e3c1186e329c..b5b2a8f79a8a 100644 --- a/eng/pipelines/common/ui-tests-steps.yml +++ b/eng/pipelines/common/ui-tests-steps.yml @@ -186,6 +186,8 @@ steps: - task: PublishBuildArtifacts@1 condition: always() displayName: publish artifacts + inputs: + artifactName: drop-$(System.StageName)-$(System.JobName)-$(System.JobAttempt) # Enable Notification Center re-enabled only for catalyst - ${{ if eq(parameters.platform, 'catalyst')}}: diff --git a/src/Controls/src/Core/Shell/ShellNavigationManager.cs b/src/Controls/src/Core/Shell/ShellNavigationManager.cs index dbe205c51e56..efac0f54f2e6 100644 --- a/src/Controls/src/Core/Shell/ShellNavigationManager.cs +++ b/src/Controls/src/Core/Shell/ShellNavigationManager.cs @@ -313,14 +313,27 @@ public static void ApplyQueryAttributes(Element element, ShellRouteParameters qu var mergedData = MergeData(element, filteredQuery, isPopping); //if we are pop or navigating back, we need to apply the query attributes to the ShellContent - if (isPopping) + if (isPopping && mergedData.Count > 0 ) { element.SetValue(ShellContent.QueryAttributesProperty, mergedData); } - baseShellItem.ApplyQueryAttributes(mergedData); + + // Skip applying query attributes if the merged data is empty and we're popping back + // This respects when user calls query.Clear() - they don't want attributes applied on back navigation + if (mergedData.Count > 0 || !isPopping) + { + baseShellItem.ApplyQueryAttributes(mergedData); + } } else if (isLastItem) - element.SetValue(ShellContent.QueryAttributesProperty, MergeData(element, query, isPopping)); + { + var mergedData = MergeData(element, query, isPopping); + // Skip setting query attributes if the merged data is empty and we're popping back + if (mergedData.Count > 0 || !isPopping) + { + element.SetValue(ShellContent.QueryAttributesProperty, mergedData); + } + } ShellRouteParameters MergeData(Element shellElement, ShellRouteParameters data, bool isPopping) { diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs new file mode 100644 index 000000000000..e714715ed485 --- /dev/null +++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs @@ -0,0 +1,167 @@ +using Microsoft.Maui.Controls; +using System.Collections.Generic; + +namespace Maui.Controls.Sample.Issues +{ + [Issue(IssueTracker.Github, 33415, "ApplyQueryAttributes gets called with empty Dictionary on back", PlatformAffected.All)] + public class Issue33415 : TestShell + { + protected override void Init() + { + // Start on a landing page, not the test page + var landingPage = new ContentPage + { + Content = new Button + { + AutomationId = "NavigateToMainPageButton", + Text = "Navigate to Main Page with Parameters", + Command = new Command(async () => + { + await Shell.Current.GoToAsync("MainPage", new Dictionary + { + { "TestKey", "TestValue" } + }); + }) + } + }; + Items.Add(new ShellContent { Title = "Landing", Content = landingPage }); + + // Register the main test page as a route (not starting page) + Routing.RegisterRoute("MainPage", typeof(Issue33415MainPage)); + Routing.RegisterRoute("SecondPage", typeof(Issue33415SecondPage)); + } + } + + public class Issue33415MainPage : ContentPage, IQueryAttributable + { + private readonly Label _statusLabel; + private readonly Label _callCountLabel; + private readonly Button _navigateButton; + private int _applyQueryCallCount = 0; + + public Issue33415MainPage() + { + _statusLabel = new Label + { + AutomationId = "StatusLabel", + Text = "Status: Not called", + Margin = new Thickness(10) + }; + + _callCountLabel = new Label + { + AutomationId = "CallCountLabel", + Text = "Call count: 0", + Margin = new Thickness(10) + }; + + _navigateButton = new Button + { + AutomationId = "NavigateButton", + Text = "Navigate to Second Page", + Margin = new Thickness(10) + }; + + _navigateButton.Clicked += OnNavigateClicked; + + Content = new StackLayout + { + Padding = new Thickness(10), + Children = + { + new Label + { + Text = "Issue 33415 - ApplyQueryAttributes called with empty Dictionary on back", + FontSize = 16, + FontAttributes = FontAttributes.Bold, + Margin = new Thickness(0, 0, 0, 20) + }, + _statusLabel, + _callCountLabel, + _navigateButton + } + }; + } + + public void ApplyQueryAttributes(IDictionary query) + { + _applyQueryCallCount++; + _callCountLabel.Text = $"Call count: {_applyQueryCallCount}"; + + if (query.Count == 0) + { + _statusLabel.Text = "Status: ApplyQueryAttributes called with EMPTY dictionary"; + } + else + { + _statusLabel.Text = $"Status: ApplyQueryAttributes called with {query.Count} parameter(s)"; + + // According to documentation, calling Clear() should prevent + // ApplyQueryAttributes from being called again on back navigation + query.Clear(); + } + } + + private async void OnNavigateClicked(object sender, System.EventArgs e) + { + // Navigate to second page - pass through the same parameter for testing + await Shell.Current.GoToAsync("SecondPage", new Dictionary + { + { "TestKey", "TestValue" } + }); + } + } + + [QueryProperty(nameof(TestKey), "TestKey")] + public class Issue33415SecondPage : ContentPage + { + private readonly Label _receivedLabel; + private string _testKey; + + public string TestKey + { + get => _testKey; + set + { + _testKey = value; + _receivedLabel.Text = $"Received: {value ?? "null"}"; + } + } + + public Issue33415SecondPage() + { + _receivedLabel = new Label + { + AutomationId = "ReceivedLabel", + Text = "Received: null", + Margin = new Thickness(10) + }; + + var backButton = new Button + { + AutomationId = "BackButton", + Text = "Go Back", + Margin = new Thickness(10) + }; + + backButton.Clicked += async (s, e) => await Shell.Current.GoToAsync(".."); + + Content = new StackLayout + { + Padding = new Thickness(10), + Children = + { + new Label + { + Text = "Second Page", + FontSize = 16, + FontAttributes = FontAttributes.Bold, + Margin = new Thickness(0, 0, 0, 20) + }, + _receivedLabel, + backButton + } + }; + } + } +} diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs new file mode 100644 index 000000000000..78c477aef22e --- /dev/null +++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs @@ -0,0 +1,68 @@ +using NUnit.Framework; +using UITest.Appium; +using UITest.Core; + +namespace Microsoft.Maui.TestCases.Tests.Issues +{ + public class Issue33415 : _IssuesUITest + { + public override string Issue => "ApplyQueryAttributes gets called with empty Dictionary on back"; + + public Issue33415(TestDevice device) : base(device) { } + + [Test] + [Category(UITestCategories.Shell)] + public void ApplyQueryAttributesShouldNotBeCalledWithEmptyDictionaryOnBack() + { + // Start on landing page - tap to navigate TO MainPage WITH parameters + App.WaitForElement("NavigateToMainPageButton"); + App.Tap("NavigateToMainPageButton"); + + // Wait for main page to appear + App.WaitForElement("StatusLabel"); + App.WaitForElement("CallCountLabel"); + + // Verify ApplyQueryAttributes was called with parameters + var callCountAfterNav = App.FindElement("CallCountLabel").GetText(); + var statusAfterNav = App.FindElement("StatusLabel").GetText(); + Assert.That(callCountAfterNav, Is.EqualTo("Call count: 1"), "ApplyQueryAttributes should be called once with parameters"); + Assert.That(statusAfterNav, Does.Contain("1 parameter"), "Status should indicate parameters were received"); + + // Navigate to second page + App.Tap("NavigateButton"); + + // Wait for second page to appear + App.WaitForElement("ReceivedLabel"); + + // Verify second page received the parameter (passed through from MainPage navigation) + var receivedText = App.FindElement("ReceivedLabel").GetText(); + Assert.That(receivedText, Is.EqualTo("Received: TestValue"), "Second page should receive the parameter"); + + // Go back to main page + App.Tap("BackButton"); + + // Wait for main page to reappear + App.WaitForElement("StatusLabel"); + App.WaitForElement("CallCountLabel"); + + // BUG: According to documentation, after calling query.Clear() in ApplyQueryAttributes, + // the method should NOT be called again when navigating back. + // However, it IS called with an empty dictionary. + var finalCallCount = App.FindElement("CallCountLabel").GetText(); + var finalStatus = App.FindElement("StatusLabel").GetText(); + + // This assertion will FAIL (demonstrating the bug) + // Expected: Call count should still be 1 (ApplyQueryAttributes should not be called on back) + // Actual: Call count will be 2 (ApplyQueryAttributes IS called with empty dictionary) + Assert.That(finalCallCount, Is.EqualTo("Call count: 1"), + "ApplyQueryAttributes should NOT be called when navigating back after query.Clear()"); + + // If the bug exists, this will show it was called with empty dictionary + if (finalCallCount != "Call count: 1") + { + Assert.That(finalStatus, Is.EqualTo("Status: ApplyQueryAttributes called with EMPTY dictionary"), + "If ApplyQueryAttributes is incorrectly called, it's called with empty dictionary"); + } + } + } +}