Fixed BindingContext of the Window TitleBar is not being passed on to its child content.#30080
Conversation
|
Hey there @@NirmalKumarYuvaraj! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue where the BindingContext of the Window TitleBar was not propagated to its child content by adding an override for OnBindingContextChanged in the TitleBar class and updating the property change handlers.
- Propagated BindingContext changes to Content, LeadingContent, and TrailingContent.
- Added test cases in TestCases.Shared.Tests and updated the host sample in TestCases.HostApp to validate the fix.
- Updated PublicAPI files to include the new override method.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue24831.cs | Added a test case to verify BindingContext propagation for the TitleBar. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue24831.cs | Implemented a sample demonstrating the TitleBar BindingContext propagation fix. |
| src/Controls/src/Core/TitleBar/TitleBar.cs | Overrode OnBindingContextChanged and adjusted OnLeadingChanged, OnContentChanged, and OnTrailingContentChanged to propagate the BindingContext. |
| PublicAPI Unshipped Files | Declared the new override in the public API for multiple target frameworks. |
|
@NirmalKumarYuvaraj Could you rebase to fix the conflicts? |
fe27d8d to
3690fe2
Compare
@jsuarezruiz , I have rebased. |
Sorry, but requires rebase again :( |
3690fe2 to
048d802
Compare
Rebased |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
|
kubaflo
left a comment
There was a problem hiding this comment.
Could you please resolve conflicts?
dc298c1 to
aeb72fb
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 30080Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 30080" |
|
@kubaflo , I have resolved the conflicts. Please let me know if you have any concerns. |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…otnet#34548) <!-- Please let the below note in for people that find this PR --> > [!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 Adds a [gh-aw (GitHub Agentic Workflows)](https://github.github.com/gh-aw/introduction/overview/) workflow that automatically evaluates test quality on PRs using the `evaluate-pr-tests` skill. ### What it does When a PR adds or modifies test files, this workflow: 1. **Checks out the PR branch** (including fork PRs) in a pre-agent step 2. **Runs the `evaluate-pr-tests` skill** via Copilot CLI in a sandboxed container 3. **Posts the evaluation report** as a PR comment using gh-aw safe-outputs ### Triggers | Trigger | When | Fork PR support | |---------|------|-----------------| | `pull_request` | Automatic on test file changes (`src/**/tests/**`) | ❌ Blocked by `pre_activation` gate | | `workflow_dispatch` | Manual — enter PR number | ✅ Works for all PRs | | `issue_comment` (`/evaluate-tests`) | Comment on PR |⚠️ Same-repo only (see Known Limitations) | ### Security model | Layer | Implementation | |-------|---------------| | **gh-aw sandbox** | Agent runs in container with scrubbed credentials, network firewall | | **Safe outputs** | Max 1 PR comment per run, content-limited | | **Checkout without execution** | `steps:` checks out PR code but never executes workspace scripts | | **Base branch restoration** | `.github/skills/`, `.github/instructions/`, `.github/copilot-instructions.md` restored from base branch after checkout | | **Fork PR activation gate** | `pull_request` events blocked for forks via `head.repo.id == repository_id` | | **Pinned actions** | SHA-pinned `actions/checkout`, `actions/github-script`, etc. | | **Minimal permissions** | Each job declares only what it needs | | **Concurrency** | One evaluation per PR, cancels in-progress | | **Threat detection** | gh-aw built-in threat detection analyzes agent output | ### Files added/modified - `.github/workflows/copilot-evaluate-tests.md` — gh-aw workflow source - `.github/workflows/copilot-evaluate-tests.lock.yml` — Compiled workflow (auto-generated by `gh aw compile`) - `.github/skills/evaluate-pr-tests/scripts/Gather-TestContext.ps1` — Test context gathering script (binary-safe file download, path traversal protection) - `.github/instructions/gh-aw-workflows.instructions.md` — Copilot instructions for gh-aw development ### Known Limitations **Fork PR evaluation via `/evaluate-tests` comment is not supported in v1.** The gh-aw platform inserts a `checkout_pr_branch.cjs` step after all user steps, which may overwrite base-branch skill files restored for fork PRs. This is a known gh-aw platform limitation — user steps always run before platform-generated steps, with no way to insert steps after. **Workaround:** Use `workflow_dispatch` (Actions UI → "Run workflow" → enter PR number) to evaluate fork PRs. This trigger bypasses the platform checkout step entirely and works correctly. **Related upstream issues:** - [github/gh-aw#18481](github/gh-aw#18481) — "Using gh-aw in forks of repositories" - [github/gh-aw#18518](github/gh-aw#18518) — Fork detection and warning in `gh aw init` - [github/gh-aw#18520](github/gh-aw#18520) — Fork context hint in failure messages - [github/gh-aw#18521](github/gh-aw#18521) — Fork support documentation ### Fixes - Fixes dotnet#34602 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
## Summary Enables the copilot-evaluate-tests gh-aw workflow to run on fork PRs by adding `forks: ["*"]` to the `pull_request` trigger and removing the fork guard from `Checkout-GhAwPr.ps1`. ## Changes 1. **copilot-evaluate-tests.md**: Added `forks: ["*"]` to opt out of gh-aw auto-injected fork activation guard. Scoped `Checkout-GhAwPr.ps1` step to `workflow_dispatch` only (redundant for other triggers since platform handles checkout). 2. **copilot-evaluate-tests.lock.yml**: Recompiled via `gh aw compile` — fork guard removed from activation `if:` conditions. 3. **Checkout-GhAwPr.ps1**: Removed the `isCrossRepository` fork guard. Updated header docs and restore comments to accurately describe behavior for all trigger×fork combinations (including corrected step ordering). 4. **gh-aw-workflows.instructions.md**: Updated all stale references to the removed fork guard. Documented `forks: ["*"]` opt-in, clarified residual risk model for fork PRs, and updated troubleshooting table. ## Security Model Fork PRs are safe because: - Agent runs in **sandboxed container** with all credentials scrubbed - Output limited to **1 comment** via `safe-outputs: add-comment: max: 1` - Agent **prompt comes from base branch** (`runtime-import`) — forks cannot alter instructions - Pre-flight check catches missing `SKILL.md` if fork isn't rebased on `main` - No workspace code is executed with `GITHUB_TOKEN` (checkout without execution) ## Testing - ✅ `workflow_dispatch` tested against fork PR dotnet#34621 - ✅ Lock.yml statically verified — fork guard removed from `if:` conditions - ⏳ `pull_request` trigger on fork PRs can only be verified post-merge (GitHub Actions reads lock.yml from default branch) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aeb72fb to
5d89b17
Compare
|
@kubaflo , The Last AI summary is incomplete. Gate failed due to baseline snap missing. I have attached the baseline snap for windows. |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #30080 | Override OnBindingContextChanged() + update property-changed handlers to propagate/clear BindingContext |
⏳ PENDING (Gate) | TitleBar.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: ✅ PASSED
Platform: Windows
| # | Type | Test Name | Filter |
|---|---|---|---|
| 1 | UITest | Issue24831 | Issue24831 |
| Step | Expected | Actual | Result |
|---|---|---|---|
| Without fix | FAIL | FAIL | ✅ |
| With fix | PASS | PASS | ✅ |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Logical child management (AddLogicalChild/RemoveLogicalChild) + SetChildInheritedBindingContext override |
✅ PASS | 1 file | Fixes root cause in TemplatedView; 2 iterations |
| 2 | try-fix (claude-sonnet-4.6) | Live TypedBinding.ForSingleNestingLevel on child's BindingContextProperty — no override needed |
✅ PASS | 1 file | Trim-safe; 1 iteration; unconventional pattern |
| 3 | try-fix (gpt-5.3-codex) | Template-level propagation in BuildDefaultTemplate() — bind PART_ hosts to TitleBar.BindingContext |
✅ PASS | 1 file | Minimal; may break if ControlTemplate replaced |
| 4 | try-fix (gemini-3-pro-preview) | Subscribe to BindingContextChanged event in constructor; propagate to children in handler + set in property-changed handlers |
✅ PASS | 1 file | 4 iterations; conceptually similar to PR fix |
| PR | PR #30080 | Override OnBindingContextChanged() + SetInheritedBindingContext in each property-changed handler (clear old, set new) |
✅ PASSED (Gate) | 1 file | Canonical MAUI pattern (matches ContentView/Frame) |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | Design space exhausted |
| claude-sonnet-4.6 | 2 | No | Design space exhausted |
| gpt-5.3-codex | 2 | Yes | Set value.Parent = this — mechanistically same as Attempt 1; no new attempt needed |
| gemini-3-pro-preview | 2 | Yes | Same value.Parent = this suggestion; covered by Attempt 1 |
Exhausted: Yes
Selected Fix: PR's fix — matches the canonical MAUI pattern for TemplatedView subclasses with content properties (same as ContentView, Frame, Border). Correctly handles both "content set before BindingContext" and "BindingContext changes after content set" scenarios. The minor code duplication across the 3 handlers is a style concern already noted and resolved as a nitpick.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #24831, TitleBar BindingContext propagation on Windows/MacCatalyst |
| Gate | ✅ PASSED | Windows — Issue24831 FAIL without fix, PASS with fix |
| Try-Fix | ✅ COMPLETE | 4 attempts, all 4 passing; PR's fix selected |
| Report | ✅ COMPLETE |
Summary
PR #30080 correctly fixes the BindingContext propagation for TitleBar's Content, LeadingContent, and TrailingContent child views on Windows (and MacCatalyst). The fix is well-targeted, tested, and matches the canonical MAUI pattern. All 4 independent exploration models found alternative passing approaches, but the PR's fix is the best fit for the codebase.
Root Cause
TitleBar extends TemplatedView. Its Content, LeadingContent, and TrailingContent are stored as bindable properties, not as logical/visual children of TitleBar. MAUI's inherited BindingContext propagation only traverses logical and visual children — it never reaches these views. When window.BindingContext = viewModel is set, the TitleBar receives the context change, but the three content views remain disconnected.
Fix Quality
The PR's approach is correct and idiomatic:
- Overrides
OnBindingContextChanged()to propagate the TitleBar'sBindingContextto all three content views — handles the case where BindingContext changes after content is set - Updates
OnLeadingChanged,OnContentChanged, andOnTrailingContentChangedto apply/clear BindingContext when content properties change — handles the case where content is set after BindingContext is set - Calls
base.OnBindingContextChanged()at the end of the override — correct MAUI convention - Uses
SetInheritedBindingContext(not direct assignment) — correct way to propagate inherited context in MAUI
This pattern exactly mirrors how ContentView, Frame, and Border handle their Content property in the MAUI codebase.
Minor concern (non-blocking): The three On*Changed handlers contain identical 5-line blocks for propagating BindingContext. A helper method would reduce duplication. This was already raised as a resolved nitpick by the Copilot code review; since the PR author acknowledged it, this is acceptable as-is.
Test coverage: The PR includes a UITest (Issue24831) that verifies all three child views (Content/LeadingContent/TrailingContent) receive the BindingContext correctly by checking bound values render in the title bar. A Windows snapshot is included. The test FAILS without the fix and PASSES with it, providing valid regression coverage.
Try-Fix comparison: Four alternative approaches were explored — all passed, but each had trade-offs:
- Attempt 1 (logical children): More complex, overrides an internal method
- Attempt 2 (TypedBinding): Unconventional use of binding infrastructure for BindingContext itself
- Attempt 3 (template-level): Simpler but fragile if ControlTemplate is replaced by user
- Attempt 4 (event subscription): More indirection, required 4 test iterations
The PR's fix is the simplest, most idiomatic, and most maintainable of all candidates.
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 from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details:
BindingContext of the Window TitleBar is not being passed on to its child content (Content, LeadingContent, TrailingContent).
Description of Change
OnBindingContextChangedmethod in theTitleBarclass to propagate theBindingContextto its child elements (Content,LeadingContent, andTrailingContent).OnLeadingChanged,OnContentChanged, andOnTrailingContentChangedmethods to clear and reapply the inheritedBindingContextwhen the respective properties change.Validated the behaviour in the following platforms
Issues Fixed
Fixes #24831
Output