block. Capture extra signal
+# so reviewers and downstream agents can act on it.
+function Get-GateFallbackDetails {
+ param([string]$Tail, [int]$ExitCode, [string]$VerifyDir, [string]$ReviewedPlatform)
+
+ $sections = @()
+
+ $sections += "**Exit code:** ``$ExitCode``"
+
+ # Surface auto-detected tests / fix files from the verify script's stdout
+ # so reviewers can tell whether detection failed vs. the test run itself.
+ $detected = @{}
+ foreach ($line in ($Tail -split "`n")) {
+ if ($line -match '^\s*Detected test type:\s*(\S+)') { $detected['type'] = $Matches[1] }
+ elseif ($line -match '^\s*Test filter:\s*(\S+)') { $detected['filter'] = $Matches[1] }
+ elseif ($line -match '^\s*Fix files \((\d+)\):') { $detected['fixCount'] = $Matches[1] }
+ elseif ($line -match '^\s*Merge base:\s*(\S+)') { $detected['mergeBase'] = $Matches[1] }
+ }
+ if ($detected.Count -gt 0) {
+ $items = @()
+ if ($detected.ContainsKey('type')) { $items += "- Test type: ``$($detected['type'])``" }
+ if ($detected.ContainsKey('filter')) { $items += "- Test filter: ``$($detected['filter'])``" }
+ if ($detected.ContainsKey('fixCount')) { $items += "- Fix files detected: $($detected['fixCount'])" }
+ if ($detected.ContainsKey('mergeBase')) { $items += "- Merge base: ``$($detected['mergeBase'])``" }
+ $sections += "**Auto-detected:**`n" + ($items -join "`n")
+ }
+
+ # Heuristic classification — make the cause actionable instead of leaving the
+ # reader to grep stderr.
+ $likely = @()
+ if ($Tail -match '(?i)Could not auto-detect PR number|no tests detected|0 test\(s\) detected') {
+ $likely += "Test detection failed — no runnable tests were found in the PR diff."
+ }
+ # Match coded build errors (CS, MSB, NU, MAUI, NETSDK, XA, etc.) without false-positiving
+ # on lines like "MSBUILD output: 0 error(s)". The general form `error ` covers
+ # compiler, MSBuild, NuGet restore, MAUI analyzer, .NET SDK, Android packaging diagnostics.
+ if ($Tail -match '(?i)build failed|\berror\s+[A-Z]{2,}\d+\b') {
+ $likely += "Build error before any test could run."
+ }
+ if ($Tail -match '(?i)emulator.*(?:timeout|failed|not.found)|adb.*(?:server|crashed)|xharness.*(?:failed|timeout)') {
+ $likely += "Device/emulator setup failed (env error class)."
+ }
+ if ($Tail -match '(?i)merge.conflict|conflict.*merge.base') {
+ $likely += "Merge conflict prevented running the gate."
+ }
+ if ($Tail -match '(?i)NO FIX FILES DETECTED') {
+ $likely += "No fix files detected in the diff (PR may be test-only — should now run in failure-only mode)."
+ }
+ if ($likely.Count -gt 0) {
+ $sections += "**Likely cause:**`n" + (($likely | ForEach-Object { "- $_" }) -join "`n")
+ }
+
+ # List artifacts under gate/verify-tests-fail/ — partial logs sometimes survive
+ # even when the report itself was not written.
+ if (Test-Path $VerifyDir) {
+ $files = Get-ChildItem -Path $VerifyDir -File -ErrorAction SilentlyContinue |
+ Sort-Object Name | ForEach-Object { "- ``$($_.Name)`` ($([math]::Round($_.Length / 1KB, 1)) KB)" }
+ if ($files) {
+ $sections += "**Artifacts written before exit:**`n" + ($files -join "`n")
+ }
+ }
+
+ return ($sections -join "`n`n")
+}
if (Test-Path $verificationReport) {
$reportContent = Get-Content $verificationReport -Raw -ErrorAction SilentlyContinue
@@ -612,13 +688,18 @@ if (Test-Path $verificationReport) {
# Report exists but has bad format — generate fallback with logs
Write-Host " ⚠️ Verification report has invalid format — using fallback" -ForegroundColor Yellow
$resultIcon = switch ($gateResult) { "PASSED" { "✅" } "SKIPPED" { "⚠️" } default { "❌" } }
+ $fallbackDetails = Get-GateFallbackDetails -Tail $gateLogTail -ExitCode $gateExitCode -VerifyDir (Join-Path $gateOutputDir "verify-tests-fail") -ReviewedPlatform $gatePlatform
@"
### Gate Result: $resultIcon $gateResult
**Platform:** $($gatePlatform.ToUpper())
+> ⚠️ ``verify-tests-fail.ps1`` produced an empty report. Diagnostics below.
+
+$fallbackDetails
+
-
Gate output log
+Gate output log (last 60 lines)
`````` $gateLogTail @@ -638,13 +719,18 @@ No tests were detected in this PR. "@ | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 } else { $resultIcon = switch ($gateResult) { "PASSED" { "✅" } default { "❌" } } + $fallbackDetails = Get-GateFallbackDetails -Tail $gateLogTail -ExitCode $gateExitCode -VerifyDir (Join-Path $gateOutputDir "verify-tests-fail") -ReviewedPlatform $gatePlatform @" ### Gate Result: $resultIcon $gateResult **Platform:** $($gatePlatform.ToUpper()) +> ⚠️ ``verify-tests-fail.ps1`` exited before writing a verification report. Diagnostics below. + +$fallbackDetails +
-"
+}
+``````
+Rules:
+- ``isPRFix`` MUST be ``true`` when ``winner`` is ``pr`` or ``pr-plus-reviewer``.
+- ``isPRFix`` MUST be ``false`` when ``winner`` is any ``try-fix-*``.
+- When ``isPRFix`` is ``false``, ``candidateDiff`` MUST be a non-empty unified diff. Truncate at 55 KB if larger and end with a ``... [truncated]`` marker line.
$platformInstruction
$autonomousRules
@@ -723,9 +848,6 @@ $autonomousRules
**Gate result (already completed in a prior step):** $gateStatusForPrompt
Do NOT re-run gate verification. The gate phase is handled separately.
⚠️ Do NOT create or overwrite ``gate/content.md`` — it is already generated by the gate script with detailed test output.
-
-📁 Write phase output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/{phase}/content.md``
-(phases: pre-flight, try-fix, report — NOT gate)
"@
Invoke-CopilotStep -StepName "STEP 2: PR REVIEW" -Prompt $step2Prompt | Out-Null
@@ -806,6 +928,138 @@ if (Test-Path $reviewScript) {
Write-Host " ⚠️ post-ai-summary-comment.ps1 not found — skipping review summary" -ForegroundColor Yellow
}
+# Determine winning candidate (winner.json) — drives whether we post inline findings or request changes
+$winnerFile = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/winner.json"
+$winner = $null
+if (Test-Path $winnerFile) {
+ try {
+ $winner = Get-Content -Raw -LiteralPath $winnerFile | ConvertFrom-Json
+ # Validation
+ $allowed = @('pr','pr-plus-reviewer','try-fix-1','try-fix-2','try-fix-3','try-fix-4')
+ if (-not $winner.winner -or $allowed -notcontains $winner.winner) {
+ Write-Host " ⚠️ winner.json has invalid 'winner' value: $($winner.winner) — falling back to PR-fix path" -ForegroundColor Yellow
+ $winner = $null
+ } elseif ($winner.winner -in @('pr','pr-plus-reviewer') -and $winner.isPRFix -ne $true) {
+ Write-Host " ⚠️ winner.json: '$($winner.winner)' must have isPRFix=true — overriding" -ForegroundColor Yellow
+ $winner.isPRFix = $true
+ } elseif ($winner.winner -like 'try-fix-*' -and $winner.isPRFix -ne $false) {
+ Write-Host " ⚠️ winner.json: '$($winner.winner)' must have isPRFix=false — overriding" -ForegroundColor Yellow
+ $winner.isPRFix = $false
+ }
+ if ($winner -and $winner.isPRFix -eq $false -and [string]::IsNullOrWhiteSpace($winner.candidateDiff)) {
+ Write-Host " ⚠️ winner.json: non-PR winner has empty candidateDiff — falling back to PR-fix path" -ForegroundColor Yellow
+ $winner = $null
+ }
+ if ($winner) {
+ Write-Host " 🏆 Winning candidate: $($winner.winner) (isPRFix=$($winner.isPRFix))" -ForegroundColor Cyan
+ }
+ } catch {
+ Write-Host " ⚠️ Failed to parse winner.json (non-fatal): $_ — falling back to PR-fix path" -ForegroundColor Yellow
+ $winner = $null
+ }
+} else {
+ Write-Host " ℹ️ No winner.json — defaulting to PR-fix posting path" -ForegroundColor Gray
+}
+
+$isPRWinner = (-not $winner) -or ($winner.isPRFix -eq $true)
+
+if ($isPRWinner) {
+ # Post inline review comments (file:line findings from expert-reviewer agent)
+ $inlineScript = Join-Path $summaryScriptsDir "post-inline-review.ps1"
+ $findingsFile = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json"
+ if ((Test-Path $inlineScript) -and (Test-Path $findingsFile)) {
+ try {
+ Write-Host " 📝 Posting inline review comments..." -ForegroundColor Cyan
+ if ($DryRun) {
+ & $inlineScript -PRNumber $PRNumber -FindingsFile $findingsFile -DryRun
+ } else {
+ & $inlineScript -PRNumber $PRNumber -FindingsFile $findingsFile
+ }
+ Write-Host " ✅ Inline review comments posted" -ForegroundColor Green
+ } catch {
+ Write-Host " ⚠️ Inline review posting failed (non-fatal): $_" -ForegroundColor Yellow
+ }
+ } else {
+ if (-not (Test-Path $findingsFile)) {
+ Write-Host " ℹ️ No inline findings file — agent may not have produced findings" -ForegroundColor Gray
+ }
+ }
+} else {
+ # Non-PR candidate won — submit a REQUEST_CHANGES review with the candidate diff in the body
+ Write-Host " 📝 Non-PR candidate won — submitting REQUEST_CHANGES review with candidate diff..." -ForegroundColor Cyan
+
+ $maxDiffBytes = 55KB
+ $diff = [string]$winner.candidateDiff
+ $truncated = $false
+ # Truncate by binary-searching the largest character count whose UTF-8
+ # encoding fits within the byte budget (reserving room for the marker line).
+ # O(log n) and much cheaper than the previous O(n²) trim-512-and-recount loop.
+ $marker = "`n... [truncated]"
+ $markerBytes = [System.Text.Encoding]::UTF8.GetByteCount($marker)
+ $budget = $maxDiffBytes - $markerBytes
+ if ([System.Text.Encoding]::UTF8.GetByteCount($diff) -gt $maxDiffBytes) {
+ $lo = 0
+ $hi = $diff.Length
+ while ($lo -lt $hi) {
+ $mid = [int](($lo + $hi + 1) / 2)
+ $bytes = [System.Text.Encoding]::UTF8.GetByteCount($diff.Substring(0, $mid))
+ if ($bytes -le $budget) { $lo = $mid } else { $hi = $mid - 1 }
+ }
+ $diff = $diff.Substring(0, $lo) + $marker
+ $truncated = $true
+ }
+
+ # Compute an outer code fence longer than any backtick run inside the diff
+ # (minimum 4) so the diff content cannot accidentally close the fence and
+ # leak into the surrounding markdown. Preserves the diff text exactly.
+ $maxBacktickRun = 0
+ foreach ($m in [regex]::Matches($diff, '`+')) {
+ if ($m.Length -gt $maxBacktickRun) { $maxBacktickRun = $m.Length }
+ }
+ $fenceLen = [Math]::Max(4, $maxBacktickRun + 1)
+ $fence = '`' * $fenceLen
+
+ $rationale = if ($winner.summary) { [string]$winner.summary } else { "Automated review identified a stronger candidate fix." }
+ $reviewBody = @"
+🤖 **Automated review — alternative fix proposed**
+
+The expert-reviewer evaluation compared the PR fix against $($winner.winner -replace 'try-fix-','#') automatically generated candidates and selected ``$($winner.winner)`` as the strongest fix.
+
+**Why:** $rationale
+
+Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
+
+
+$( if ($truncated) { "`n_The diff was truncated to fit GitHub's review body limit._" } )
+"@
+
+ if ($DryRun) {
+ Write-Host " [DryRun] Would POST review state=REQUEST_CHANGES with body length $($reviewBody.Length)" -ForegroundColor Yellow
+ } else {
+ try {
+ $bodyJson = @{ body = $reviewBody; event = 'REQUEST_CHANGES' } | ConvertTo-Json -Compress -Depth 5
+ $tmp = New-TemporaryFile
+ Set-Content -LiteralPath $tmp -Value $bodyJson -Encoding utf8 -NoNewline
+ $resp = & gh api -X POST "repos/dotnet/maui/pulls/$PRNumber/reviews" --input $tmp 2>&1
+ Remove-Item -LiteralPath $tmp -Force -ErrorAction SilentlyContinue
+ if ($LASTEXITCODE -eq 0) {
+ Write-Host " ✅ REQUEST_CHANGES review submitted" -ForegroundColor Green
+ } else {
+ Write-Host " ⚠️ Failed to submit REQUEST_CHANGES review (non-fatal): $resp" -ForegroundColor Yellow
+ }
+ } catch {
+ Write-Host " ⚠️ REQUEST_CHANGES submission threw (non-fatal): $_" -ForegroundColor Yellow
+ }
+ }
+ Write-Host " ⏭️ Skipping inline findings (winner is not the PR fix)" -ForegroundColor Gray
+}
+
# ═════════════════════════════════════════════════════════════════════════════
# STEP 4: Apply Labels
# ═════════════════════════════════════════════════════════════════════════════
diff --git a/.github/scripts/post-inline-review.ps1 b/.github/scripts/post-inline-review.ps1
new file mode 100644
index 000000000000..e4fe45f509df
--- /dev/null
+++ b/.github/scripts/post-inline-review.ps1
@@ -0,0 +1,264 @@
+#!/usr/bin/env pwsh
+<#
+.SYNOPSIS
+ Posts inline review comments on a GitHub Pull Request from a JSON findings file.
+
+.DESCRIPTION
+ Reads inline-findings.json (produced by the maui-expert-reviewer agent) and posts
+ them as a GitHub PR review with inline file:line comments.
+
+ Also posts review-summary.md as the review body.
+
+ Uses the GitHub Pulls Review API to create a single review with all inline comments
+ attached at their exact file:line locations.
+
+.PARAMETER PRNumber
+ The pull request number (required)
+
+.PARAMETER FindingsFile
+ Path to inline-findings.json. Default: CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/inline-findings.json
+
+.PARAMETER SummaryFile
+ Path to review-summary.md. Default: CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/review-summary.md
+
+.PARAMETER DryRun
+ Print the review payload instead of posting
+
+.EXAMPLE
+ ./post-inline-review.ps1 -PRNumber 12345
+
+.EXAMPLE
+ ./post-inline-review.ps1 -PRNumber 12345 -DryRun
+
+.EXAMPLE
+ ./post-inline-review.ps1 -PRNumber 12345 -FindingsFile /tmp/findings.json
+#>
+
+param(
+ [Parameter(Mandatory = $true)]
+ [int]$PRNumber,
+
+ [Parameter(Mandatory = $false)]
+ [string]$FindingsFile,
+
+ [Parameter(Mandatory = $false)]
+ [string]$SummaryFile,
+
+ [Parameter(Mandatory = $false)]
+ [switch]$DryRun
+)
+
+$ErrorActionPreference = "Stop"
+
+# ============================================================================
+# RESOLVE FILE PATHS
+# ============================================================================
+
+$PRAgentDir = "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent"
+if (-not (Test-Path $PRAgentDir)) {
+ $repoRoot = git rev-parse --show-toplevel 2>$null
+ if ($repoRoot) {
+ $PRAgentDir = Join-Path $repoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent"
+ }
+}
+
+if (-not $FindingsFile) {
+ $FindingsFile = Join-Path $PRAgentDir "inline-findings.json"
+}
+if (-not $SummaryFile) {
+ $SummaryFile = Join-Path $PRAgentDir "review-summary.md"
+}
+
+if (-not (Test-Path $FindingsFile)) {
+ Write-Host "No findings file found at: $FindingsFile" -ForegroundColor Yellow
+ Write-Host "Nothing to post." -ForegroundColor Yellow
+ exit 0
+}
+
+# ============================================================================
+# LOAD FINDINGS
+# ============================================================================
+
+Write-Host "Loading findings from: $FindingsFile" -ForegroundColor Cyan
+$findings = Get-Content -Path $FindingsFile -Raw -Encoding UTF8 | ConvertFrom-Json
+
+if (-not $findings -or $findings.Count -eq 0) {
+ Write-Host "No findings to post." -ForegroundColor Green
+ exit 0
+}
+
+Write-Host " Found $($findings.Count) inline findings" -ForegroundColor Gray
+
+# Load summary if available
+$summaryBody = ""
+if (Test-Path $SummaryFile) {
+ $summaryBody = Get-Content -Path $SummaryFile -Raw -Encoding UTF8
+ Write-Host " Loaded summary ($($summaryBody.Length) chars)" -ForegroundColor Gray
+} else {
+ $summaryBody = "## Expert Review — $($findings.Count) findings`n`nSee inline comments for details."
+}
+
+# ============================================================================
+# GET PR HEAD COMMIT (required by GitHub API)
+# ============================================================================
+
+Write-Host "Fetching PR #$PRNumber head commit..." -ForegroundColor Cyan
+$prJson = gh api "repos/dotnet/maui/pulls/$PRNumber" --jq '{sha: .head.sha}' 2>&1
+if ($LASTEXITCODE -ne 0) {
+ throw "Failed to fetch PR #${PRNumber}: $prJson"
+}
+$prData = $prJson | ConvertFrom-Json
+$commitSha = $prData.sha
+Write-Host " HEAD: $commitSha" -ForegroundColor Gray
+
+# ============================================================================
+# BUILD REVIEW PAYLOAD
+# ============================================================================
+
+$comments = @()
+foreach ($f in $findings) {
+ # Defense-in-depth: reject suspicious paths so a malformed/hostile finding
+ # cannot poison the whole review post (especially in the fallback branch
+ # below where the GitHub diff fetch failed and we can't cross-validate).
+ $p = [string]$f.path
+ if ([string]::IsNullOrWhiteSpace($p) -or
+ $p.Contains('..') -or
+ $p.StartsWith('/') -or
+ $p.StartsWith('\') -or
+ $p.Contains('\') -or
+ $p -match '[\x00-\x1F]' -or
+ $p -match '^[A-Za-z]:') {
+ Write-Host " ⚠️ Skipping finding with suspicious path: '$p'" -ForegroundColor Yellow
+ continue
+ }
+
+ $comment = @{
+ path = $p
+ line = [int]$f.line
+ body = $f.body
+ }
+ # GitHub API requires 'side' for pull request review comments
+ $comment['side'] = 'RIGHT'
+ $comments += $comment
+}
+
+# ============================================================================
+# FILTER COMMENTS TO LINES PRESENT IN THE PR DIFF
+# GitHub returns 422 "Line could not be resolved" if ANY comment targets a
+# line outside the diff. Pre-validate to avoid losing the entire review.
+# ============================================================================
+
+Write-Host "Fetching PR diff for line validation..." -ForegroundColor Cyan
+$filesJson = gh api --paginate "repos/dotnet/maui/pulls/$PRNumber/files" 2>&1
+if ($LASTEXITCODE -ne 0) {
+ Write-Host " ⚠️ Could not fetch PR files for validation: $filesJson" -ForegroundColor Yellow
+ Write-Host " Posting all findings without pre-validation." -ForegroundColor Yellow
+} else {
+ $files = $filesJson | ConvertFrom-Json
+ # Build map: path -> set of new-file line numbers commentable on RIGHT side
+ $diffLines = @{}
+ foreach ($file in $files) {
+ $path = $file.filename
+ $patch = $file.patch
+ if (-not $patch) { continue }
+ $lineSet = New-Object System.Collections.Generic.HashSet[int]
+ $newLine = 0
+ foreach ($pl in ($patch -split "`n")) {
+ if ($pl -match '^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@') {
+ $newLine = [int]$Matches[1]
+ continue
+ }
+ if ($pl.StartsWith('+') -and -not $pl.StartsWith('+++')) {
+ [void]$lineSet.Add($newLine)
+ $newLine++
+ } elseif ($pl.StartsWith('-') -and -not $pl.StartsWith('---')) {
+ # deletion — does not advance new-file line
+ } elseif ($pl.StartsWith(' ')) {
+ # context — also commentable (RIGHT side)
+ [void]$lineSet.Add($newLine)
+ $newLine++
+ }
+ }
+ $diffLines[$path] = $lineSet
+ }
+
+ $kept = @()
+ $dropped = @()
+ foreach ($c in $comments) {
+ if ($diffLines.ContainsKey($c.path) -and $diffLines[$c.path].Contains([int]$c.line)) {
+ $kept += $c
+ } else {
+ $dropped += $c
+ }
+ }
+ if ($dropped.Count -gt 0) {
+ Write-Host " ⚠️ Dropping $($dropped.Count) finding(s) whose lines aren't in the PR diff:" -ForegroundColor Yellow
+ foreach ($d in $dropped) {
+ Write-Host " $($d.path):$($d.line)" -ForegroundColor Gray
+ }
+ }
+ Write-Host " ✅ $($kept.Count) of $($comments.Count) findings target lines in the diff" -ForegroundColor Gray
+ $comments = $kept
+}
+
+if ($comments.Count -eq 0) {
+ Write-Host "No inline-commentable findings remain after diff validation. Skipping review post." -ForegroundColor Yellow
+ exit 0
+}
+
+$reviewPayload = @{
+ commit_id = $commitSha
+ body = $summaryBody
+ event = "COMMENT" # Never APPROVE or REQUEST_CHANGES — that's a human decision
+ comments = $comments
+}
+
+$payloadJson = $reviewPayload | ConvertTo-Json -Depth 10
+
+# ============================================================================
+# DRY RUN
+# ============================================================================
+
+if ($DryRun) {
+ Write-Host ""
+ Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow
+ Write-Host " DRY RUN — Review preview (not posted)" -ForegroundColor Yellow
+ Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow
+ Write-Host ""
+ Write-Host "Summary:" -ForegroundColor Cyan
+ Write-Host $summaryBody
+ Write-Host ""
+ Write-Host "Inline comments ($($comments.Count)):" -ForegroundColor Cyan
+ foreach ($c in $comments) {
+ Write-Host " $($c.path):$($c.line)" -ForegroundColor White -NoNewline
+ Write-Host " — $($c.body.Substring(0, [Math]::Min($c.body.Length, 120)))..." -ForegroundColor Gray
+ }
+ Write-Host ""
+ Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow
+ Write-Host " Payload size: $($payloadJson.Length) chars" -ForegroundColor Gray
+ Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow
+ return
+}
+
+# ============================================================================
+# POST REVIEW
+# ============================================================================
+
+Write-Host "Posting review with $($comments.Count) inline comments..." -ForegroundColor Cyan
+
+$tempFile = [System.IO.Path]::GetTempFileName()
+try {
+ $payloadJson | Set-Content -Path $tempFile -Encoding UTF8
+
+ $result = gh api --method POST "repos/dotnet/maui/pulls/$PRNumber/reviews" --input $tempFile 2>&1
+ if ($LASTEXITCODE -ne 0) {
+ throw "Failed to post review: $result"
+ }
+
+ $reviewData = $result | ConvertFrom-Json
+ Write-Host "Review posted (ID: $($reviewData.id))" -ForegroundColor Green
+ Write-Host " $($comments.Count) inline comments at file:line" -ForegroundColor Gray
+ Write-Host " URL: $($reviewData.html_url)" -ForegroundColor Gray
+} finally {
+ Remove-Item -Path $tempFile -Force -ErrorAction SilentlyContinue
+}
diff --git a/.github/scripts/shared/Detect-TestsInDiff.ps1 b/.github/scripts/shared/Detect-TestsInDiff.ps1
index 253b98e5a867..b7a141d4e034 100644
--- a/.github/scripts/shared/Detect-TestsInDiff.ps1
+++ b/.github/scripts/shared/Detect-TestsInDiff.ps1
@@ -185,6 +185,40 @@ $IgnoredFileNames = @(
# Intermediate: collect test files grouped by type + test name
$testGroups = @{} # Key: "Type:TestName" → Value: hashtable
+# ─── Reliability fix #7: parse the actual class name from the .cs file ───
+# The previous logic derived the dotnet test filter from the *filename basename*,
+# but maui's test repo uses a "category-prefix" file-naming convention where the
+# filename includes a logical bucket dot the class name (e.g.
+# `CarouselViewUITests.ProgrammaticPositionBounceBack.cs` containing the class
+# `CarouselViewProgrammaticPositionBounceBack`). Filtering by the filename
+# yielded `--filter FullyQualifiedName~CarouselViewUITests.ProgrammaticPositionBounceBack`
+# which matched zero tests, and the gate marked the PR FAILED purely because of
+# our auto-detection mistake. Reading the actual `public class` declaration
+# from the file content is more reliable. Falls back to the filename basename
+# when the file can't be read (e.g., file deleted, path unresolvable).
+$RepoRootForRead = git rev-parse --show-toplevel 2>$null
+function Get-ClassNameFromFile {
+ param([string]$RelativePath)
+ $candidates = @($RelativePath)
+ if ($RepoRootForRead) {
+ $candidates += (Join-Path $RepoRootForRead $RelativePath)
+ }
+ foreach ($p in $candidates) {
+ if (Test-Path $p) {
+ try {
+ $content = Get-Content $p -Raw -ErrorAction Stop
+ } catch { continue }
+ # Match the first non-static, non-abstract `public class XXX` or
+ # `public partial class XXX` declaration — only concrete classes (skip
+ # `abstract`/`static`) so a base test class declared above the concrete
+ # test class isn't picked up and turned into a non-matching test filter.
+ $m = [regex]::Match($content, '(?m)^\s*public(?:\s+(?:partial|sealed))*\s+class\s+(\w+)')
+ if ($m.Success) { return $m.Groups[1].Value }
+ }
+ }
+ return $null
+}
+
foreach ($file in $ChangedFiles) {
# Skip non-code files
if ($file -notmatch "\.(cs|xaml)$") { continue }
@@ -207,7 +241,14 @@ foreach ($file in $ChangedFiles) {
# Only Shared.Tests files define actual test classes.
# HostApp files are UI pages associated with tests but aren't tests themselves.
if ($file -match "TestCases\.Shared\.Tests") {
- if ($file -match "[/\\]([^/\\]+)\.cs$") {
+ # Prefer the class name parsed from the file content over
+ # the filename basename. maui's test repo often uses a
+ # category-prefix in the filename that does NOT match the
+ # actual class name (e.g. CarouselViewUITests.X.cs → class X).
+ $parsedClass = Get-ClassNameFromFile -RelativePath $file
+ if ($parsedClass) {
+ $testName = $parsedClass
+ } elseif ($file -match "[/\\]([^/\\]+)\.cs$") {
$testName = $matches[1]
}
$filter = $testName
@@ -223,7 +264,10 @@ foreach ($file in $ChangedFiles) {
}
"XamlUnitTest" {
- if ($file -match "[/\\]([^/\\]+)\.(cs|xaml)$") {
+ $parsedClass = Get-ClassNameFromFile -RelativePath $file
+ if ($parsedClass) {
+ $testName = $parsedClass
+ } elseif ($file -match "[/\\]([^/\\]+)\.(cs|xaml)$") {
$testName = $matches[1]
$testName = $testName -replace '\.(rt|rtsg|rtxc|xaml)$', ''
}
@@ -232,7 +276,10 @@ foreach ($file in $ChangedFiles) {
}
"DeviceTest" {
- if ($file -match "[/\\]([^/\\]+)\.cs$") {
+ $parsedClass = Get-ClassNameFromFile -RelativePath $file
+ if ($parsedClass) {
+ $testName = $parsedClass
+ } elseif ($file -match "[/\\]([^/\\]+)\.cs$") {
$className = $matches[1]
# Strip platform suffix: EditorTests.iOS → EditorTests
$className = $className -replace '\.(iOS|Android|Windows|MacCatalyst)$', ''
@@ -256,7 +303,10 @@ foreach ($file in $ChangedFiles) {
}
"UnitTest" {
- if ($file -match "[/\\]([^/\\]+)\.cs$") {
+ $parsedClass = Get-ClassNameFromFile -RelativePath $file
+ if ($parsedClass) {
+ $testName = $parsedClass
+ } elseif ($file -match "[/\\]([^/\\]+)\.cs$") {
$testName = $matches[1]
}
diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md
index 3f0b145ffa04..1a8d1bb0b37b 100644
--- a/.github/skills/code-review/SKILL.md
+++ b/.github/skills/code-review/SKILL.md
@@ -2,10 +2,11 @@
name: code-review
description: >-
Deep code review of PR changes for correctness, safety, and MAUI conventions.
- Uses independence-first assessment (code before narrative) with 345 lines of
- maintainer-sourced review rules. Triggers on: "review code for PR", "code review PR",
- "analyze code changes", "check PR code quality". Do NOT use for: summarizing PRs,
- describing what changed, general PR questions, running tests, or fixing code.
+ Uses independence-first assessment (code before narrative) and delegates to the
+ maui-expert-reviewer agent for per-dimension sub-agent evaluation. Triggers on:
+ "review code for PR", "code review PR", "analyze code changes", "check PR code quality".
+ Do NOT use for: summarizing PRs, describing what changed, general PR questions,
+ running tests, or fixing code.
---
# Code Review Skill
@@ -17,7 +18,7 @@ Standalone skill that evaluates PR code changes for correctness, safety, perform
**Do NOT use for:** "what does PR #XXXXX do?", "summarize PR", "describe the changes", or any informational query — just answer those directly without invoking this skill.
> **How this differs from other skills:**
-> - **`pr-review`** — End-to-end PR workflow (3 phases: pre-flight with code review, try-fix, report; gate runs separately). Use when you want the full pipeline including test verification and fix attempts. Pre-Flight invokes this skill as a sub-agent for independence-first code analysis.
+> - **`pr-review`** — End-to-end PR workflow (4 phases: pre-flight, gate, try-fix, report). Use when you want the full pipeline including test verification and fix attempts.
> - **`pr-finalize`** — Verifies PR title/description match implementation + light code review. Use before merging.
> - **`code-review`** (this skill) — Deep code-only review with MAUI domain rules. Use when you want a thorough code analysis without running tests or modifying the PR.
@@ -71,9 +72,24 @@ Standalone skill that evaluates PR code changes for correctness, safety, perform
git log --oneline -10 --
```
-### Step 2: Load Review Rules
+### Step 2: Delegate to Expert Reviewer
-Read `.github/skills/code-review/references/review-rules.md`. These rules are distilled from real code reviews by senior MAUI maintainers across 142 high-discussion PRs.
+Delegate to the `maui-expert-reviewer` agent (`.github/agents/maui-expert-reviewer.md`) which runs per-dimension sub-agent evaluation. The agent's sole output is `inline-findings.json` — file:line comments in GitHub Review API format.
+
+**After the agent finishes:**
+
+- **If `COMMENTS_VIA_FILE=true`** (CI): Done. The pipeline calls `post-inline-review.ps1` to post findings using `GH_COMMENT_TOKEN`.
+- **If `COMMENTS_VIA_FILE` is unset** (local): Post inline findings directly:
+ ```bash
+ COMMIT_SHA=$(gh pr view $PR_NUMBER --json headRefOid --jq .headRefOid)
+ gh api repos/dotnet/maui/pulls/$PR_NUMBER/reviews \
+ --method POST \
+ --input <(jq -n \
+ --arg sha "$COMMIT_SHA" \
+ --arg body "Expert review — see inline comments." \
+ --argjson comments "$(cat CustomAgentLogsTmp/PRState/$PR_NUMBER/PRAgent/inline-findings.json)" \
+ '{commit_id: $sha, body: $body, event: "COMMENT", comments: [$comments[] | {path, line, body, side: "RIGHT"}]}')
+ ```
### Step 3: Form Independent Assessment
@@ -82,7 +98,7 @@ Based ONLY on the code (no PR description), answer:
1. **What does this change do?** Describe the behavioral change in your own words
2. **Why might it be needed?** Infer motivation from the code
3. **Is the approach sound?** Would a simpler alternative work?
-4. **What problems do you see?** Run through the review rules AND the MAUI-Specific Review Checklist below
+4. **What problems do you see?** Run through the agent's dimension CHECKs for matched dimensions
### Step 4: Read PR Narrative and Reconcile
@@ -119,33 +135,6 @@ Then deliver your verdict:
---
-## MAUI-Specific Review Rules
-
-Apply the rules in `references/review-rules.md` to each changed file. The rules are distilled from real code reviews across 142 high-discussion PRs and cover 20 categories:
-
-**Platform & Lifecycle:** Handler lifecycle, platform-specific code, Android, iOS/macCatalyst, Windows
-**Architecture:** Memory management, threading, safe area, layout, navigation, CollectionView
-**Code Quality:** Error handling, null safety, performance, XAML & bindings, API design
-**Ecosystem:** Testing, build & MSBuild, image handling, gestures, accessibility
-
-The rules file also includes a **"What NOT to Flag"** section — respect it to avoid noise.
-
----
-
-## Multi-Model Review (Optional)
-
-When the environment supports multiple models, run the review in parallel for diverse perspectives. Different model families catch different classes of issues.
-
-**How:**
-1. Select 2-3 models from different families (e.g., Claude + GPT + Gemini)
-2. Launch sub-agents in parallel, each running the full 6-step workflow above
-3. Synthesize: deduplicate findings, elevate issues flagged by multiple models
-4. Present unified review noting which findings had multi-model agreement
-
-**Timeout:** If a sub-agent hasn't completed after 5 minutes, proceed with available results.
-
----
-
## Review Output Format
**Constraints (from Android team's approach):**
@@ -194,23 +183,30 @@ When the environment supports multiple models, run the review in parallel for di
3. **Never approve what you can't verify.** If the fix touches platform code you can't fully reason about, say so explicitly and use `NEEDS_DISCUSSION`.
4. **LGTM means no ❌ Errors.** You can LGTM with 💡 Suggestions. You can LGTM with ⚠️ Warnings only if you've explained why they're acceptable.
5. **🚨 NEVER use `--approve` or `--request-changes` on GitHub.** Only post comments. Approval is a human decision.
-6. **Output to terminal only by default.** Do not post review comments to GitHub (`gh pr review --comment`) unless explicitly asked by the user or orchestrated by another agent. This matches `pr-finalize` policy.
+6. **Write findings to disk, do not post directly.** The agent does not have the GitHub comment token. Write findings to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/` — the CI pipeline or posting scripts handle GitHub interaction.
---
## Posting the Review
-After completing your review, suggest using the `Post-CodeReview.ps1` script to format and post the comment. **Do NOT post automatically** - always let the user decide when to post.
+The agent writes findings to disk. Posting is done separately by `Review-PR.ps1`:
+**Inline review comments** (preferred — findings at exact file:line):
```bash
-# Save your review to a file, then suggest:
-pwsh .github/scripts/Post-CodeReview.ps1 -PRNumber -ReviewFile /tmp/review.md -DryRun
+# Preview first:
+pwsh .github/scripts/post-inline-review.ps1 -PRNumber -DryRun
+
+# Post when ready:
+pwsh .github/scripts/post-inline-review.ps1 -PRNumber
+```
-# User can then post when ready:
-pwsh .github/scripts/Post-CodeReview.ps1 -PRNumber -ReviewFile /tmp/review.md
+**Wall-of-text summary** (phase content assembled into a single PR comment):
+```bash
+# Called by Review-PR.ps1 automatically:
+pwsh .github/scripts/post-ai-summary-comment.ps1
```
-The script wraps the review in a collapsible `
Gate output log
+Gate output log (last 60 lines)
`````` $gateLogTail @@ -655,11 +741,8 @@ $gateLogTail } } -# Post gate result by updating (or creating) the unified AI Summary comment. -# The same script is called again in STEP 3 once the review phases finish; here -# it runs early so the PR author sees the gate outcome without waiting for the -# full review. -$postGateScript = Join-Path $PSScriptRoot "post-ai-summary-comment.ps1" +# Post gate result as a separate PR comment +$postGateScript = Join-Path $PSScriptRoot "post-gate-comment.ps1" if (Test-Path $postGateScript) { try { if ($DryRun) { @@ -668,10 +751,10 @@ if (Test-Path $postGateScript) { & $postGateScript -PRNumber $PRNumber } } catch { - Write-Host " ⚠️ Failed to post gate section (non-fatal): $_" -ForegroundColor Yellow + Write-Host " ⚠️ Failed to post gate comment (non-fatal): $_" -ForegroundColor Yellow } } else { - Write-Host " ⚠️ post-ai-summary-comment.ps1 not found" -ForegroundColor Yellow + Write-Host " ⚠️ post-gate-comment.ps1 not found" -ForegroundColor Yellow } # Apply gate result label @@ -715,7 +798,49 @@ $gateStatusForPrompt = switch ($gateResult) { } $step2Prompt = @" -Use a skill to review PR #$PRNumber. +Run a multi-candidate PR review for PR #$PRNumber using the following flow. + +## Phase 1 — Pre-Flight (context only) +Use the pr-review skill's pre-flight phase to gather context. Do NOT modify code. +Write summary to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/pre-flight/content.md``. + +## Phase 2 — Candidate generation (run BOTH branches; do not skip either) +Generate the following candidates. Each candidate is an alternative diff against the PR's base branch. Do this work in isolated worktrees / scratch copies so artifacts do NOT clobber each other. + +### Branch A — Expert reviewer evaluation of the current PR fix (in sandbox) +Use the code-review skill with the maui-expert-reviewer agent to evaluate the PR's existing fix. Apply the reviewer's actionable feedback in a sandbox copy and treat the result as a candidate named ``pr-plus-reviewer``. +- Always also write the raw inline findings to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json`` (these are file:line findings against the PR's diff and feed the inline-comment posting step). +- Write candidate output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/expert-pr-eval/content.md``. + +### Branch B — Try-Fix ×4 (ALWAYS runs — do NOT skip) +Use the pr-review skill's try-fix phase to generate FOUR independent candidate fixes (``try-fix-1`` through ``try-fix-4``). Each candidate must load domain knowledge from a different maui-expert-reviewer dimension so the candidates are diverse. +- 🚨 You MUST generate all four candidates. Do not short-circuit even if Pre-Flight or the expert eval suggests the PR is already correct. +- Write each candidate's output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/try-fix-{N}/content.md`` (N = 1..4). +- Aggregate try-fix narrative for the AI summary comment to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/try-fix/content.md``. + +## Phase 3 — Report +The expert reviewer evaluates ALL candidates against each other: +- ``pr`` (the raw PR fix as submitted) +- ``pr-plus-reviewer`` (PR fix + reviewer feedback applied in sandbox) +- ``try-fix-1``..``try-fix-4`` +Pick the single winning candidate. +Write the comparative analysis to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/report/content.md``. + +## Phase 4 — Winner manifest (REQUIRED) +Write ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/winner.json`` with this exact schema: +``````json +{ + "schemaVersion": 1, + "winner": "pr" | "pr-plus-reviewer" | "try-fix-1" | "try-fix-2" | "try-fix-3" | "try-fix-4", + "isPRFix": true | false, + "summary": "1-3 sentence rationale for why this candidate won", + "candidateDiff": "Candidate diff (``$($winner.winner)``)
+ +${fence}diff +$diff +$fence + +` section, adds PR metadata (commit SHA, title), and auto-detects the verdict for a colored status dot (🟢 Approved, 🟡 Changes Suggested, 🟠 Discussion Needed).
+In CI (`eng/pipelines/ci-copilot.yml`), `Review-PR.ps1` calls both `post-inline-review.ps1` (for inline findings) and `post-ai-summary-comment.ps1` (for the wall-of-text from `{phase}/content.md` files), using `GH_COMMENT_TOKEN`.
---
diff --git a/.github/skills/code-review/references/review-rules.md b/.github/skills/code-review/references/review-rules.md
deleted file mode 100644
index 38831b6d740d..000000000000
--- a/.github/skills/code-review/references/review-rules.md
+++ /dev/null
@@ -1,345 +0,0 @@
-# .NET MAUI Review Rules
-
-Distilled from code reviews by senior maintainers of dotnet/maui.
-Covers 142 high-discussion PRs with 2,883 review comments.
-
----
-
-## 1. Handler Lifecycle & Patterns
-
-Handlers bridge the cross-platform virtual view to the native platform view.
-Lifecycle mistakes cause leaks, crashes on reconnect, and orphaned event subscriptions.
-
-| Check | What to look for |
-|-------|-----------------|
-| **ConnectHandler / DisconnectHandler symmetry** | Every listener, event handler, or callback registered in `ConnectHandler` must be unregistered in `DisconnectHandler`. Asymmetry is the #1 source of handler leaks. (PR #31022, PR #32278) |
-| **Don't null handler references eagerly in Disconnect** | The view might be removed and re-added to the window (e.g., Shell tab switching). Setting `_handler = null` in `DisconnectHandler` breaks reconnection. Prefer clearing subscriptions while keeping the weak reference alive. (PR #7886) |
-| **Assign per-view state in AttachedToWindow / DetachedFromWindow** | For Android, register layout listeners or inset listeners in `AttachedToWindow` and unregister in `DetachedFromWindow` rather than in the constructor. This avoids leaks when views are recycled. (PR #31022, PR #32278) |
-| **Mapper methods must be idempotent** | Mapper callbacks can be called at any time, not just initial setup. They must fully initialize state from scratch — don't assume defaults or rely on prior calls. (PR #15512) |
-| **Use `ModifyMapping` for Controls-layer overrides** | When adding mapper properties at the Controls level that override Core behavior, use `Mapper.ModifyMapping` so user-registered mappings aren't silently replaced. (PR #15512) |
-| **Commands vs Mapper properties** | Commands (`CommandMapper`) are for requests from cross-platform to platform code (`ScrollTo`, `Remove`). Mapper properties associate with `BindableProperty` values. Don't mix the two patterns. (PR #15512) |
-| **Base class calls ordering** | `ConnectHandler` should call `base.ConnectHandler()` before custom setup. `DisconnectHandler` should clean up before calling `base.DisconnectHandler()`. Reversed ordering can access disposed resources. (PR #32278) |
-| **Centralize listener instances via static registry** | For listeners that are per-CoordinatorLayout or per-DrawerLayout (e.g., inset listeners), prefer a static registry method over instantiating a new listener object per call. Reduces allocations and centralizes lifecycle. (PR #32278) |
-
----
-
-## 2. Memory Management & Leak Prevention
-
-iOS and Android have fundamentally different GC behaviors. iOS's reference-counting
-GC is especially sensitive to cycles. Android's Java/C# bridge creates hidden roots.
-
-| Check | What to look for |
-|-------|-----------------|
-| **Store handler references as `WeakReference`** | Platform views that communicate back to their handler must store `WeakReference` — not a strong reference. A strong handler ↔ platform view cycle prevents collection, especially on iOS. (PR #7886, PR #20124) |
-| **Prefer delegates/Funcs over handler references** | Layout code uses `Func<>` callbacks to communicate without coupling to handler instances. Adopt this pattern: the platform view stores a `Func<>` rather than an `IViewHandler` reference. (PR #7886) |
-| **Prefer static callbacks on iOS** | iOS tends to do better cleaning things up when the callback target is a static method. Move gesture recognizer callbacks and event handlers into static methods where feasible, passing state through the sender/tag. (PR #7886) |
-| **Unsubscribe and dispose Android listeners** | Android listeners that extend `Java.Lang.Object` should be unsubscribed (`view.SetOnXxxListener(null)`) in `DisconnectHandler` to remove Java's reference. Also call `_listener?.Dispose(); _listener = null;` to release the Java peer. The unsubscribe is often more important than the Dispose — if the handler is GC'd, the listener will be collected too, but Java's reference to the listener prevents that from happening. (PR #31022) |
-| **Closures capturing UIKit views are leaks** | Lambdas that close over `UIView`, `UIScrollView`, or any `NSObject` subclass create hidden strong references that the iOS GC cannot break. Extract the view access into a local, use a weak reference capture, or mark the lambda `static` to force no captured variables at compile time. (PR #13499) |
-| **NSString and other IDisposable iOS types** | Static `NSString` constants (e.g., notification names) should be declared once as `static readonly` fields, not allocated on every use. Repeated allocation of undisposed `NSString` increases memory pressure. (PR #13499) |
-| **CollectionView attached property memory impact** | Data stored on `BindableObject` via attached properties persists for the CollectionView's lifetime. For per-item or per-layout data, store it on the handler instance instead. CollectionView memory is critical. (PR #29496) |
-| **Add memory leak device tests for new subscriptions** | When adding new event subscriptions or handler references, consider adding a leak-detection device test that verifies the handler/view can be collected after disconnect. (PR #20124, PR #31022) |
-
----
-
-## 3. Safe Area (iOS/macCatalyst)
-
-Safe area is the most frequently regressed subsystem. macOS CI runs on macOS 14/15
-(~28px title bar inset), but local dev may use macOS 26+ (~0px). Fixes must pass CI.
-
-| Check | What to look for |
-|-------|-----------------|
-| **Opt-in model: only `ISafeAreaView` / `ISafeAreaView2` views** | Only views implementing `ISafeAreaView` should receive safe area adjustments. Non-safe-area views must return empty padding, not device insets. (PR #30337) |
-| **No double-padding across ancestor chain** | Before applying safe area adjustments, walk ancestors checking if any already handle the same edges. Edge-aware: parent handling `Top` does not block child handling `Bottom`. (PR #30337, PR #15512) |
-| **Raw vs adjusted inset comparison** | `_safeArea` is filtered by `GetSafeAreaForEdge` (zeros edges per `SafeAreaRegions`); raw `UIView.SafeAreaInsets` includes all edges. Never compare them — compare raw-to-raw or adjusted-to-adjusted. (PR #30337) |
-| **Never gate view callbacks on window-level insets** | Comparing `Window.SafeAreaInsets` to filter per-view `SafeAreaInsetsDidChange` callbacks blocks legitimate updates. On macCatalyst with custom TitleBar, the view's insets change without the window's changing, causing a 28px shift on CI. (PR #30337) |
-| **Use constants for magic strings** | Property names like `"SafeAreaInsets"` used in mapper lookups must be constants (e.g., `PlatformConfiguration.iOSSpecific.Page.SafeAreaInsetsProperty.PropertyName`). Bare strings will break silently if renamed. (PR #15512) |
-| **New safe area types belong in `src/Core`** | Types like `SafeAreaRegions` must be in `Core.csproj` so that `ISafeAreaView2` can reference them. Don't add core interface dependencies to `Controls.csproj`. (PR #30337) |
-| **Don't use ISafeAreaView2 if ISafeAreaView can be extended** | Before creating a versioned interface like `ISafeAreaView2`, check if the existing interface can be extended with default interface methods or deprecated members. Prefer extending over versioning. (PR #30337 — StephaneDelcroix) |
-
----
-
-## 4. Layout & Measure
-
-Layout is the hot path. Mistakes cause infinite measure loops, incorrect sizing,
-and subtle clipping issues that only manifest on specific device/OS combinations.
-
-| Check | What to look for |
-|-------|-----------------|
-| **Propagate changes through virtual view, not INCC** | Collection changes (e.g., map elements, picker items) should propagate via `Handler.UpdateValue(PropertyName)` at the Controls level, not via `INotifyCollectionChanged` from the platform view. INCC creates tight coupling. (PR #7886) |
-| **Guard against infinite ContentSize oscillation** | On iOS, `MauiScrollView` can enter infinite loops when content with very large `ContentSize` triggers layout → resize → layout cycles. Add a guard (e.g., `EqualsAtPixelLevel` comparison) to break sub-pixel oscillation caused by animation noise. (PR #26629, PR #30337) |
-| **Compare sizes at pixel resolution** | Safe area and content size comparisons must account for device-pixel rounding. Use `EqualsAtPixelLevel` (threshold ~0.0000001pt) to prevent oscillation during `TranslateToAsync` animations. (PR #30337) |
-| **Don't apply padding in aspect ratio calculations** | When computing image aspect ratios on iOS, include padding for sizing but not for the aspect ratio computation itself. Padding should be added after the aspect ratio is applied. (PR #26629) |
-| **Matrix test nesting for visibility propagation** | When testing `IsVisible` propagation through nested containers, test the full matrix: 2-level and 3-level nesting, set/unset sequences. A single level test misses propagation bugs. (PR #20154 — mattleibow) |
-
----
-
-## 5. Platform-Specific Code
-
-| Check | What to look for |
-|-------|-----------------|
-| **File extensions determine compilation targets** | `.android.cs` → Android only. `.ios.cs` → iOS AND MacCatalyst. `.maccatalyst.cs` → MacCatalyst only. `.windows.cs` → Windows only. Both `.ios.cs` and `.maccatalyst.cs` compile for MacCatalyst — check for duplicate definitions. (PR #24396) |
-| **Use type aliases for namespace collisions** | `View`, `Color`, `Context` exist in both MAUI and Android/iOS/Windows namespaces. Use `using AView = Android.Views.View;`, `using NativeScrollView = Microsoft.UI.Xaml.Controls.ScrollViewer;`, etc. Prevents subtle misresolution bugs. (PR #3351 — mattleibow) |
-| **Platform views must live in `Microsoft.Maui.Platform` namespace** | Custom native view wrappers (e.g., `MauiHybridWebView`, `MauiHybridWebViewClient`) must be in `Platform//` with `namespace Microsoft.Maui.Platform`. Don't put them in handler directories. (PR #22880 — mattleibow) |
-| **Don't change handler generic type parameters** | Changing the generic type on a handler (e.g., `ViewHandler` → `ViewHandler`) is a binary breaking API change. Leave existing generics as-is. (PR #13499 — PureWeen) |
-| **Align cross-platform behavior** | When fixing behavior on one platform, verify the behavior is consistent on others. A flyout state fix on iOS should match Windows behavior. Reviewers will ask: "What does Windows/Android do here?" (PR #26701 — jsuarezruiz) |
-| **Reuse platform APIs via extension methods** | If a platform already has a method (e.g., `FileSystemPlatformOpenAppPackageFile`), use it. Don't duplicate file-opening logic in handler-specific code. (PR #22880 — mattleibow) |
-
----
-
-## 6. Android Platform
-
-| Check | What to look for |
-|-------|-----------------|
-| **Store `Context` in a local before repeated use** | Accessing Android's `Context` property calls into Java each time (JNI marshaling). Store it in a local: `var context = Context;` then use the local. Same applies to any property that crosses the Java/C# bridge. (PR #26789 — jonathanpeppers) |
-| **Android resource files need correct build action** | Files used in Android tests must be declared with the correct build action (e.g., `AndroidResource`, `EmbeddedResource`). A file at `/red.png` with no build action causes `FileNotFoundException` on Android only. (PR #14109 — jonathanpeppers) |
-| **Use `PlatformLogger` for Android native code** | In Java code under `src/Core/AndroidNative/`, use `PlatformLogger` for logging — not `android.util.Log` directly. This ensures consistent log formatting. (PR #29780 — jonathanpeppers) |
-| **Use `FragmentActivity` carefully with Glide** | When resolving Glide request managers, check Glide's own source for the correct `Activity` lookup pattern. Don't add unnecessary `FragmentActivity` checks that diverge from Glide's implementation. (PR #29780 — jonathanpeppers) |
-| **Tests in Java won't run without CI changes** | New Java test projects require AzDO/yaml/CI pipeline updates. If CI changes aren't included, the tests won't execute. Write device tests in C# instead. (PR #29780 — jonathanpeppers) |
-
----
-
-## 7. iOS/macCatalyst Platform
-
-| Check | What to look for |
-|-------|-----------------|
-| **iOS has trouble collecting objects with handler references** | When an iOS platform view holds a reference to its handler, the reference-counting GC often cannot break the cycle. Use delegates, `Func<>`, or `WeakReference` patterns. See `DatePickerHandler.ios.cs` (`DatePickerDelegate`) for the recommended proxy pattern. (PR #7886 — PureWeen) |
-| **Subscribe/unsubscribe in `MovedToWindow`** | For iOS callbacks that need handler access, subscribe when the view moves to a window and unsubscribe when removed. This is more reliable than constructor/dispose for iOS lifecycle. (PR #7886 — PureWeen) |
-| **Store notification `UserInfo` before repeated access** | Accessing `notification.UserInfo` multiple times with null-conditional (`?.`) is wasteful and obscures null checks. Store `var userInfo = notification.UserInfo;`, null-check once, then use the methods. (PR #13499 — mandel-macaque) |
-| **Check `Handle == IntPtr.Zero` for disposed native objects** | A `UICollectionView` or other native object may be disposed but not null. Check `collectionView?.Handle == IntPtr.Zero` as an additional guard against accessing disposed objects. (PR #29397 — jsuarezruiz) |
-| **Wrap CollectionView layout callbacks in try-catch** | Layout factory callbacks can fire after disposal. Wrap in `catch (Exception ex) when (ex is ObjectDisposedException or InvalidOperationException)` to prevent crashes. (PR #29397 — jsuarezruiz) |
-| **`UIImage.FromImage` creates a copy** | Calling `UIImage.FromImage` allocates a new instance. If you only need to transform the existing image, modify in place when possible to avoid double memory usage. (PR #26016 — jsuarezruiz) |
-| **Use `IUITextInput` interface for cursor/text range** | When accessing `SelectedTextRange` or similar text input properties across `UITextField`/`UITextView`, use the `IUITextInput` interface to avoid duplicating code for each concrete type. (PR #13499 — mandel-macaque) |
-| **macCatalyst defaults `UseSafeArea` to `true`** | Unlike iOS where `UseSafeArea` defaults to `false`, macCatalyst defaults to `true`. Code that assumes `false` across Apple platforms will misbehave on macCatalyst. (PR #30337) |
-
----
-
-## 8. Windows Platform
-
-| Check | What to look for |
-|-------|-----------------|
-| **Appium on WinUI can't retrieve non-accessible elements** | `BoxView` and other elements without text aren't visible to the WinUI accessibility tree. Use `Label` or elements with text content for Appium-based UI test assertions on Windows. (PR #19371 — PureWeen) |
-| **Check null/empty collections before `.FirstOrDefault()`** | On Windows, calling `.FirstOrDefault().ToPlatform()` on an empty (but non-null) `Accelerators` collection will throw. Add an empty-collection guard. (PR #14740 — mattleibow) |
-| **WebView2 assembly identity differs between WinUI and WPF** | The WebView2 types have different assembly identities across WinUI, WPF, and WinForms. You cannot directly share WebView2 helper code between these targets. (PR #654 — Eilon) |
-| **Prefix new MSBuild properties with `Maui`** | Any new MSBuild property (e.g., `EnableXamlLoading`) must be prefixed with `Maui` (e.g., `MauiEnableXamlLoading`) to avoid collisions with WindowsAppSdk or other framework properties. (PR #19310 — jonathanpeppers) |
-| **Apply theme per window, not to all windows** | In `OnWindowHandlerChanged`, don't call `ApplyThemeToAllWindows()` — it iterates all windows redundantly (N+N-1+...+1 total applications for N windows). Apply theme only to the specific window from the sender. (PR #31714 — jsuarezruiz) |
-| **Track applied state to avoid redundant work** | Theme and style application should track whether it's already been applied per window (e.g., using a `Dictionary`). Re-applying on every handler change without checking if the theme actually changed is a performance hit. (PR #31714 — jsuarezruiz) |
-
----
-
-## 9. Navigation & Shell
-
-| Check | What to look for |
-|-------|-----------------|
-| **Shell re-adds views on tab switch** | Shell removes and re-adds platform views when users switch tabs or flyouts. Code that nullifies state in `DisconnectHandler` or `RemovedFromSuperview` will break on re-navigation. (PR #7886 — PureWeen) |
-| **Test flyout state after window resize AND rotation** | Flyout visibility bugs often differ between maximize/restore and rotate-to-landscape/rotate-back scenarios. Create separate tests for each rather than combining into a single test method. (PR #26701 — PureWeen) |
-| **Split unrelated behaviors into separate tests** | When a test covers two different behaviors (e.g., flyout-after-maximize AND flyout-after-rotation), split into separate test methods. Combined tests obscure which scenario fails. (PR #26701 — PureWeen) |
-| **Security: validate WebView URL mapping** | When loading local HTML content in `WebView`, the local file mapping hostname should be random or scoped to prevent cross-origin file access. An app calling `LoadHtml` with `baseUrl: null` could inadvertently expose local files. (PR #7672 — Eilon) |
-| **More tab behavior is platform-specific** | iOS's "More" tab for overflow Shell items may not have a standard Apple-defined behavior. Verify against Apple HIG before assuming push navigation from the More tab is correct. (PR #26292 — mattleibow) |
-
----
-
-## 10. CollectionView
-
-Two handler implementations exist. Items/ is the only implementation for Android
-and Windows. Items2/ is the current handler for iOS/MacCatalyst (Items/ iOS is deprecated).
-
-| Check | What to look for |
-|-------|-----------------|
-| **Verify correct handler directory for platform** | Android/Windows changes go in `Handlers/Items/`. iOS/MacCatalyst changes go in `Handlers/Items2/`. Items2/ has NO Android or Windows code. Modifying the wrong directory is a no-op on the target platform. (PR #29397, PR #31336) |
-| **Don't copy already-marshaled ObjC arrays** | iOS binding APIs like `indexPathsForVisibleItems` already copy the ObjC array to managed code via `CFArray.ArrayFromHandle`. Creating a second copy (e.g., `.ToArray()`) is wasteful. (PR #31336 — jonathanpeppers) |
-| **MeasureFirstItem cache must distinguish item types** | When `MeasureFirstItem` is used with grouped CollectionView, the GroupHeader size gets cached and applied to all items if the cache key doesn't distinguish between headers and data items. Causes clipping. (PR #27847 — PureWeen) |
-| **CollectionView inside ScrollView causes infinite loops** | Placing a CollectionView inside a ScrollView (which users shouldn't do, but do) can cause infinite layout loops on iOS due to unbounded `ContentSize`. Add guards. (PR #26629 — rmarinho) |
-| **Include gallery samples alongside tests** | When fixing EmptyView, DataTemplateSelector, or other complex CollectionView features, add samples to `Controls.Sample/Pages/Controls/CollectionViewGalleries/` for manual validation alongside automated tests. (PR #25418 — jsuarezruiz) |
-| **Validate item class names are unique** | ViewModels inside test files (e.g., `CollectionViewViewModel`) must be namespaced uniquely (e.g., `Issue29130ViewModel`). Duplicate class names across issues cause build errors. (PR #29496 — jsuarezruiz) |
-
----
-
-## 11. Threading & Async
-
-| Check | What to look for |
-|-------|-----------------|
-| **Use `Interlocked.Increment` for counters accessed from UI thread** | Even counters "most likely" called on the UI thread should use `Interlocked` for safety. A debounce counter without thread safety can miss update requests. (PR #13499 — PureWeen) |
-| **Don't reset debounce counters — roll them over** | Resetting a debounce count to 0 can cause race conditions where an update request is missed. Roll the counter over at a high threshold (e.g., 100) instead. (PR #13499 — PureWeen) |
-| **Check if already on main thread before dispatching** | Before calling `MainThread.BeginInvokeOnMainThread` or `Dispatcher.Dispatch`, check if you're already on the main thread. Unnecessary dispatch adds latency and can reorder operations. (PR #26153 — PureWeen) |
-| **`Task.Delay` in tests needs justification** | A `Task.Delay(100)` in a device test is a flaky test waiting to happen. Reviewers will ask: "How do we know 100ms is enough?" Prefer `WaitForMainThread()` or similar deterministic helpers. (PR #14619 — jonathanpeppers) |
-| **Create helper methods for common async test patterns** | Repeated `await InvokeOnMainThreadAsync(() => { })` should be extracted into a `WaitForMainThread()` helper. Duplicate async boilerplate is error-prone. (PR #14619 — jonathanpeppers) |
-| **Check `DispatcherQueue` for null before posting** | On Windows, `DispatcherQueue` can be null if the Window is disposed. Guard dispatch calls with a null check. (PR #31714 — jsuarezruiz) |
-
----
-
-## 12. XAML & Bindings
-
-| Check | What to look for |
-|-------|-----------------|
-| **Compiled bindings require explicit `x:DataType`** | Every `{Binding}` with an explicit `Source` must have an explicit `x:DataType` on the binding or a parent element. Missing `x:DataType` prevents source generation and causes runtime reflection fallback. (PR #32444 — simonrozsival) |
-| **Set `x:DataType` on root element for page-level BindingContext** | When a page sets `BindingContext` in code-behind, the root `ContentPage` element needs `x:DataType="local:MyViewModel"` to enable compiled bindings for all child elements. (PR #32444 — simonrozsival) |
-| **Don't subscribe on every BindingContext change** | In `TemplatedCell` and similar reusable views, subscribing to events on BindingContext change without checking for diff will accumulate multiple subscriptions. Only subscribe if the BindingContext actually changed. (PR #14619 — rmarinho) |
-| **Use `SetValue(BP, ...)` when BindableProperty exists** | When a source generator heuristic determines a BindableProperty will be generated, the generated code must use `.SetValue(BP, ...)` instead of calling the property setter directly. (PR #32597 — simonrozsival) |
-| **Remove dead code from source generation** | When source-gen skips properties via `SkipProperties`, the `CreateValuesVisitor` should also skip `new` instantiations for those properties. Leftover dead code confuses reviewers and wastes binary size. (PR #32474 — simonrozsival) |
-| **Future-proof message types with records** | When adding a new cross-platform messaging API (e.g., `SendRawMessage`), pass a `record` type instead of a bare `string`. This allows adding fields later without breaking the API. (PR #22880 — mattleibow) |
-| **Markup extension recognition should be semantic** | Don't just check suffix (`FooExtension`); query the compilation for types implementing `IMarkupExtension`. Also support prefixed extensions (`{local:MyExtension}`). (PR #33693 — simonrozsival) |
-| **Auto-escape XML-unfriendly characters in expressions** | Users should not need to type `>` in XAML C# expressions. Use `` or automatic escaping for `<`, `>`, `&&`, `||` in expression contexts. (PR #33693 — simonrozsival) |
-
----
-
-## 13. Testing
-
-| Check | What to look for |
-|-------|-----------------|
-| **UI tests must run on all applicable platforms** | Do not restrict tests to a single platform unless there is a specific technical limitation. Tests should always cover iOS, Android, and Windows unless the fix is platform-specific. (PR #32064 — PureWeen) |
-| **Snapshot baselines must be updated across all platforms** | Changing background color, font, or layout requires updating snapshot baselines for Android, iOS, MacCatalyst, and Windows. Stale baselines cause false failures. (PR #25129, PR #27399 — jsuarezruiz) |
-| **Screenshot size must match capture method** | If the screenshot capture mechanism changes (e.g., different DPI or crop), all baselines need regeneration. A size mismatch (e.g., 1920×1080 vs 789×563) means the capture changed, not the rendering. (PR #25129 — jsuarezruiz) |
-| **Use `VerifyScreenshot(retryTimeout:)` instead of `Task.Delay`** | `VerifyScreenshot` has built-in retry logic. Adding `Task.Delay` before it is redundant. Use `retryTimeout: TimeSpan.FromSeconds(2)` for animations. (PR #32064) |
-| **Make test labels visible even if content is clipped** | When testing clipping or canvas rendering, position a label at the edge of the drawn content so screenshots prove content was actually drawn, not just that the clip removed everything. (PR #28353 — mattleibow) |
-| **Memory consumption tests on Android** | Use Appium's `GetMemoryInfo()` helper to validate memory consumption in tests. Set a threshold and assert memory stays below it to detect regressions. (PR #26789 — jsuarezruiz) |
-| **Tests that verify two behaviors need two methods** | A test called `ShouldFlyoutBeVisibleAfterMaximizingWindow` that also tests rotation is actually two tests. Split them so failures pinpoint the exact regression. (PR #26701 — PureWeen) |
-| **Test types should match test project infrastructure** | Source generator tests belong in `SourceGen.UnitTests.csproj`, not `Xaml.UnitTests.csproj`. Tests that don't need `[Values] XamlInflator` shouldn't use it — running 3 identical iterations wastes CI time. (PR #32474 — simonrozsival) |
-
----
-
-## 14. Performance
-
-| Check | What to look for |
-|-------|-----------------|
-| **Cache JNI property access in locals** | Android properties like `Context`, `Resources`, `ContentDescription` cross the Java/C# bridge on every access. Store in a local variable when used more than once in a method. (PR #26789 — jonathanpeppers) |
-| **Avoid closures that increase GC pressure** | Closures capturing UIKit objects create extra GC-tracked objects with references to captured elements. Even if technically correct, this increases memory pressure. Prefer explicit parameter passing or static methods. (PR #13499 — mandel-macaque) |
-| **`ValueTuple.GetHashCode()` may allocate** | `ValueTuple.GetHashCode()` for large tuples can allocate. For hash-code–critical types like `SetterSpecificity`, implement `GetHashCode()` explicitly to avoid boxing and allocation. (PR #13818 — jonathanpeppers) |
-| **Prefer `IReadOnlyList` on public APIs** | Public methods should return `IReadOnlyList` or `IReadOnlyCollection` instead of mutable `List`. This communicates intent and prevents callers from corrupting internal state. (PR #14740 — mattleibow) |
-| **Avoid double-copy of managed arrays from ObjC** | iOS binding APIs already marshal ObjC arrays into C# collections. Calling `.ToArray()` or `.ToList()` on the result creates a redundant copy. (PR #31336 — jonathanpeppers) |
-| **Use `StringBuilder` or `List` for code generation** | In source generators, building output strings with repeated `+=` is O(n²). Use `StringBuilder`, `List` with `string.Join`, or append patterns that avoid intermediate allocations. (PR #32420 — simonrozsival) |
-| **Allocate early, check cheap conditions first** | In validation chains, test simple boolean flags and null checks before allocating strings or doing I/O. This is especially important in mapper callbacks which run frequently. (PR #28077 — jsuarezruiz) |
-| **Don't remove caches without benchmarks** | If a cache (like `SetterSpecificityList`) was added with measured performance wins, removing or replacing it requires proving the alternative provides equivalent or better performance. Use `dotnet-trace` and `speedscope.app`. (PR #17756 — jonathanpeppers) |
-| **Flatten nested LINQ into single-level iteration** | Nested `foreach` + `SelectMany` over chained mappers can be flattened: `foreach (var key in Chained.Reverse().SelectMany(c => c.GetKeys()))`. But always benchmark to verify improvement. (PR #28077 — jsuarezruiz) |
-
----
-
-## 15. Error Handling & Null Safety
-
-| Check | What to look for |
-|-------|-----------------|
-| **Check if stream is seekable before copying** | When processing image or data streams, check `stream.CanSeek` first. If seekable, use the original stream directly instead of copying to a `MemoryStream`. (PR #29769 — jsuarezruiz) |
-| **Pattern match instead of cast + null check** | Prefer `if (localCursor is CGRect rect)` over `as` + null check. Pattern matching is safer with value types (where `as` doesn't compile) and more readable. (PR #13499 — tj-devel709) |
-| **Consistent null initialization** | Don't initialize nullable reference variables to `null` in some places but not others. Be consistent — either always set `= null` or never set it (let the default apply). (PR #13499 — mandel-macaque) |
-| **Use `?.` chains judiciously** | Multiple `?.` on the same expression (e.g., `notification.UserInfo?.ObjectForKey(...)?.ToString()`) is harder to debug. Store intermediates and null-check explicitly when the chain is more than 2 deep. (PR #13499 — mandel-macaque) |
-| **Fallback when `MauiContext` is null** | If `Window.MauiContext` is null, fall back to `Application.Current.FindMauiContext()` rather than letting a `NullReferenceException` propagate. (PR #24808 — jsuarezruiz) |
-| **Initialize WebView cookies in `CoreWebView2Initialized`** | Cookie preloading that runs before `CoreWebView2` is initialized hits null references. Wire initialization into the `CoreWebView2Initialized` event. (PR #24846 — PureWeen) |
-| **Try-catch for fire-and-forget platform calls** | Platform calls that can fail but whose failures are non-critical should be wrapped in `try/catch` or use `FireAndForget`. Unhandled exceptions in these paths crash the app. (PR #22880 — PureWeen) |
-| **Guard against empty collections in chained calls** | `collection?.FirstOrDefault().ToPlatform()` throws if the collection is empty (not null). Check `.Any()` or use null-conditional on the result of `FirstOrDefault()`. (PR #14740 — mattleibow) |
-
----
-
-## 16. API Design & Public API
-
-| Check | What to look for |
-|-------|-----------------|
-| **Adding to an interface is a breaking change** | Adding members to a public interface (e.g., `IMenuElement`) is a binary breaking change. Use default interface methods (verify platform support), create a new interface (`IMenuElement2`), or add an extension method. (PR #14740 — PureWeen) |
-| **Never modify `PublicAPI.Shipped.txt`** | To remove a shipped API, copy the line to `PublicAPI.Unshipped.txt` prefixed with `*REMOVED*`. Never edit the Shipped file directly. (PR #14740 — mattleibow) |
-| **Don't expose setters that do nothing** | If a property setter is not wired to any handler or has no observable effect, remove it. Dead setters confuse users and accumulate API surface that can't be removed later. (PR #14740 — PureWeen) |
-| **Obsolete with proper message format** | Obsolete attributes need a period at the end of the message. iOS Cecil tests enforce this convention. (PR #13499 — mandel-macaque) |
-| **Public `MauiAppBuilder` extension methods require careful review** | Once added to the builder pattern, extension methods cannot be removed. Establish a process for evaluating what belongs on the builder vs what library authors should register themselves. (PR #2137 — mattleibow) |
-| **Avoid breaking Xamarin.Forms migration** | When replacing types (e.g., `Position` → `Location`), consider keeping a compatibility class that inherits from the new type. This eases porting from Xamarin.Forms. (PR #7886 — jsuarezruiz) |
-| **Use named constants instead of magic numbers** | Bit-shift offsets, specificity values, and layout thresholds should be `const int` with descriptive names. `new SetterSpecificity(100, 0, 0, 0)` is opaque; `SetterSpecificity.Implicit` communicates intent. (PR #13818 — jonathanpeppers) |
-
----
-
-## 17. Image Handling
-
-| Check | What to look for |
-|-------|-----------------|
-| **Image source services must return native image representations** | `IImageSourceService.GetDrawableAsync` should return a result that represents the actual image data, not just a status. This enables usage without a view (e.g., notification icons). (PR #2360 — mattleibow) |
-| **Verify Glide callback thread assumptions** | Glide callbacks like `onResourceReady` are assumed to be on the main thread because they call `view.setImageDrawable`. But this isn't documented — verify by checking Glide's source. (PR #14109 — jonathanpeppers) |
-| **Clean up bitmaps on overlay add/remove cycles** | If an overlay is removed and re-added, resources loaded during the first add must be either cached or reloadable. Disposal without a reload path causes blank overlays. (PR #3351 — mattleibow) |
-| **Inner vs outer corner radius for clipping** | When using a `RoundRectangle` for clipping (e.g., inside a `Border`), the corner radius for the inner clip is different from the outer border radius. Use the inner value. Clipping with the outer value leaves visible gaps. (PR #10964 — jsuarezruiz) |
-
----
-
-## 18. Gestures
-
-| Check | What to look for |
-|-------|-----------------|
-| **Use `Tap` over `Click` for mobile platforms** | In UI tests, `Tap` is more flexible and mobile-appropriate. `Click` may not be converted to `Tap` on all platforms, causing test failures on Android/iOS while passing on Windows. (PR #19371 — PureWeen) |
-| **Ensure gesture tests verify their precondition** | A test that verifies "GetPosition returns correct coordinates" must confirm the element was actually tapped. If the element isn't tappable (e.g., iOS BoxView issue), the test passes trivially. Use a verifiable side effect. (PR #19371 — PureWeen) |
-| **Span tap region calculation across multiple lines** | The `CGRect` region for each `Span` in a `FormattedString` is incorrectly calculated when text wraps to multiple lines (inherited from Xamarin.Forms). Verify tap targets with multi-line text. (PR #15544 — jsuarezruiz) |
-| **EventArgs naming convention** | Custom event args should follow `{Event}EventArgs : EventArgs` naming. E.g., if the event is `Touch`, the args class should be `TouchEventArgs`, not `DiagnosticsTouchArgs`. (PR #3351 — mattleibow) |
-
----
-
-## 19. Build & MSBuild
-
-| Check | What to look for |
-|-------|-----------------|
-| **Use `dotnet-public` feed to avoid dependency confusion** | NuGet sources must use the `dotnet-public` feed. Adding arbitrary third-party feeds creates a dependency confusion attack surface. New package sources require approval. (PR #5020 — jonathanpeppers) |
-| **Build tasks can't depend on optional packages** | `Controls.Build.Tasks.csproj` cannot reference optional NuGet packages like Maps. Build tasks must only depend on core assemblies that are always present. (PR #7886 — mattleibow) |
-| **Feature flag properties belong in `src/Core`** | Feature flags (e.g., `IsXamlLoadingSupported`) should be in `Core.csproj` / `Microsoft.Maui.dll` since all assemblies depend on it. Don't scatter feature flags across Controls or platform-specific assemblies. (PR #19310 — jonathanpeppers) |
-| **Follow naming conventions: `Support` suffix → `IsXSupported`** | MSBuild property: `XamlLoadingSupport`. C# property: `IsXamlLoadingSupported`. This matches the runtime feature switch pattern used across .NET. (PR #19310 — simonrozsival) |
-| **Document feature switch breakage and alternatives** | For each new feature switch, document: (1) which APIs break when disabled, (2) code examples of what users should do instead. (PR #19310 — jonathanpeppers) |
-| **Targets in NuGet vs workload have different import timing** | Moving MSBuild targets from workload to NuGet changes when they're imported relative to package restore. Test the sequence: install VS → new project → NuGet restore → build. (PR #11206 — jonathanpeppers) |
-| **Never commit `cgmanifest.json` or `templatestrings.json`** | These are auto-generated during CI. AI-generated PRs frequently include changes to these files. Always reset them before committing. (PR #11206) |
-
----
-
-## 20. Accessibility
-
-| Check | What to look for |
-|-------|-----------------|
-| **Don't disable font scaling globally** | An implicit style that disables font scaling across the app makes it inaccessible to partially-sighted users. "Rather have an ugly app that a partially blind person can use instead of a beautiful one they can't." (PR #1774 — PureWeen) |
-| **WinUI Appium requires accessible elements** | Elements without text content (e.g., `BoxView`) are invisible to Appium on WinUI. Use `Label` or set `AutomationProperties.Name` for elements that need to be located in Windows UI tests. (PR #19371 — PureWeen) |
-| **Test accessibility properties propagate** | When a control exposes `AutomationProperties`, verify the native accessibility tree reflects them. A broken binding silently removes accessibility. (PR #25011 — jsuarezruiz) |
-
----
-
-## 21. Regression Prevention (Lessons from Reverts & Candidate Failures)
-
-Rules distilled from 30 reverted PRs and 50 candidate-branch failures. When a PR touches these areas, apply extra scrutiny.
-
-| Check | What to look for |
-|-------|-----------------|
-| **CollectionView changes need broad scenario coverage** | CV is the single highest-regression component (15 candidate failures). Any change to layout, scroll, spacing, cell alignment, Header/Footer, or `KeepLastItemInView` must be tested across all four: empty collection, single item, many items, and with grouping. A fix for one layout scenario routinely breaks another. (CollectionView candidate failures, multiple PRs) |
-| **Style/theme changes have cascading effects** | `ApplyToDerivedTypes`, implicit styles, and `AppThemeBinding` interact in non-obvious ways. A fix to one style propagation path often breaks another — this pattern caused two separate reverts for PR #9648 and broke source gen in PR #32728. When touching style resolution or `AppThemeColor`, test: explicit style, implicit style, derived-type style, and dark/light theme switching. |
-| **Test the fix scenario AND adjacent scenarios** | Most reverts happen because the fix works for the reported issue but breaks a neighboring case. ToolbarItem image fix (PR #28833, reverted twice) fixed one image mode while breaking others. Entry `SelectionLength` (PR #26213) fixed selection but broke focus. Require authors to enumerate what adjacent behaviors they checked. |
-| **Never remove `InternalsVisibleTo` without auditing NuGet consumers** | IVT removal looks like cleanup but silently breaks community packages that depend on internal APIs. At least one revert went all the way back to a release branch because of this. Flag any `InternalsVisibleTo` deletion and ask: "Have you checked whether any published NuGet consumers reference this?" |
-| **ToolbarItem/Image changes must be tested across all image source types** | Font images, file images, and tinted images each go through different code paths. PR #28833 (reverted twice) and PR #26048 show this pattern. When modifying `ImageSourceExtensions` or any ToolbarItem rendering code, verify all three modes: `FontImageSource`, `FileImageSource`, and `BitmapImageSource` with tinting. |
-| **Entry/Editor focus and selection state is fragile** | `CursorPosition`, `SelectionLength`, keyboard show/hide, and focus order interact tightly on every platform. PR #26213 shows how a `SelectionLength` change broke Entry focus. Flag any handler change that touches these properties and ask whether focus behavior was verified after the change — especially when the keyboard is dismissed and re-shown. |
-| **Measurement timing on iOS lags behind property changes** | `UIButton.TitleLabel.Bounds` and `UIView` frame values are not updated synchronously after a property change — the layout pass hasn't run yet. PR #25122 (tj-devel709) shows the correct pattern: measure the title manually instead of reading `.Bounds`. Flag any iOS handler code that reads frame/bounds immediately after setting a property, without deferring to the next layout pass. |
-| **Template changes need all-template validation** | A template fix for MAUI app can break Blazor hybrid or multi-project templates. `MapStaticAssets` vs `UseStaticFiles` (candidate failure) shows how a single API swap breaks one template variant silently. Any `src/Templates/` change must be validated against all template IDs: `maui`, `maui-blazor`, `maui-blazor-web`, `mauilib`, `maui-multiproject`. |
-| **Candidate-branch PRs must not mix concerns** | PureWeen's explicit rule (PR #33838): candidate fixes should not bundle unrelated flakiness fixes. A PR that mixes "fix the regression" with "also fix this flaky test" makes bisection impossible and obscures what actually fixed the candidate failure. Flag mixed-concern candidate PRs. |
-| **Screenshot tests must prove content was drawn, not just that clipping worked** | mattleibow on PR #28353: "give this label a negative margin so that we can see that the image is drawn at all." A clipping test that passes when the view renders nothing is not a useful test. Flag screenshot tests for `GraphicsView`, `Image`, or any drawing surface that don't verify the content region itself — use a visible element inside the clip boundary as a sentinel. |
-| **Arithmetic in index/position calculations needs explicit parentheses** | mattleibow on PR #23369: "This line of code is a bit ambiguous for a quick read." Silent operator-precedence bugs in scroll offset, index math, or spacing calculations are hard to spot and have caused gesture/tap regressions. Flag expressions like `a + b * c` or `offset - padding / 2` without explicit parentheses when they appear in position or size computations. |
-| **Major dependency upgrades need broad platform validation** | WindowsAppSDK upgrade (PR #32174) was reverted because it broke too many things simultaneously. Flag PRs that bump `WindowsAppSDK`, `Microsoft.Maui.*` NuGet versions, or other platform SDK dependencies, and ask whether CI was green on all platforms (Android, iOS, MacCatalyst, Windows) before merge, not just the changed platform. |
-| **ContentPresenter BindingContext propagation breaks explicit TemplateBindings** | Propagating `BindingContext` through `ContentPresenter` overwrites `{TemplateBinding}` values that were set explicitly. This was a reverted PR. Flag any handler or renderer change that sets or propagates `BindingContext` on a `ContentPresenter` or control template root without verifying that `TemplateBinding` expressions still resolve correctly. |
-
----
-
-## 22. Most Frequently Regressed Components
-
-Components ranked by regression frequency from 366 analyzed PRs (reverts + candidate fixes + regression fixes):
-
-| Component | Regressions | Key Risk Areas |
-|-----------|-------------|----------------|
-| CollectionView | 15 | Layout, scroll position, spacing, cell alignment, Header/Footer |
-| Image/Graphics | 15 | Aspect ratio, CornerRadius, Background, DrawString |
-| Theme/Style | 8 | AppThemeBinding, implicit styles, ApplyToDerivedTypes |
-| CarouselView | 7 | ScrollTo, CurrentItem, ItemSpacing, loop mode |
-| Gesture/Tap | 7 | TapGestureRecognizer, SwipeView, outside-tap dismiss |
-| Button/Entry | 7 | Dynamic resize, focus/selection, AppThemeBinding colors |
-| Toolbar | 5 | Icon color, back button, BarTextColor across modes |
-| Shell/TabBar | 4 | TabBarIsVisible, Shell crashes, section rendering |
-
-Use this table as a triage guide: PRs touching these components warrant a more thorough pass through sections 1–21 above, with particular attention to the adjacent-scenario rule (§21 row 3) and the component-specific rows in this section.
-
----
-
-## What NOT to Flag
-
-Do not waste reviewer time on these:
-
-| Category | Why |
-|----------|-----|
-| **Style/formatting** | CI enforces via `dotnet format`. |
-| **Missing XML docs on non-public APIs** | Not required by MAUI convention. |
-| **Test naming preferences** | Unless names are genuinely misleading. |
-| **`var` vs explicit types** | Project allows both; consistency within a file is sufficient. |
-| **Micro-optimizations in cold paths** | Readability wins unless profiling proves it's a hot path. |
-| **Single-use LINQ vs foreach** | Either is fine; don't bikeshed. |
-| **Comment style** | Only flag if a comment is factually wrong or stale. |
-| **PR commit count/squash** | That's the author's workflow choice. |
diff --git a/.github/skills/code-review/tests/eval.yaml b/.github/skills/code-review/tests/eval.yaml
index 8e0d8e981045..f02d9d1ad109 100644
--- a/.github/skills/code-review/tests/eval.yaml
+++ b/.github/skills/code-review/tests/eval.yaml
@@ -26,7 +26,7 @@ scenarios:
value: "Devil's Advocate"
rubric:
- "The agent provides a plain summary without launching a structured multi-step review workflow"
- - "The agent does NOT load review-rules.md or walk through MAUI-specific review checklists"
+ - "The agent does NOT walk through a multi-step review workflow"
timeout: 120
- name: "Independence-first - agent reads diff before description"
@@ -64,7 +64,7 @@ scenarios:
pattern: "(NEEDS_CHANGES|NEEDS_DISCUSSION)"
rubric:
- "If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM"
- - "The agent applies handler lifecycle rules from review-rules.md (ConnectHandler/DisconnectHandler symmetry)"
+ - "The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry)"
- "The agent cites specific file and line references for the concern"
timeout: 300
diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md
index eab358b1d112..51baca74c8f3 100644
--- a/.github/skills/pr-review/SKILL.md
+++ b/.github/skills/pr-review/SKILL.md
@@ -244,10 +244,14 @@ CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/
├── try-fix/
│ ├── content.md # Phase 2 summary
│ └── attempt-{N}/ # Per-model attempt
-│ ├── approach.md # What was tried
-│ ├── result.txt # Pass / Fail / Blocked
-│ ├── fix.diff # git diff of changes
-│ └── analysis.md # Why it worked/failed
+│ ├── baseline.log # Baseline establishment proof
+│ ├── approach.md # What was tried
+│ ├── result.txt # Pass / Fail / Blocked
+│ ├── fix.diff # git diff of changes
+│ ├── test-output.log # Full test command output
+│ ├── reviewer-findings.json # Inline expert self-review (`[]` if clean) — reflects the FINAL diff (refreshed by Step 7.5 if test loop modified code)
+│ ├── reviewer-findings.diff # Snapshot of the diff that the self-review evaluated (used by Step 7.5 to detect drift)
+│ └── analysis.md # Why it worked/failed + self-review summary
└── report/
└── content.md # Phase 3 output (pr-report)
```
diff --git a/.github/skills/try-fix/SKILL.md b/.github/skills/try-fix/SKILL.md
index ca1fcdfc4089..e421eabffed5 100644
--- a/.github/skills/try-fix/SKILL.md
+++ b/.github/skills/try-fix/SKILL.md
@@ -26,7 +26,7 @@ If the prompt does not include a **problem to fix** and a **test command to veri
4. **Empirical** - Actually implement and test, don't just theorize
5. **Context-driven** - Work with what's provided and git history; don't search external sources
-**Every invocation:** Review existing fixes → Think of DIFFERENT approach → Implement and test → Report results
+**Every invocation runs all 11 Workflow steps below.** Step 6 (Expert Self-Review) is performed inline against `.github/agents/maui-expert-reviewer.md` — do NOT spawn the `@maui-expert-reviewer` sub-agent. Step 7.5 refreshes the self-review if the test loop modified code so the recorded findings reflect the final diff. Step 8 enforces this via a file-existence gate on `reviewer-findings.json`.
## ⚠️ CRITICAL: Sequential Execution Only
@@ -69,6 +69,7 @@ Results reported back to the invoker:
| `result` | `Pass`, `Fail`, or `Blocked` |
| `analysis` | Why it worked, or why it failed and what was learned |
| `diff` | The actual code changes made (for review) |
+| `findings_count` | Number of self-review findings recorded (0 = clean self-review) |
## Output Structure (MANDATORY)
@@ -96,10 +97,12 @@ Write-Host "Output directory: $OUTPUT_DIR"
|------|----------------|---------|
| `baseline.log` | After Step 2 (Baseline) | Output from EstablishBrokenBaseline.ps1 proving baseline was established |
| `approach.md` | After Step 4 (Design) | What fix you're attempting and why it's different from existing fixes |
-| `result.txt` | After Step 6 (Test) | Single word: `Pass`, `Fail`, or `Blocked` |
-| `fix.diff` | After Step 6 (Test) | Output of `git diff` showing your changes |
-| `test-output.log` | After Step 6 (Test) | Full output from test command |
-| `analysis.md` | After Step 6 (Test) | Why it worked/failed, insights learned |
+| `reviewer-findings.json` | After Step 6 (Self-Review), refreshed by Step 7.5 | JSON array of self-review findings — `[]` when clean. **MUST reflect the final diff.** |
+| `reviewer-findings.diff` | After Step 6 (Self-Review), refreshed by Step 7.5 | Snapshot of `git diff` at the time the self-review was written. Step 7.5 compares this to the post-test-loop diff to detect drift. |
+| `result.txt` | After Step 7 (Test) | Single word: `Pass`, `Fail`, or `Blocked` |
+| `fix.diff` | After Step 7 (Test) | Output of `git diff` showing your changes |
+| `test-output.log` | After Step 7 (Test) | Full output from test command |
+| `analysis.md` | After Step 8 (Capture) | Why it worked/failed, insights learned, and a one-line self-review summary |
**Example approach.md:**
```markdown
@@ -125,10 +128,11 @@ The skill is complete when:
- [ ] ONE fix approach designed and implemented
- [ ] Fix tested with provided test command (iterated up to 3 times if errors/failures)
- [ ] Either: Tests PASS ✅, or exhausted attempts and documented why approach won't work ❌
+- [ ] **Expert self-review performed inline (Step 6) and `reviewer-findings.json` written** — `[]` if clean. **Refreshed by Step 7.5 if the test loop modified code, so the saved findings reflect the final diff.**
- [ ] Analysis provided (success explanation or failure reasoning with evidence)
-- [ ] Artifacts saved to output directory
+- [ ] Artifacts saved to output directory (verified by Step 8 file-existence gate)
- [ ] Baseline restored (working directory clean)
-- [ ] Results reported to invoker
+- [ ] Results reported to invoker (including `findings_count`)
🚨 **CRITICAL: What counts as "Pass" vs "Fail"**
@@ -261,7 +265,86 @@ Based on your analysis and any provided hints, design a single fix approach:
Implement your fix. Use `git status --short` and `git diff` to track changes.
-### Step 6: Test and Iterate (MANDATORY)
+### Step 6: Expert Self-Review (MANDATORY — runs BEFORE testing)
+
+🚨 **You perform this self-review yourself. Do NOT spawn the `@maui-expert-reviewer` sub-agent.** Step 8's file-existence gate enforces that `reviewer-findings.json` is written every attempt.
+
+This step runs BEFORE testing so you can catch design flaws before spending time on build+test cycles.
+
+**Procedure:**
+
+1. **Read the rules.** View these specific sections of `.github/agents/maui-expert-reviewer.md`:
+ - **`## Overarching Principles`** (8 numbered principles, near the top of the file) — apply to every fix
+ - **`## Dimension Routing`** + **`### Always-Active Dimensions`** — pick the dimensions that match your changed files
+ - For each routed dimension, jump to its CHECK list under `## Review Dimensions` (e.g., `### 1. Layout Measure-Arrange Correctness`)
+
+ You only need the dimensions that match the files you actually touched plus the always-active ones — typically 3–6 sections, not all 30.
+
+2. **Identify your changed files:**
+ ```powershell
+ git diff --name-only HEAD
+ ```
+
+ If you have NO code changes (e.g., Blocked because no device available before any fix was applied), still proceed to step 4 and write `'[]'` — the artifact gate is the enforcement mechanism.
+
+3. **Walk your diff against the rules:**
+ - For each Overarching Principle → does your diff violate it?
+ - For each routed dimension → walk every CHECK rule against the relevant hunks
+ - Always-Active dimensions (Logic and Correctness, Regression Prevention, Complexity Reduction) → apply regardless of file paths
+ - Be honest. If unsure, flag it.
+
+4. **Write findings to `$OUTPUT_DIR/reviewer-findings.json`.** Always write the file, even when there are zero findings. Use the same JSON format as the `@maui-expert-reviewer` agent (matches the GitHub Pull Request Review API):
+
+ ```powershell
+ # No findings — clean self-review (or no diff to review):
+ '[]' | Set-Content "$OUTPUT_DIR/reviewer-findings.json"
+
+ # With findings — JSON array of {path, line, body}:
+ @'
+ [
+ {
+ "path": "src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs",
+ "line": 42,
+ "body": "**[major] Layout Measure-Arrange** — Content measured with unconstrained height but arranged with bounded height. Concrete scenario: ScrollView inside a Grid with Star row height."
+ }
+ ]
+ '@ | Set-Content "$OUTPUT_DIR/reviewer-findings.json"
+ ```
+
+ Each entry has exactly 3 fields:
+ - **`path`** (string) — file relative to repo root, must be a file present in your diff
+ - **`line`** (integer ≥ 1) — line number on the changed (right) side of the diff. The line MUST appear in your diff — picking an unchanged line is wrong. Use `1` only as a fallback for file-level concerns where no single line captures the issue (e.g., missing import, structural concern).
+ - **`body`** (string) — format `**[severity] Dimension** — description`. Severity is one of `critical`/`major`/`moderate`/`minor`.
+
+5. **Validate the JSON parses and capture the count:**
+ ```powershell
+ try {
+ $findings = @(Get-Content "$OUTPUT_DIR/reviewer-findings.json" -Raw | ConvertFrom-Json)
+ $findingsCount = $findings.Count
+ Write-Host "✅ reviewer-findings.json: $findingsCount findings"
+ } catch {
+ Write-Host "❌ reviewer-findings.json is invalid JSON: $_"
+ throw
+ }
+
+ # Snapshot the diff that was reviewed — Step 7.5 uses this to detect whether the test loop mutated code.
+ # Use Set-Content -Value with Out-String so the file is created even when the diff is empty
+ # (a bare `git diff | Set-Content` does NOT create the file when the pipe is empty).
+ Set-Content -Path "$OUTPUT_DIR/reviewer-findings.diff" -Value (git diff | Out-String) -NoNewline
+ ```
+
+ **Remember `$findingsCount`** — you will report it as `findings_count` in Step 10 and summarize it in `analysis.md` (Step 8).
+
+6. **Fix critical/major findings BEFORE testing:**
+ - If there are any `[critical]` or `[major]` findings → apply fixes for them in a single batch and rewrite `reviewer-findings.json` to reflect the new diff.
+ - All `[moderate]` and `[minor]` findings → note in `analysis.md` (Step 8); do NOT iterate.
+ - Only ONE correction round. Then proceed to Step 7 (Test).
+
+**Threshold guidance.** Only record findings with a concrete failing scenario. Stylistic preferences and bikeshedding (see the **`## What NOT to Flag`** table in `maui-expert-reviewer.md`) are not findings. An empty `[]` is the correct output for a clean fix — do not invent findings to fill the file.
+
+> **Why before testing?** Self-review catches design flaws (wrong null check, missing platform guard, thread safety issue) before you spend 5-15 minutes on a build+test cycle. It also runs when context is lightest — before test output floods the context window.
+
+### Step 7: Test and Iterate (MANDATORY)
🚨 **CRITICAL: ALWAYS use the provided test command script - NEVER manually build/compile.**
@@ -286,7 +369,7 @@ pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project **Why no programmatic "did you actually rewrite the JSON" check?** A SHA256 hash sentinel rejects the legitimate byte-identical case (e.g., `[]` → `[]` after a small compile fix that introduces no new violations), and that case is common. The procedural enforcement is sub-step 2's explicit numbered list above, plus the example-invocation chain that walks the dimensions explicitly. If sub-step 2 is skipped, the JSON validates but ships a stale review — accept that risk in exchange for not blocking valid clean fixes.
+
+**Severity handling is the same as Step 6.** If the refresh surfaces new `[critical]` or `[major]` findings, you may apply ONE more fix batch and re-run the test loop, then re-refresh. Do not loop indefinitely — if a fix introduces critical findings on the third pass, mark the attempt `Blocked` and explain in `analysis.md`.
+
+### Step 8: Capture Artifacts (MANDATORY)
**Before reverting, save ALL required files to `$OUTPUT_DIR`:**
@@ -307,13 +446,18 @@ See [references/compile-errors.md](references/compile-errors.md) for error patte
# 1. Save result (MUST be exactly "Pass", "Fail", or "Blocked")
"Pass" | Set-Content "$OUTPUT_DIR/result.txt" # or "Fail"
-# 2. Save the diff
-git diff | Set-Content "$OUTPUT_DIR/fix.diff"
+# 2. Save the diff (use Set-Content -Value with Out-String so the file is created
+# even when the diff is empty — a bare `git diff | Set-Content` does not create
+# the file when the pipe is empty, which would fail the artifact gate.)
+Set-Content -Path "$OUTPUT_DIR/fix.diff" -Value (git diff | Out-String) -NoNewline
-# 3. Save test output (should already exist from Step 6)
+# 3. Save test output (should already exist from Step 7)
# Copy-Item "path/to/test-output.log" "$OUTPUT_DIR/test-output.log"
-# 4. Save analysis
+# 4. reviewer-findings.json should already exist from Step 6 (and may have been refreshed by Step 7.5)
+# 4b. reviewer-findings.diff snapshot (used by Step 7.5 to detect drift)
+
+# 5. Save analysis (include a one-line summary of self-review findings)
@"
## Analysis
@@ -323,23 +467,46 @@ git diff | Set-Content "$OUTPUT_DIR/fix.diff"
**Why it worked/failed:** [Root cause analysis]
+**Self-review:** [N findings: brief summary of each, or "clean — no findings"]
+
**Insights:** [What was learned that could help future attempts]
"@ | Set-Content "$OUTPUT_DIR/analysis.md"
```
-**Verify all required files exist:**
+**Verify all required files exist (this is the enforcement gate for Steps 6 and 7 — primarily `reviewer-findings.json` from Step 6, refreshed by Step 7.5 if needed):**
+
+🚨 **The artifact check below MUST be wrapped so that Step 9 (Restore) ALWAYS runs even if the check fails.** A failed gate that skips restore would leave the worktree dirty and corrupt the next sequential try-fix attempt.
+
```powershell
-@("baseline.log", "approach.md", "result.txt", "fix.diff", "analysis.md", "test-output.log") | ForEach-Object {
- if (Test-Path "$OUTPUT_DIR/$_") { Write-Host "✅ $_" }
- else { Write-Host "❌ MISSING: $_" }
+# Run the file-existence check, but DEFER any throw until after Step 9 has restored the worktree.
+$missing = @()
+@("baseline.log", "approach.md", "result.txt", "fix.diff", "analysis.md", "test-output.log", "reviewer-findings.json", "reviewer-findings.diff") | ForEach-Object {
+ if (Test-Path "$OUTPUT_DIR/$_") {
+ Write-Host "✅ $_"
+ } else {
+ Write-Host "❌ MISSING: $_"
+ $missing += $_
+ }
+}
+
+# Record the gate result for use after Step 9 — DO NOT throw here.
+if ($missing.Count -gt 0) {
+ $gateFailureMessage = "Required artifacts missing: $($missing -join ', '). If 'reviewer-findings.json' is missing, Step 6 (Expert Self-Review) was not performed (or Step 7.5 did not refresh it after the test loop) — it is mandatory and must contain at least '[]' that reflects the final diff."
+ Write-Host "⚠️ ARTIFACT GATE FAILED — proceeding to Step 9 restore before reporting failure."
+ Write-Host $gateFailureMessage
+ "Blocked" | Set-Content "$OUTPUT_DIR/result.txt" -Force
+} else {
+ $gateFailureMessage = $null
}
```
+**If `$gateFailureMessage` was set:** Step 9 still runs (do NOT skip it). After Step 9 restores the worktree, surface the failure in Step 10's report — set `result.txt` to `Blocked` (already done above) and explain in `analysis.md` which artifact was missing. The next sequential attempt then starts from a clean worktree.
+
**Analysis quality matters.** Bad: "Didn't work". Good: "Fix attempted to reset state in OnPageSelected, but this fires after layout measurement. The cached value was already used."
-### Step 8: Restore Working Directory (MANDATORY)
+### Step 9: Restore Working Directory (MANDATORY — runs even if Step 8 gate failed)
-**ALWAYS restore, even if fix failed.**
+**ALWAYS restore, even if fix failed or Step 8 detected missing artifacts.** Skipping restore corrupts the next sequential try-fix attempt.
```bash
pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore
@@ -347,7 +514,7 @@ pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore
🚨 Use `EstablishBrokenBaseline.ps1 -Restore` — not `git checkout`, `git restore`, or `git reset` (see Step 2 for why).
-### Step 9: Report Results
+### Step 10: Report Results
Provide structured output to the invoker:
@@ -361,6 +528,8 @@ Provide structured output to the invoker:
**Result:** ✅ PASS / ❌ FAIL
+**Self-Review:** N findings (X critical, Y major, Z moderate/minor) — see `reviewer-findings.json`
+
**Analysis:**
[Why it worked, or why it failed and what was learned]
@@ -381,7 +550,7 @@ Provide structured output to the invoker:
| Test command fails to run | Report build/setup error with details |
| Test times out | Report timeout, include partial output |
| Can't determine fix approach | Report "no viable approach identified" with reasoning |
-| Git state unrecoverable | Run `pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore` (see Step 2/8) |
+| Git state unrecoverable | Run `pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore` (see Step 2/9) |
---
diff --git a/.github/skills/try-fix/references/example-invocation.md b/.github/skills/try-fix/references/example-invocation.md
index 112f8b7ce069..362077a665c1 100644
--- a/.github/skills/try-fix/references/example-invocation.md
+++ b/.github/skills/try-fix/references/example-invocation.md
@@ -23,4 +23,4 @@ hints: |
- Focus on the Disconnect/Cleanup methods
```
-**Skill execution:** Reads context → Analyzes target files → Designs fix (add IsDisposed check) → Applies fix → Runs test (PASS) → Reports result → Reverts changes
+**Skill execution:** Reads context → Analyzes target files → Designs fix (add IsDisposed check) → Applies fix → Performs inline expert self-review against `.github/agents/maui-expert-reviewer.md` rules and writes `reviewer-findings.json` (`[]` if clean) → Runs test (PASS) → If code changed during the test loop, refreshes `reviewer-findings.json` against the final diff → Captures artifacts → Reverts changes → Reports result
diff --git a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1
index 168f9211bf9e..c623c8a8860f 100644
--- a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1
+++ b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1
@@ -622,8 +622,42 @@ function Get-TestResultFromOutput {
}
# Check for build failures (before any test results)
+ # Mark these explicitly with BuildError = $true so Write-MarkdownReport can
+ # surface them as "Fix does not compile" instead of "Fix does not pass the tests".
if ($content -match "Build FAILED" -or $content -match "Build failed with exit code" -or $content -match "error MSB\d+" -or $content -match "error CS\d+") {
- return @{ Passed = $false; Error = "Build failed before tests could run"; FailCount = 0; Failed = 0; Total = 0; Skipped = 0 }
+ # Capture the first compile error so the diagnosis is concrete.
+ $buildErrorExcerpt = $null
+ $errMatch = [regex]::Match($content, '(?m)^.*\b(error CS\d+|error MSB\d+)\b.*$')
+ if ($errMatch.Success) {
+ $excerpt = $errMatch.Value.Trim()
+ if ($excerpt.Length -gt 200) { $excerpt = $excerpt.Substring(0, 200) + "..." }
+ $buildErrorExcerpt = $excerpt
+ }
+ return @{
+ Passed = $false; BuildError = $true
+ Error = if ($buildErrorExcerpt) { "Build failed: $buildErrorExcerpt" } else { "Build failed before tests could run" }
+ FailureMessage = $buildErrorExcerpt
+ FailCount = 0; Failed = 0; Total = 0; Skipped = 0
+ }
+ }
+
+ # Check for filter mismatch — the test runner ran successfully but the supplied
+ # -filter expression matched zero test cases. Without this branch the gate
+ # would treat "0 tests ran" as ENV ERROR (or worse, silently as a failed
+ # test) — both misclassifications. The fix is to surface this as a separate
+ # "FilterMismatch" outcome so Write-MarkdownReport can label it accurately.
+ if ($content -match 'No test matches the given testcase filter' -or
+ $content -match '(?im)^\s*Test count:\s*0\b') {
+ $attemptedFilter = $null
+ $fmMatch = [regex]::Match($content, "No test matches the given testcase filter '([^']+)'")
+ if ($fmMatch.Success) { $attemptedFilter = $fmMatch.Groups[1].Value }
+ elseif ($TestFilter) { $attemptedFilter = $TestFilter }
+ return @{
+ Passed = $false; FilterMismatch = $true
+ Error = if ($attemptedFilter) { "Test filter '$attemptedFilter' matched 0 tests" } else { "Test filter matched 0 tests" }
+ FailureMessage = $attemptedFilter
+ FailCount = 0; Failed = 0; Total = 0; Skipped = 0
+ }
}
# --- Device test output: [PASS]/[FAIL] markers from xharness ---
@@ -1153,11 +1187,74 @@ function Write-MarkdownReport {
$status = if ($hasEnvError) { "⚠️ ENV ERROR" } elseif ($VerificationPassed) { "✅ PASSED" } else { "❌ FAILED" }
$mergeBaseShort = if ($ReportMergeBase -and $ReportMergeBase.Length -ge 8) { $ReportMergeBase.Substring(0, 8) } else { "$ReportMergeBase" }
+ # ─── Improvement #2: classify the failure mode so the headline matches the cause ───
+ # Without this, every non-PASSED gate just says "tests did not behave as expected".
+ # Map the without/with-fix outcomes per test into a concrete diagnosis the
+ # downstream Try-Fix×4 stage and the human reader can act on.
+ #
+ # Reliability extensions:
+ # - BuildError flag → headline says "Fix does not compile" (was conflated
+ # with "Fix does not pass the tests" because the test runner can't load
+ # an assembly that doesn't compile, so every test in it appears to fail).
+ # - FilterMismatch flag → headline says "Test filter matched 0 tests"
+ # (was misclassified as ENV ERROR or as a generic FAIL because zero
+ # tests ran but exit code was non-zero).
+ $failureClassification = $null
+ if (-not $hasEnvError -and -not $VerificationPassed -and $WithoutFixResultsList -and $WithFixResultsList) {
+ # Build error in the with-fix run trumps every other classification — if
+ # the fix doesn't compile, no per-test outcome is meaningful.
+ $wBuildError = @($WithFixResultsList | Where-Object { $_.BuildError })
+ $woBuildError = @($WithoutFixResultsList | Where-Object { $_.BuildError })
+ $wFilterMiss = @($WithFixResultsList | Where-Object { $_.FilterMismatch })
+ $woFilterMiss = @($WithoutFixResultsList | Where-Object { $_.FilterMismatch })
+
+ $woStates = @($WithoutFixResultsList | ForEach-Object { if ($_.EnvError) { "ENV" } elseif ($_.BuildError) { "BUILD" } elseif ($_.FilterMismatch) { "NOMATCH" } elseif ($_.Passed) { "PASS" } else { "FAIL" } })
+ $wStates = @($WithFixResultsList | ForEach-Object { if ($_.EnvError) { "ENV" } elseif ($_.BuildError) { "BUILD" } elseif ($_.FilterMismatch) { "NOMATCH" } elseif ($_.Passed) { "PASS" } else { "FAIL" } })
+
+ $allWoPass = ($woStates | Where-Object { $_ -ne "PASS" }).Count -eq 0
+ $allWoFail = ($woStates | Where-Object { $_ -ne "FAIL" }).Count -eq 0
+ $allWFail = ($wStates | Where-Object { $_ -ne "FAIL" }).Count -eq 0
+ $hasRegression = $false
+ # Regression: at least one test fixes (FAIL→PASS) AND at least one regresses (FAIL→FAIL)
+ for ($i = 0; $i -lt $woStates.Count -and $i -lt $wStates.Count; $i++) {
+ if ($woStates[$i] -eq "FAIL" -and $wStates[$i] -eq "FAIL") { $hasRegression = $true }
+ }
+ $hasFixedTest = $false
+ for ($i = 0; $i -lt $woStates.Count -and $i -lt $wStates.Count; $i++) {
+ if ($woStates[$i] -eq "FAIL" -and $wStates[$i] -eq "PASS") { $hasFixedTest = $true }
+ }
+
+ if ($wBuildError.Count -gt 0) {
+ $excerpt = ($wBuildError | ForEach-Object { $_.FailureMessage } | Where-Object { $_ } | Select-Object -First 1)
+ $excerptLine = if ($excerpt) { "`n> ``$excerpt``" } else { "" }
+ $failureClassification = "🩺 **Fix does not compile** — applying the PR's fix produces a build error before tests can run. The earlier-than-test failure is the root cause; the per-test ❌ FAIL marks are downstream effects, not real test failures.$excerptLine"
+ } elseif ($woBuildError.Count -gt 0) {
+ $failureClassification = "🩺 **Base branch does not compile** — the without-fix build failed. The gate's ""does the test fail without the fix"" check is unreliable here; this usually means ``main`` is broken or a merge-base file went missing. Investigate before trusting this gate."
+ } elseif ($wFilterMiss.Count -gt 0 -or $woFilterMiss.Count -gt 0) {
+ $missing = ($wFilterMiss + $woFilterMiss | ForEach-Object { $_.FailureMessage } | Where-Object { $_ } | Select-Object -First 1)
+ $hint = if ($missing) { " — filter ``$missing`` matched 0 tests" } else { "" }
+ $failureClassification = "🩺 **Test filter mismatch**$hint. The test runner produced zero results because no test class or method matched the filter. Common causes: the gate filter was derived from the file name but the actual test class is named differently, or the test was renamed/moved without updating the auto-detection. Verify the test class name matches what the gate is searching for."
+ } elseif ($allWoPass) {
+ $failureClassification = "🩺 **Test does not reproduce the bug** — ran the same in both states (PASS without fix, PASS with fix). The repro test is not exercising the issue. Strengthen the test before reviewing the fix."
+ } elseif ($allWoFail -and $allWFail) {
+ $failureClassification = "🩺 **Fix does not pass the tests** — every test still fails after applying the fix. The PR's change does not resolve the failure(s)."
+ } elseif ($hasFixedTest -and $hasRegression) {
+ $failureClassification = "🩺 **Regression in another test** — at least one test goes FAIL→PASS (fix works there), but another test FAILs both with and without the fix. The fix breaks a pre-existing or sibling test."
+ } elseif ($hasRegression -and -not $hasFixedTest) {
+ $failureClassification = "🩺 **Fix breaks tests** — one or more tests fail with the fix applied, and none of the failures are resolved by the fix."
+ }
+ # else: leave $failureClassification unset; the per-test table + Failure Details below tell the story.
+ }
+
$lines = @()
$lines += "### Gate Result: $status"
$lines += ""
$platformDisplay = if ($ReportPlatform) { $ReportPlatform.ToUpper() } else { "N/A" }
$lines += "**Platform:** $platformDisplay · **Base:** $ReportBaseBranch · **Merge base:** ``$mergeBaseShort``"
+ if ($failureClassification) {
+ $lines += ""
+ $lines += $failureClassification
+ }
$lines += ""
# ── Side-by-side per-test comparison table ──
@@ -1172,6 +1269,10 @@ function Write-MarkdownReport {
$woDur = if ($woResult.Duration) { "$([math]::Round($woResult.Duration.TotalSeconds))s" } else { "" }
if ($woResult.EnvError) {
$woCell = "⚠️ ENV ERROR"
+ } elseif ($woResult.BuildError) {
+ $woCell = "🛠️ BUILD ERROR"
+ } elseif ($woResult.FilterMismatch) {
+ $woCell = "🔍 NO MATCH"
} elseif (-not $woResult.Passed) {
$woCell = "✅ FAIL — $woDur"
} else {
@@ -1182,6 +1283,10 @@ function Write-MarkdownReport {
$wDur = if ($wResult.Duration) { "$([math]::Round($wResult.Duration.TotalSeconds))s" } else { "" }
if ($wResult.EnvError) {
$wCell = "⚠️ ENV ERROR"
+ } elseif ($wResult.BuildError) {
+ $wCell = "🛠️ BUILD ERROR"
+ } elseif ($wResult.FilterMismatch) {
+ $wCell = "🔍 NO MATCH"
} elseif ($wResult.Passed) {
$wCell = "✅ PASS — $wDur"
} else {
@@ -1203,7 +1308,7 @@ function Write-MarkdownReport {
# Without fix log
$woLogFile = Join-Path $OutputPath "test-without-fix-$sanitizedName.log"
- $woStatus = if ($woResult.EnvError) { "⚠️ ENV ERROR" } elseif (-not $woResult.Passed) { "FAIL ✅" } else { "PASS ❌" }
+ $woStatus = if ($woResult.EnvError) { "⚠️ ENV ERROR" } elseif ($woResult.BuildError) { "🛠️ BUILD ERROR" } elseif ($woResult.FilterMismatch) { "🔍 NO MATCH" } elseif (-not $woResult.Passed) { "FAIL ✅" } else { "PASS ❌" }
$woDur = if ($woResult.Duration) { " · $([math]::Round($woResult.Duration.TotalSeconds))s" } else { "" }
$lines += ""
$lines += "
"
@@ -1232,7 +1337,7 @@ function Write-MarkdownReport {
# With fix log
$wLogFile = Join-Path $OutputPath "test-with-fix-$sanitizedName.log"
- $wStatus = if ($wResult.EnvError) { "⚠️ ENV ERROR" } elseif ($wResult.Passed) { "PASS ✅" } else { "FAIL ❌" }
+ $wStatus = if ($wResult.EnvError) { "⚠️ ENV ERROR" } elseif ($wResult.BuildError) { "🛠️ BUILD ERROR" } elseif ($wResult.FilterMismatch) { "🔍 NO MATCH" } elseif ($wResult.Passed) { "PASS ✅" } else { "FAIL ❌" }
$wDur = if ($wResult.Duration) { " · $([math]::Round($wResult.Duration.TotalSeconds))s" } else { "" }
$lines += ""
$lines += "
"
@@ -1262,13 +1367,31 @@ function Write-MarkdownReport {
# ── Failure details (shown directly — not collapsed) ──
$failureLines = @()
foreach ($r in $WithoutFixResultsList) {
- if ($r.Passed) {
+ if ($r.BuildError) {
+ $failureLines += "- 🛠️ **$($r.TestName)** without fix: build failed before tests could run"
+ if ($r.FailureMessage) {
+ $msg = if ($r.FailureMessage.Length -gt 300) { $r.FailureMessage.Substring(0, 300) + "..." } else { $r.FailureMessage }
+ $failureLines += " - ``$msg``"
+ }
+ } elseif ($r.FilterMismatch) {
+ $failureLines += "- 🔍 **$($r.TestName)** without fix: test filter matched 0 tests"
+ if ($r.FailureMessage) { $failureLines += " - filter: ``$($r.FailureMessage)``" }
+ } elseif ($r.Passed) {
$failureLines += "- ❌ **$($r.TestName)** PASSED without fix (should fail) — tests don't catch the bug"
}
if ($r.EnvError) { $failureLines += "- ⚠️ **$($r.TestName)** without fix: ``$($r.Error)``" }
}
foreach ($r in $WithFixResultsList) {
- if (-not $r.Passed -and -not $r.EnvError) {
+ if ($r.BuildError) {
+ $failureLines += "- 🛠️ **$($r.TestName)** with fix: build failed (fix does not compile)"
+ if ($r.FailureMessage) {
+ $msg = if ($r.FailureMessage.Length -gt 300) { $r.FailureMessage.Substring(0, 300) + "..." } else { $r.FailureMessage }
+ $failureLines += " - ``$msg``"
+ }
+ } elseif ($r.FilterMismatch) {
+ $failureLines += "- 🔍 **$($r.TestName)** with fix: test filter matched 0 tests"
+ if ($r.FailureMessage) { $failureLines += " - filter: ``$($r.FailureMessage)``" }
+ } elseif (-not $r.Passed -and -not $r.EnvError) {
$failureLines += "- ❌ **$($r.TestName)** FAILED with fix (should pass)"
if ($r.FailureReason) { $failureLines += " - ``$($r.FailureReason)``" }
if ($r.FailureMessage) {
@@ -1280,10 +1403,10 @@ function Write-MarkdownReport {
}
if ($failureLines.Count -gt 0) {
- # Count actual failed tests (lines beginning with "- ❌" or "- ⚠️") to decide
- # whether to collapse. Sub-bullets (FailureReason / FailureMessage) start with
- # two leading spaces so they don't match.
- $failedTestCount = @($failureLines | Where-Object { $_ -match '^- (❌|⚠️)' }).Count
+ # Count actual failed tests (lines beginning with "- ❌"/"- ⚠️"/"- 🛠️"/"- 🔍")
+ # to decide whether to collapse. Sub-bullets (FailureReason / FailureMessage)
+ # start with two leading spaces so they don't match.
+ $failedTestCount = @($failureLines | Where-Object { $_ -match '^- (❌|⚠️|🛠️|🔍)' }).Count
# Threshold: if more than 5 tests failed, collapse the section so the gate
# summary stays visible above the fold in PR comments. Below the threshold,
# show details inline so reviewers don't need an extra click.
diff --git a/eng/pipelines/ci-copilot.yml b/eng/pipelines/ci-copilot.yml
index 165cb94cd6c0..2f77543be29d 100644
--- a/eng/pipelines/ci-copilot.yml
+++ b/eng/pipelines/ci-copilot.yml
@@ -649,6 +649,7 @@ stages:
COPILOT_GITHUB_TOKEN: $(COPILOT_TOKEN)
GH_TOKEN: $(GH_COMMENT_TOKEN)
DEVICE_UDID: $(DEVICE_UDID)
+ COMMENTS_VIA_FILE: "true"
# Publish Copilot logs and session artifacts
- task: PublishPipelineArtifact@1
From f8cb875eeec2dff76dd158e21634d3216ddc2edd Mon Sep 17 00:00:00 2001
From: TamilarasanSF4853
Date: Mon, 11 May 2026 15:55:53 +0530
Subject: [PATCH 05/12] [Testing] The Windows WebView category is removed from
CI because WebView2 is not connected in Appium. (#35335)
### Description of Changes
- Recently, the Appium driver has not been connecting properly to the
native WebView2 control on Windows. While running locally using Appium
Inspector with the WebView control, the inspector is unable to recognize
the WebView and displays an error.
- Due to this Appium driver issue, the WebView lane in CI takes a long
time to run (approximately 3 hours) and eventually gets cancelled. As a
temporary workaround, the WebView lane has been temporarily removed from
the Windows CI pipeline to allow the CI process to complete more
quickly.
**Issue:** https://github.com/dotnet/maui/issues/35334
---
eng/pipelines/common/ui-tests.yml | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/eng/pipelines/common/ui-tests.yml b/eng/pipelines/common/ui-tests.yml
index 258ab00de3e7..eadb114c2799 100644
--- a/eng/pipelines/common/ui-tests.yml
+++ b/eng/pipelines/common/ui-tests.yml
@@ -457,9 +457,12 @@ stages:
- job: winui_ui_tests_${{ project.name }}
strategy:
matrix:
+ # WebView is excluded on Windows: Appium driver cannot attach to WebView2
+ # (appium/appium#22217 / dotnet/maui#35334). Remove this filter once fixed.
${{ each categoryGroup in parameters.categoryGroupsToTest }}:
- ${{ categoryGroup }}:
- CATEGORYGROUP: ${{ categoryGroup }}
+ ${{ if ne(categoryGroup, 'WebView') }}:
+ ${{ categoryGroup }}:
+ CATEGORYGROUP: ${{ categoryGroup }}
timeoutInMinutes: ${{ parameters.timeoutInMinutes }} # how long to run the job before automatically cancelling
workspace:
clean: all
From e752fea35e62a2c889d32596927e5fdff345b24c Mon Sep 17 00:00:00 2001
From: BagavathiPerumal
Date: Tue, 24 Mar 2026 16:08:36 +0530
Subject: [PATCH 06/12] fix-33510: Made code changes to prevent RefreshView
from intercepting WebView drag gestures when internal DOM content can still
scroll up.
---
.../TestCases.HostApp/Issues/Issue33510.cs | 225 ++++++++++++++++++
.../Tests/Issues/Issue33510.cs | 128 ++++++++++
.../Android/MauiSwipeRefreshLayout.cs | 62 ++++-
src/Core/src/Platform/Android/MauiWebView.cs | 29 +++
.../src/Platform/Android/MauiWebViewClient.cs | 3 +
.../RefreshViewWebViewScrollCapture.cs | 166 +++++++++++++
.../net-android/PublicAPI.Unshipped.txt | 3 +
7 files changed, 615 insertions(+), 1 deletion(-)
create mode 100644 src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
create mode 100644 src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
create mode 100644 src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
new file mode 100644
index 000000000000..8e575d8ae768
--- /dev/null
+++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
@@ -0,0 +1,225 @@
+using System.Text;
+
+namespace Maui.Controls.Sample.Issues;
+
+[Issue(IssueTracker.Github, 33510, "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView", PlatformAffected.Android)]
+public class Issue33510 : TestContentPage
+{
+ WebView _webView;
+ RefreshView _refreshView;
+ Label _statusLabel;
+ Label _scrollTopLabel;
+ bool _isWebViewLoaded;
+
+ protected override void Init()
+ {
+ Title = "RefreshView + WebView";
+
+ _statusLabel = new Label
+ {
+ AutomationId = "StatusLabel",
+ Text = "Loading WebView..."
+ };
+
+ _scrollTopLabel = new Label
+ {
+ AutomationId = "ScrollTopLabel",
+ Text = "ScrollTop: unavailable"
+ };
+
+ var scrollWebViewButton = new Button
+ {
+ AutomationId = "ScrollWebViewButton",
+ Text = "Scroll down in WebView"
+ };
+
+ scrollWebViewButton.Clicked += async (_, _) =>
+ {
+ if (!_isWebViewLoaded)
+ {
+ return;
+ }
+
+ var result = await _webView.EvaluateJavaScriptAsync("window.scrollInnerContainerTo(900);");
+ _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
+ };
+
+ var readScrollTopButton = new Button
+ {
+ AutomationId = "ReadScrollTopButton",
+ Text = "Read WebView scroll position"
+ };
+
+ readScrollTopButton.Clicked += async (_, _) => await UpdateScrollStatusAsync();
+
+ var scrollWebViewToTopButton = new Button
+ {
+ AutomationId = "ScrollWebViewToTopButton",
+ Text = "Scroll WebView to top"
+ };
+
+ scrollWebViewToTopButton.Clicked += async (_, _) =>
+ {
+ if (!_isWebViewLoaded)
+ {
+ return;
+ }
+
+ var result = await _webView.EvaluateJavaScriptAsync("window.scrollInnerContainerTo(0);");
+ _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
+ };
+
+ _webView = new WebView
+ {
+ AutomationId = "TestWebView",
+ };
+
+ _webView.Navigated += async (_, _) =>
+ {
+ _isWebViewLoaded = true;
+ _statusLabel.Text = "WebView ready. Scroll down, then drag downward inside the WebView.";
+ await UpdateScrollStatusAsync();
+ };
+
+ _webView.Source = new HtmlWebViewSource
+ {
+ Html = CreateHtml()
+ };
+
+ var webViewContainer = new ContentView
+ {
+ AutomationId = "TestWebViewContainer",
+ HorizontalOptions = LayoutOptions.Fill,
+ VerticalOptions = LayoutOptions.Fill,
+ Content = _webView
+ };
+
+ _refreshView = new RefreshView
+ {
+ AutomationId = "TestRefreshView",
+ Content = webViewContainer
+ };
+
+ _refreshView.Command = new Command(async () =>
+ {
+ _statusLabel.Text = "Refresh triggered";
+
+ await Task.Delay(150);
+ _refreshView.IsRefreshing = false;
+
+ await UpdateScrollStatusAsync();
+ });
+
+ var controlsLayout = new VerticalStackLayout
+ {
+ Padding = new Thickness(12),
+ Spacing = 8,
+ Children =
+ {
+ new Label
+ {
+ Text = "This page reproduces issue #33510. On Android, RefreshView should not refresh while the WebView can still scroll upward internally."
+ },
+ _statusLabel,
+ _scrollTopLabel,
+ scrollWebViewButton,
+ scrollWebViewToTopButton,
+ readScrollTopButton
+ }
+ };
+
+ var separator = new BoxView
+ {
+ HeightRequest = 1,
+ Color = Colors.LightGray
+ };
+
+ var grid = new Grid
+ {
+ RowDefinitions =
+ {
+ new RowDefinition { Height = GridLength.Auto },
+ new RowDefinition { Height = GridLength.Auto },
+ new RowDefinition { Height = GridLength.Star }
+ }
+ };
+
+ grid.Add(controlsLayout);
+ Grid.SetRow(controlsLayout, 0);
+
+ grid.Add(separator);
+ Grid.SetRow(separator, 1);
+
+ grid.Add(_refreshView);
+ Grid.SetRow(_refreshView, 2);
+
+ Content = grid;
+ }
+
+ async Task UpdateScrollStatusAsync()
+ {
+ if (!_isWebViewLoaded)
+ {
+ _scrollTopLabel.Text = "ScrollTop: unavailable";
+ return;
+ }
+
+ var result = await _webView.EvaluateJavaScriptAsync("window.getInnerScrollTop();");
+ _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
+ }
+
+ static string NormalizeJavaScriptNumber(string result)
+ {
+ if (string.IsNullOrWhiteSpace(result))
+ return "unknown";
+
+ return result.Trim().Trim('"');
+ }
+
+ static string CreateHtml()
+ {
+ var rows = string.Join(Environment.NewLine, Enumerable.Range(1, 30).Select(index =>
+ $"
Date: Fri, 27 Mar 2026 18:25:44 +0530
Subject: [PATCH 07/12] fix-33510-Changes updated.
---
.../Android/MauiSwipeRefreshLayout.cs | 26 +++++++++++++++----
.../src/Platform/Android/MauiWebViewClient.cs | 6 ++++-
.../RefreshViewWebViewScrollCapture.cs | 2 ++
3 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
index 284ecb5270de..3e3edac88e88 100644
--- a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
+++ b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
@@ -151,6 +151,27 @@ public override bool OnInterceptTouchEvent(MotionEvent? ev)
_webViewOwnsGesture = _touchStartedInWebView &&
RefreshViewWebViewScrollCapture.TryGetCanScrollUp(_activeTouchWebView, out var canScrollUpAtStart) &&
canScrollUpAtStart;
+ if (_webViewOwnsGesture)
+ {
+ // Forward to base so SwipeRefreshLayout records the initial pointer ID
+ // and Y position – required for correct mid-gesture intercept if the
+ // web content scrolls to the top during the same drag.
+ base.OnInterceptTouchEvent(ev);
+ return false;
+ }
+ break;
+ case MotionEventActions.Move:
+ // Re-evaluate scrollability so that once the WebView reaches the top,
+ // RefreshLayout can start intercepting mid-gesture.
+ if (_touchStartedInWebView && _webViewOwnsGesture && _activeTouchWebView is not null)
+ {
+ if (!RefreshViewWebViewScrollCapture.TryGetCanScrollUp(_activeTouchWebView, out var canStillScrollUp) || !canStillScrollUp)
+ {
+ _webViewOwnsGesture = false;
+ }
+ }
+ if (_touchStartedInWebView && _webViewOwnsGesture)
+ return false;
break;
case MotionEventActions.Cancel:
case MotionEventActions.Up:
@@ -160,11 +181,6 @@ public override bool OnInterceptTouchEvent(MotionEvent? ev)
break;
}
- if (_touchStartedInWebView && _webViewOwnsGesture)
- {
- return false;
- }
-
return base.OnInterceptTouchEvent(ev);
}
diff --git a/src/Core/src/Platform/Android/MauiWebViewClient.cs b/src/Core/src/Platform/Android/MauiWebViewClient.cs
index abaac0f770ab..a5b42375e31f 100644
--- a/src/Core/src/Platform/Android/MauiWebViewClient.cs
+++ b/src/Core/src/Platform/Android/MauiWebViewClient.cs
@@ -66,7 +66,11 @@ public override void OnPageFinished(WebView? view, string? url)
handler.SyncPlatformCookiesToVirtualView(url);
handler?.PlatformView.UpdateCanGoBackForward(handler.VirtualView);
- RefreshViewWebViewScrollCapture.InjectObserver(view);
+
+ // Only inject the scroll-capture observer when the WebView is hosted inside
+ // a RefreshView – avoids unnecessary JS overhead for standalone WebViews.
+ if (RefreshViewWebViewScrollCapture.IsAttached(view))
+ RefreshViewWebViewScrollCapture.InjectObserver(view);
base.OnPageFinished(view, url);
}
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
index 83497c730377..4cfa7b92f5fd 100644
--- a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -113,6 +113,8 @@ internal static void InjectObserver(WebView? webView)
webView.EvaluateJavascript(ObserverScript, null);
}
+ internal static bool IsAttached(WebView? webView) => GetState(webView) is not null;
+
internal static bool TryGetCanScrollUp(WebView? webView, out bool canScrollUp)
{
if (webView is null)
From 3c51cf5ec7f2f85f308687297a0cbe7132c6d8a3 Mon Sep 17 00:00:00 2001
From: BagavathiPerumal
Date: Tue, 21 Apr 2026 18:29:29 +0530
Subject: [PATCH 08/12] fix-33510-Simplied Testcase and test script.
Additionally, updated the code changes based on AI summary.
---
.../TestCases.HostApp/Issues/Issue33510.cs | 189 +-----------------
.../Tests/Issues/Issue33510.cs | 86 ++------
.../RefreshViewWebViewScrollCapture.cs | 17 +-
3 files changed, 37 insertions(+), 255 deletions(-)
diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
index 8e575d8ae768..16a54137b17d 100644
--- a/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
+++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
@@ -1,225 +1,52 @@
-using System.Text;
-
namespace Maui.Controls.Sample.Issues;
-[Issue(IssueTracker.Github, 33510, "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView", PlatformAffected.Android)]
+[Issue(IssueTracker.Github, 33510, "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView", PlatformAffected.Android, isInternetRequired: true)]
public class Issue33510 : TestContentPage
{
- WebView _webView;
RefreshView _refreshView;
Label _statusLabel;
- Label _scrollTopLabel;
- bool _isWebViewLoaded;
protected override void Init()
{
- Title = "RefreshView + WebView";
-
_statusLabel = new Label
{
AutomationId = "StatusLabel",
- Text = "Loading WebView..."
- };
-
- _scrollTopLabel = new Label
- {
- AutomationId = "ScrollTopLabel",
- Text = "ScrollTop: unavailable"
- };
-
- var scrollWebViewButton = new Button
- {
- AutomationId = "ScrollWebViewButton",
- Text = "Scroll down in WebView"
+ Text = "Loading..."
};
- scrollWebViewButton.Clicked += async (_, _) =>
- {
- if (!_isWebViewLoaded)
- {
- return;
- }
-
- var result = await _webView.EvaluateJavaScriptAsync("window.scrollInnerContainerTo(900);");
- _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
- };
-
- var readScrollTopButton = new Button
- {
- AutomationId = "ReadScrollTopButton",
- Text = "Read WebView scroll position"
- };
-
- readScrollTopButton.Clicked += async (_, _) => await UpdateScrollStatusAsync();
-
- var scrollWebViewToTopButton = new Button
- {
- AutomationId = "ScrollWebViewToTopButton",
- Text = "Scroll WebView to top"
- };
-
- scrollWebViewToTopButton.Clicked += async (_, _) =>
- {
- if (!_isWebViewLoaded)
- {
- return;
- }
-
- var result = await _webView.EvaluateJavaScriptAsync("window.scrollInnerContainerTo(0);");
- _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
- };
-
- _webView = new WebView
+ var webView = new WebView
{
AutomationId = "TestWebView",
+ Source = new UrlWebViewSource { Url = "https://material.angular.io/components/sidenav/overview" }
};
- _webView.Navigated += async (_, _) =>
- {
- _isWebViewLoaded = true;
- _statusLabel.Text = "WebView ready. Scroll down, then drag downward inside the WebView.";
- await UpdateScrollStatusAsync();
- };
-
- _webView.Source = new HtmlWebViewSource
- {
- Html = CreateHtml()
- };
-
- var webViewContainer = new ContentView
- {
- AutomationId = "TestWebViewContainer",
- HorizontalOptions = LayoutOptions.Fill,
- VerticalOptions = LayoutOptions.Fill,
- Content = _webView
- };
+ webView.Navigated += (_, _) => _statusLabel.Text = "WebView ready";
_refreshView = new RefreshView
{
AutomationId = "TestRefreshView",
- Content = webViewContainer
+ Content = webView
};
_refreshView.Command = new Command(async () =>
{
_statusLabel.Text = "Refresh triggered";
-
await Task.Delay(150);
_refreshView.IsRefreshing = false;
-
- await UpdateScrollStatusAsync();
});
- var controlsLayout = new VerticalStackLayout
- {
- Padding = new Thickness(12),
- Spacing = 8,
- Children =
- {
- new Label
- {
- Text = "This page reproduces issue #33510. On Android, RefreshView should not refresh while the WebView can still scroll upward internally."
- },
- _statusLabel,
- _scrollTopLabel,
- scrollWebViewButton,
- scrollWebViewToTopButton,
- readScrollTopButton
- }
- };
-
- var separator = new BoxView
- {
- HeightRequest = 1,
- Color = Colors.LightGray
- };
-
var grid = new Grid
{
RowDefinitions =
{
- new RowDefinition { Height = GridLength.Auto },
new RowDefinition { Height = GridLength.Auto },
new RowDefinition { Height = GridLength.Star }
}
};
- grid.Add(controlsLayout);
- Grid.SetRow(controlsLayout, 0);
-
- grid.Add(separator);
- Grid.SetRow(separator, 1);
-
- grid.Add(_refreshView);
- Grid.SetRow(_refreshView, 2);
+ grid.Add(_statusLabel, 0, 0);
+ grid.Add(_refreshView, 0, 1);
Content = grid;
}
-
- async Task UpdateScrollStatusAsync()
- {
- if (!_isWebViewLoaded)
- {
- _scrollTopLabel.Text = "ScrollTop: unavailable";
- return;
- }
-
- var result = await _webView.EvaluateJavaScriptAsync("window.getInnerScrollTop();");
- _scrollTopLabel.Text = $"ScrollTop: {NormalizeJavaScriptNumber(result)}";
- }
-
- static string NormalizeJavaScriptNumber(string result)
- {
- if (string.IsNullOrWhiteSpace(result))
- return "unknown";
-
- return result.Trim().Trim('"');
- }
-
- static string CreateHtml()
- {
- var rows = string.Join(Environment.NewLine, Enumerable.Range(1, 30).Select(index =>
- $"
Date: Mon, 27 Apr 2026 18:31:32 +0530
Subject: [PATCH 09/12] fix-33510-Testcase updated.
---
.../TestCases.HostApp/Issues/Issue33510.cs | 80 ++++++++++++++++++-
.../Tests/Issues/Issue33510.cs | 1 -
2 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
index 16a54137b17d..305937bb2313 100644
--- a/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
+++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue33510.cs
@@ -1,6 +1,6 @@
namespace Maui.Controls.Sample.Issues;
-[Issue(IssueTracker.Github, 33510, "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView", PlatformAffected.Android, isInternetRequired: true)]
+[Issue(IssueTracker.Github, 33510, "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView", PlatformAffected.Android)]
public class Issue33510 : TestContentPage
{
RefreshView _refreshView;
@@ -17,7 +17,7 @@ protected override void Init()
var webView = new WebView
{
AutomationId = "TestWebView",
- Source = new UrlWebViewSource { Url = "https://material.angular.io/components/sidenav/overview" }
+ Source = new HtmlWebViewSource { Html = ScrollableHtml }
};
webView.Navigated += (_, _) => _statusLabel.Text = "WebView ready";
@@ -49,4 +49,80 @@ protected override void Init()
Content = grid;
}
+
+ const string ScrollableHtml = """
+
+
+
+
+
+
+
+
+
Date: Thu, 30 Apr 2026 13:00:23 +0530
Subject: [PATCH 10/12] fix-33510-Code changes updated.
---
.../Android/MauiSwipeRefreshLayout.cs | 12 ++++++--
src/Core/src/Platform/Android/MauiWebView.cs | 8 ++++++
.../RefreshViewWebViewScrollCapture.cs | 28 ++++++++++++++++++-
3 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
index 3e3edac88e88..14749d2d2a37 100644
--- a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
+++ b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
@@ -236,6 +236,13 @@ static bool CanScrollUpViewByType(AView? view)
return true;
}
+ // Recursively hit-tests the view tree to find a WebView at the given
+ // coordinates (in the parent's coordinate space).
+ // ScrollX/ScrollY are added when converting to a child's local coordinate
+ // space so that scrolled containers (HorizontalScrollView, NestedScrollView,
+ // etc.) are handled correctly. Without this adjustment, any ViewGroup that
+ // has been scrolled would cause the hit-test to miss the WebView or match
+ // the wrong region.
static AWebView? FindWebView(AView? view, float x, float y)
{
if (view is null || view.Visibility != ViewStates.Visible)
@@ -250,8 +257,8 @@ static bool CanScrollUpViewByType(AView? view)
if (view is not ViewGroup viewGroup)
return null;
- var localX = x - view.Left;
- var localY = y - view.Top;
+ var localX = x - view.Left + view.ScrollX;
+ var localY = y - view.Top + view.ScrollY;
for (int i = viewGroup.ChildCount - 1; i >= 0; i--)
{
@@ -262,5 +269,6 @@ static bool CanScrollUpViewByType(AView? view)
return null;
}
+
}
}
diff --git a/src/Core/src/Platform/Android/MauiWebView.cs b/src/Core/src/Platform/Android/MauiWebView.cs
index 13b2c3621d8a..3f21af4e6ea1 100644
--- a/src/Core/src/Platform/Android/MauiWebView.cs
+++ b/src/Core/src/Platform/Android/MauiWebView.cs
@@ -37,7 +37,15 @@ protected override void OnAttachedToWindow()
UpdateClipBounds(Width, Height);
if (IsInsideSwipeRefreshLayout())
+ {
RefreshViewWebViewScrollCapture.Attach(this);
+ // If a page has already loaded before this WebView was placed inside a
+ // RefreshView (late-attach), OnPageFinished already fired with IsAttached=false
+ // and the observer was never injected. Re-inject it now so inner-scroll can
+ // correctly prevent pull-to-refresh.
+ if (!string.IsNullOrEmpty(Url))
+ RefreshViewWebViewScrollCapture.InjectObserver(this);
+ }
}
protected override void OnDetachedFromWindow()
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
index 402eb8d49a0b..872f3312a286 100644
--- a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -79,7 +79,9 @@ function report(target) {
internal static void Attach(WebView webView)
{
if (GetState(webView) is not null)
+ {
return;
+ }
var state = new ScrollCaptureState();
webView.SetTag(ScrollCaptureStateKey, state);
@@ -89,26 +91,41 @@ internal static void Attach(WebView webView)
internal static void Detach(WebView? webView)
{
if (webView is null)
+ {
return;
+ }
if (GetState(webView) is not ScrollCaptureState state)
+ {
return;
+ }
+ // Mark detached BEFORE removing the interface so any in-flight JNI
+ // callbacks to SetCanScrollUp become no-ops instead of accessing a
+ // disposed object. RemoveJavascriptInterface is async from V8's
+ // perspective — the JS bridge can still fire after this call returns.
+ state.MarkDetached();
webView.RemoveJavascriptInterface(JavaScriptInterfaceName);
webView.SetTag(ScrollCaptureStateKey, null);
- state.Dispose();
+ // Do NOT call state.Dispose() here — V8 may still hold a reference to
+ // the state object via the JS bridge. The GC will collect it once V8
+ // releases its last reference.
}
internal static void Reset(WebView? webView)
{
if (GetState(webView) is ScrollCaptureState state)
+ {
state.Reset();
+ }
}
internal static void InjectObserver(WebView? webView)
{
if (webView is null)
+ {
return;
+ }
webView.EvaluateJavascript(ObserverScript, null);
}
@@ -150,6 +167,9 @@ sealed class ScrollCaptureState : Java.Lang.Object
// and read from the UI thread, so they must be volatile to ensure visibility on ARM.
volatile bool _canScrollUp;
volatile bool _hasReportedState;
+ // Set before RemoveJavascriptInterface so any in-flight JNI callbacks become
+ // no-ops rather than accessing a disposed object.
+ volatile bool _detached;
internal bool CanScrollUp => _canScrollUp;
@@ -160,10 +180,16 @@ sealed class ScrollCaptureState : Java.Lang.Object
[Export("setCanScrollUp")]
public void SetCanScrollUp(bool canScrollUp)
{
+ if (_detached)
+ {
+ return;
+ }
_canScrollUp = canScrollUp;
_hasReportedState = true;
}
+ internal void MarkDetached() => _detached = true;
+
internal void Reset()
{
_canScrollUp = false;
From 746249131d8189a1ece1dcc7f72aef261deed157 Mon Sep 17 00:00:00 2001
From: BagavathiPerumal
Date: Wed, 6 May 2026 15:05:17 +0530
Subject: [PATCH 11/12] fix-33510-Updated the code changes, addressed the issue
in MauiHybridWebView, optimized the implementation, and resolved the related
issues as well.
---
.../src/Platform/Android/MauiHybridWebView.cs | 45 ++++++++++++++++
.../Android/MauiHybridWebViewClient.cs | 27 ++++++++++
.../Android/MauiSwipeRefreshLayout.cs | 23 +++++++--
src/Core/src/Platform/Android/MauiWebView.cs | 6 +++
.../src/Platform/Android/MauiWebViewClient.cs | 14 +++++
.../RefreshViewWebViewScrollCapture.cs | 51 ++++++++++++-------
.../net-android/PublicAPI.Unshipped.txt | 4 ++
7 files changed, 148 insertions(+), 22 deletions(-)
diff --git a/src/Core/src/Platform/Android/MauiHybridWebView.cs b/src/Core/src/Platform/Android/MauiHybridWebView.cs
index 6354bdf7abea..2a26aea9f4c4 100644
--- a/src/Core/src/Platform/Android/MauiHybridWebView.cs
+++ b/src/Core/src/Platform/Android/MauiHybridWebView.cs
@@ -36,12 +36,46 @@ protected override void OnSizeChanged(int width, int height, int oldWidth, int o
UpdateClipBounds(width, height);
}
+ // OnAttachedToWindow — calls Attach(this) when inside a SwipeRefreshLayout.
protected override void OnAttachedToWindow()
{
base.OnAttachedToWindow();
// Re-evaluate ClipBounds when re-parented (e.g., wrapped in WrapperView for shadow)
UpdateClipBounds(Width, Height);
+
+ if (IsInsideSwipeRefreshLayout())
+ {
+ RefreshViewWebViewScrollCapture.Attach(this);
+ // If a page has already loaded before this HybridWebView was placed inside a
+ // RefreshView (late-attach), the observer was never injected. Re-inject now.
+ if (!string.IsNullOrEmpty(Url))
+ {
+ RefreshViewWebViewScrollCapture.InjectObserver(this);
+ }
+ }
+ }
+
+ // OnDetachedFromWindow — calls Detach().
+ protected override void OnDetachedFromWindow()
+ {
+ RefreshViewWebViewScrollCapture.Detach(this);
+ base.OnDetachedFromWindow();
+ }
+
+ // IsInsideSwipeRefreshLayout — walks parent view tree.
+ bool IsInsideSwipeRefreshLayout()
+ {
+ var parent = Parent;
+ while (parent is not null)
+ {
+ if (parent is MauiSwipeRefreshLayout)
+ {
+ return true;
+ }
+ parent = parent.Parent;
+ }
+ return false;
}
void UpdateClipBounds(int width, int height)
@@ -77,5 +111,16 @@ public void SendRawMessage(string rawMessage)
PostWebMessage(new WebMessage(rawMessage), AndroidAppOriginUri);
#pragma warning restore CA1416 // Validate platform compatibility
}
+
+ // Dispose(bool) — calls Detach() on cleanup.
+ protected override void Dispose(bool disposing)
+ {
+ if (disposing)
+ {
+ RefreshViewWebViewScrollCapture.Detach(this);
+ }
+
+ base.Dispose(disposing);
+ }
}
}
diff --git a/src/Core/src/Platform/Android/MauiHybridWebViewClient.cs b/src/Core/src/Platform/Android/MauiHybridWebViewClient.cs
index 1c8b1bd3ce1b..c202d14e2366 100644
--- a/src/Core/src/Platform/Android/MauiHybridWebViewClient.cs
+++ b/src/Core/src/Platform/Android/MauiHybridWebViewClient.cs
@@ -7,6 +7,7 @@
using System.IO;
using System.Text;
using System.Web;
+using Android.Graphics;
using Android.Webkit;
using Java.Net;
using Microsoft.Extensions.Logging;
@@ -30,6 +31,32 @@ public MauiHybridWebViewClient(HybridWebViewHandler handler)
private HybridWebViewHandler? Handler => _handler is not null && _handler.TryGetTarget(out var h) ? h : null;
+ // OnPageStarted — calls Reset() to clear stale scroll state.
+ public override void OnPageStarted(AWebView? view, string? url, Bitmap? favicon)
+ {
+ RefreshViewWebViewScrollCapture.Reset(view);
+ base.OnPageStarted(view, url, favicon);
+ }
+
+ // OnPageFinished — calls InjectObserver() to inject JS bridge when page loads.
+ public override void OnPageFinished(AWebView? view, string? url)
+ {
+ if (string.IsNullOrWhiteSpace(url))
+ {
+ base.OnPageFinished(view, url);
+ return;
+ }
+
+ // Only inject the scroll-capture observer when the WebView is hosted inside
+ // a RefreshView – avoids unnecessary JS overhead for standalone HybridWebViews.
+ if (RefreshViewWebViewScrollCapture.IsAttached(view))
+ {
+ RefreshViewWebViewScrollCapture.InjectObserver(view);
+ }
+
+ base.OnPageFinished(view, url);
+ }
+
public override WebResourceResponse? ShouldInterceptRequest(AWebView? view, IWebResourceRequest? request)
{
var url = request?.Url?.ToString();
diff --git a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
index 14749d2d2a37..23cc46bd77d5 100644
--- a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
+++ b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
@@ -20,6 +20,7 @@ public class MauiSwipeRefreshLayout : SwipeRefreshLayout
AView? _contentView;
bool _refreshEnabled = true;
AWebView? _activeTouchWebView;
+ RefreshViewWebViewScrollCapture.ScrollCaptureState? _activeTouchScrollState;
bool _webViewOwnsGesture;
bool _touchStartedInWebView;
@@ -148,6 +149,8 @@ public override bool OnInterceptTouchEvent(MotionEvent? ev)
case MotionEventActions.Down:
_activeTouchWebView = FindWebView(_contentView, ev.GetX(), ev.GetY());
_touchStartedInWebView = _activeTouchWebView is not null;
+ // ACTION_DOWN — caches ScrollCaptureState via GetAttachedState().
+ _activeTouchScrollState = RefreshViewWebViewScrollCapture.GetAttachedState(_activeTouchWebView);
_webViewOwnsGesture = _touchStartedInWebView &&
RefreshViewWebViewScrollCapture.TryGetCanScrollUp(_activeTouchWebView, out var canScrollUpAtStart) &&
canScrollUpAtStart;
@@ -160,22 +163,34 @@ public override bool OnInterceptTouchEvent(MotionEvent? ev)
return false;
}
break;
+ case MotionEventActions.PointerDown:
+ // Reset WebView gesture ownership when a second finger is placed –
+ // multi-touch cancels the pending single-finger pull-to-refresh guard.
+ _activeTouchWebView = null;
+ _activeTouchScrollState = null;
+ _touchStartedInWebView = false;
+ _webViewOwnsGesture = false;
+ break;
case MotionEventActions.Move:
- // Re-evaluate scrollability so that once the WebView reaches the top,
- // RefreshLayout can start intercepting mid-gesture.
- if (_touchStartedInWebView && _webViewOwnsGesture && _activeTouchWebView is not null)
+ // ACTION_MOVE — reads CanScrollUp (volatile bool, zero JNI) from cached state
+ // instead of calling TryGetCanScrollUp every frame.
+ if (_touchStartedInWebView && _webViewOwnsGesture && _activeTouchScrollState is not null)
{
- if (!RefreshViewWebViewScrollCapture.TryGetCanScrollUp(_activeTouchWebView, out var canStillScrollUp) || !canStillScrollUp)
+ if (!_activeTouchScrollState.CanScrollUp)
{
_webViewOwnsGesture = false;
}
}
if (_touchStartedInWebView && _webViewOwnsGesture)
+ {
return false;
+ }
break;
case MotionEventActions.Cancel:
case MotionEventActions.Up:
+ // ACTION_UP/CANCEL — clears cached state.
_activeTouchWebView = null;
+ _activeTouchScrollState = null;
_touchStartedInWebView = false;
_webViewOwnsGesture = false;
break;
diff --git a/src/Core/src/Platform/Android/MauiWebView.cs b/src/Core/src/Platform/Android/MauiWebView.cs
index 3f21af4e6ea1..bd91d4622564 100644
--- a/src/Core/src/Platform/Android/MauiWebView.cs
+++ b/src/Core/src/Platform/Android/MauiWebView.cs
@@ -44,7 +44,9 @@ protected override void OnAttachedToWindow()
// and the observer was never injected. Re-inject it now so inner-scroll can
// correctly prevent pull-to-refresh.
if (!string.IsNullOrEmpty(Url))
+ {
RefreshViewWebViewScrollCapture.InjectObserver(this);
+ }
}
}
@@ -60,7 +62,9 @@ bool IsInsideSwipeRefreshLayout()
while (parent is not null)
{
if (parent is MauiSwipeRefreshLayout)
+ {
return true;
+ }
parent = parent.Parent;
}
return false;
@@ -119,7 +123,9 @@ void IWebViewDelegate.LoadUrl(string? url)
protected override void Dispose(bool disposing)
{
if (disposing)
+ {
RefreshViewWebViewScrollCapture.Detach(this);
+ }
base.Dispose(disposing);
}
diff --git a/src/Core/src/Platform/Android/MauiWebViewClient.cs b/src/Core/src/Platform/Android/MauiWebViewClient.cs
index a5b42375e31f..89e53e36c47b 100644
--- a/src/Core/src/Platform/Android/MauiWebViewClient.cs
+++ b/src/Core/src/Platform/Android/MauiWebViewClient.cs
@@ -25,7 +25,9 @@ public override void OnPageStarted(WebView? view, string? url, Bitmap? favicon)
RefreshViewWebViewScrollCapture.Reset(view);
if (!_handler.TryGetTarget(out var handler) || handler.VirtualView == null)
+ {
return;
+ }
if (!string.IsNullOrWhiteSpace(url))
{
@@ -61,7 +63,9 @@ public override void OnPageFinished(WebView? view, string? url)
// Skip Navigated event for about:blank to prevent unwanted events when Source is null
if (navigate && !IsBlankNavigation(url))
+ {
handler.VirtualView.Navigated(handler.CurrentNavigationEvent, GetValidUrl(url), _navigationResult);
+ }
handler.SyncPlatformCookiesToVirtualView(url);
@@ -70,7 +74,9 @@ public override void OnPageFinished(WebView? view, string? url)
// Only inject the scroll-capture observer when the WebView is hosted inside
// a RefreshView – avoids unnecessary JS overhead for standalone WebViews.
if (RefreshViewWebViewScrollCapture.IsAttached(view))
+ {
RefreshViewWebViewScrollCapture.InjectObserver(view);
+ }
base.OnPageFinished(view, url);
}
@@ -83,7 +89,9 @@ public override void OnReceivedError(WebView? view, IWebResourceRequest? request
_navigationResult = WebNavigationResult.Failure;
if (error?.ErrorCode == ClientError.Timeout)
+ {
_navigationResult = WebNavigationResult.Timeout;
+ }
}
base.OnReceivedError(view, request, error);
@@ -109,7 +117,9 @@ static bool IsBlankNavigation(string? url)
// Null/empty URLs are handled by the early return in OnPageFinished,
// so we only need to check for the explicit "about:blank" URL
if (string.IsNullOrWhiteSpace(url))
+ {
return false;
+ }
// Check if URL is about:blank (case insensitive)
return string.Equals(url.Trim(), "about:blank", StringComparison.OrdinalIgnoreCase);
@@ -118,7 +128,9 @@ static bool IsBlankNavigation(string? url)
static string GetValidUrl(string? url)
{
if (string.IsNullOrEmpty(url))
+ {
return string.Empty;
+ }
return url;
}
@@ -126,7 +138,9 @@ static string GetValidUrl(string? url)
protected override void Dispose(bool disposing)
{
if (disposing)
+ {
Disconnect();
+ }
base.Dispose(disposing);
}
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
index 872f3312a286..f9b03a4dfade 100644
--- a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -9,20 +9,19 @@ internal static class RefreshViewWebViewScrollCapture
const string JavaScriptInterfaceName = "mauiRefreshViewHost";
const int ScrollCaptureStateKey = 0x4D415549;
+ // Observer JS script — replaces static boolean guard (__mauiRefreshViewObserverInstalled) with
+ // dynamic window.mauiRefreshViewHost lookup inside report(). Fixes scroll tracking after Shell tab-switch.
+ //
+ // After a Shell tab-switch the WebView is detached (Detach removes the old bridge) then re-attached
+ // (Attach adds a fresh ScrollCaptureState, InjectObserver re-runs this script). A static guard would
+ // prevent re-injection, so the new bridge object would never receive callbacks, silently breaking
+ // pull-to-refresh protection until the next full page reload.
+ //
+ // Named global JS listener vars (window.__mauiTouchStartHandler/MoveHandler) so removeEventListener
+ // works on re-inject, preventing listener stacking across Detach/re-Attach cycles.
const string ObserverScript =
"""
(function () {
- if (window.__mauiRefreshViewObserverInstalled) {
- return;
- }
-
- var host = window.mauiRefreshViewHost;
- if (!host || typeof host.setCanScrollUp !== 'function') {
- return;
- }
-
- window.__mauiRefreshViewObserverInstalled = true;
-
function isScrollableElement(node) {
if (!node || node.nodeType !== Node.ELEMENT_NODE) {
return false;
@@ -58,19 +57,30 @@ function getScrollTopForElement(element) {
function report(target) {
try {
+ var host = window.mauiRefreshViewHost;
+ if (!host || typeof host.setCanScrollUp !== 'function') {
+ return;
+ }
var scrollable = getScrollableElement(target);
host.setCanScrollUp(getScrollTopForElement(scrollable) > 0);
} catch (e) {
}
}
- document.addEventListener('touchstart', function (event) {
- report(event.target);
- }, true);
+ // Remove any previously installed listeners to prevent accumulation
+ // after Shell tab-switch (detach + re-attach without page reload).
+ if (window.__mauiTouchStartHandler) {
+ document.removeEventListener('touchstart', window.__mauiTouchStartHandler, true);
+ document.removeEventListener('touchmove', window.__mauiTouchMoveHandler, true);
+ }
- document.addEventListener('touchmove', function (event) {
- report(event.target);
- }, true);
+ var touchStartHandler = function (event) { report(event.target); };
+ var touchMoveHandler = function (event) { report(event.target); };
+ window.__mauiTouchStartHandler = touchStartHandler;
+ window.__mauiTouchMoveHandler = touchMoveHandler;
+
+ document.addEventListener('touchstart', touchStartHandler, true);
+ document.addEventListener('touchmove', touchMoveHandler, true);
report(document.body);
})();
@@ -161,7 +171,12 @@ internal static bool TryGetCanScrollUp(WebView? webView, out bool canScrollUp)
static ScrollCaptureState? GetState(WebView? webView) =>
webView?.GetTag(ScrollCaptureStateKey) as ScrollCaptureState;
- sealed class ScrollCaptureState : Java.Lang.Object
+ // Returns the cached ScrollCaptureState for the given WebView so callers on the
+ // UI thread can read CanScrollUp (a volatile bool) directly without any JNI overhead.
+ // Returns null when the WebView is not inside a RefreshView.
+ internal static ScrollCaptureState? GetAttachedState(WebView? webView) => GetState(webView);
+
+ internal sealed class ScrollCaptureState : Java.Lang.Object
{
// These fields are written from the JavaBridge thread (via [JavascriptInterface])
// and read from the UI thread, so they must be volatile to ensure visibility on ARM.
diff --git a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
index 44cc4b54cd5f..db676dbda3ad 100644
--- a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
+++ b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
@@ -13,7 +13,11 @@ override Microsoft.Maui.Platform.ContentViewGroup.HasOverlappingRendering.get ->
override Microsoft.Maui.Platform.LayoutViewGroup.HasOverlappingRendering.get -> bool
override Microsoft.Maui.Platform.WrapperView.HasOverlappingRendering.get -> bool
override Microsoft.Maui.Platform.MauiHybridWebView.OnAttachedToWindow() -> void
+override Microsoft.Maui.Platform.MauiHybridWebView.OnDetachedFromWindow() -> void
+override Microsoft.Maui.Platform.MauiHybridWebView.Dispose(bool disposing) -> void
override Microsoft.Maui.Platform.MauiHybridWebView.OnSizeChanged(int width, int height, int oldWidth, int oldHeight) -> void
+override Microsoft.Maui.Platform.MauiHybridWebViewClient.OnPageFinished(Android.Webkit.WebView? view, string? url) -> void
+override Microsoft.Maui.Platform.MauiHybridWebViewClient.OnPageStarted(Android.Webkit.WebView? view, string? url, Android.Graphics.Bitmap? favicon) -> void
override Microsoft.Maui.Platform.MauiWebView.OnAttachedToWindow() -> void
override Microsoft.Maui.Platform.MauiWebView.OnSizeChanged(int width, int height, int oldWidth, int oldHeight) -> void
override Microsoft.Maui.Platform.MauiSwipeRefreshLayout.OnInterceptTouchEvent(Android.Views.MotionEvent? ev) -> bool
From 3c98c02d4db5e0ed3f5fd89e0db787cd8de83094 Mon Sep 17 00:00:00 2001
From: BagavathiPerumal
Date: Tue, 12 May 2026 17:11:30 +0530
Subject: [PATCH 12/12] fix-33510-Optimized code implementation
---
.../src/Platform/Android/MauiHybridWebView.cs | 17 +----------------
src/Core/src/Platform/Android/MauiWebView.cs | 16 +---------------
.../Android/RefreshViewWebViewScrollCapture.cs | 14 ++++++++++++++
3 files changed, 16 insertions(+), 31 deletions(-)
diff --git a/src/Core/src/Platform/Android/MauiHybridWebView.cs b/src/Core/src/Platform/Android/MauiHybridWebView.cs
index 2a26aea9f4c4..74518c09cd1e 100644
--- a/src/Core/src/Platform/Android/MauiHybridWebView.cs
+++ b/src/Core/src/Platform/Android/MauiHybridWebView.cs
@@ -44,7 +44,7 @@ protected override void OnAttachedToWindow()
// Re-evaluate ClipBounds when re-parented (e.g., wrapped in WrapperView for shadow)
UpdateClipBounds(Width, Height);
- if (IsInsideSwipeRefreshLayout())
+ if (RefreshViewWebViewScrollCapture.IsInsideMauiSwipeRefreshLayout(this))
{
RefreshViewWebViewScrollCapture.Attach(this);
// If a page has already loaded before this HybridWebView was placed inside a
@@ -63,21 +63,6 @@ protected override void OnDetachedFromWindow()
base.OnDetachedFromWindow();
}
- // IsInsideSwipeRefreshLayout — walks parent view tree.
- bool IsInsideSwipeRefreshLayout()
- {
- var parent = Parent;
- while (parent is not null)
- {
- if (parent is MauiSwipeRefreshLayout)
- {
- return true;
- }
- parent = parent.Parent;
- }
- return false;
- }
-
void UpdateClipBounds(int width, int height)
{
if (width > 0 && height > 0)
diff --git a/src/Core/src/Platform/Android/MauiWebView.cs b/src/Core/src/Platform/Android/MauiWebView.cs
index bd91d4622564..bd3f41402f31 100644
--- a/src/Core/src/Platform/Android/MauiWebView.cs
+++ b/src/Core/src/Platform/Android/MauiWebView.cs
@@ -36,7 +36,7 @@ protected override void OnAttachedToWindow()
// Re-evaluate ClipBounds when re-parented (e.g., wrapped in WrapperView for shadow)
UpdateClipBounds(Width, Height);
- if (IsInsideSwipeRefreshLayout())
+ if (RefreshViewWebViewScrollCapture.IsInsideMauiSwipeRefreshLayout(this))
{
RefreshViewWebViewScrollCapture.Attach(this);
// If a page has already loaded before this WebView was placed inside a
@@ -56,20 +56,6 @@ protected override void OnDetachedFromWindow()
base.OnDetachedFromWindow();
}
- bool IsInsideSwipeRefreshLayout()
- {
- var parent = Parent;
- while (parent is not null)
- {
- if (parent is MauiSwipeRefreshLayout)
- {
- return true;
- }
- parent = parent.Parent;
- }
- return false;
- }
-
void UpdateClipBounds(int width, int height)
{
if (width > 0 && height > 0)
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
index f9b03a4dfade..ac2ab4b54da7 100644
--- a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -142,6 +142,20 @@ internal static void InjectObserver(WebView? webView)
internal static bool IsAttached(WebView? webView) => GetState(webView) is not null;
+ internal static bool IsInsideMauiSwipeRefreshLayout(WebView webView)
+ {
+ var parent = webView.Parent;
+ while (parent is not null)
+ {
+ if (parent is MauiSwipeRefreshLayout)
+ {
+ return true;
+ }
+ parent = parent.Parent;
+ }
+ return false;
+ }
+
internal static bool TryGetCanScrollUp(WebView? webView, out bool canScrollUp)
{
if (webView is null)
Scrollable row {index}
"));
+
+ var html = new StringBuilder();
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("Internal scroll container
");
+ html.AppendLine("");
+ html.AppendLine("
");
+ html.AppendLine("");
+ html.AppendLine("");
+ html.AppendLine("");
+
+ return html.ToString();
+ }
+}
diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
new file mode 100644
index 000000000000..8aa2e6ea261c
--- /dev/null
+++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
@@ -0,0 +1,128 @@
+#if ANDROID // This test is Android-only because Issue #33510 (RefreshView triggering pull-to-refresh when scrolling inside a WebView) only reproduces on Android, and the test uses Android-specific Appium touch gesture APIs.
+using System.Globalization;
+using System.Text.RegularExpressions;
+using NUnit.Framework;
+using OpenQA.Selenium.Appium.Interactions;
+using OpenQA.Selenium.Interactions;
+using UITest.Appium;
+using UITest.Core;
+
+namespace Microsoft.Maui.TestCases.Tests.Issues;
+
+[Category(UITestCategories.RefreshView)]
+public class Issue33510 : _IssuesUITest
+{
+ const string StatusLabel = "StatusLabel";
+ const string ScrollWebViewButton = "ScrollWebViewButton";
+ const string ScrollWebViewToTopButton = "ScrollWebViewToTopButton";
+ const string ReadScrollTopButton = "ReadScrollTopButton";
+ const string ScrollTopLabel = "ScrollTopLabel";
+ const string TestWebViewContainer = "TestWebViewContainer";
+
+ public Issue33510(TestDevice device) : base(device)
+ {
+ }
+
+ public override string Issue => "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView";
+
+ protected override bool ResetAfterEachTest => true;
+
+ [Test]
+ public void PullToRefreshWaitsUntilInternalWebViewContainerReachesTop()
+ {
+ var androidApp = WaitForAndroidApp();
+ var webViewRect = App.WaitForElement(TestWebViewContainer).GetRect();
+ var x = webViewRect.CenterX();
+ var upwardFromY = webViewRect.Y + (webViewRect.Height * 75 / 100);
+ var upwardToY = webViewRect.Y + (webViewRect.Height * 30 / 100);
+
+ for (var attempt = 0; attempt < 3 && GetScrollTop() <= 200; attempt++)
+ {
+ DragInsideWebView(androidApp, x, upwardFromY, x, upwardToY);
+ }
+
+ var initialScrollTop = GetScrollTop();
+ Assert.That(initialScrollTop, Is.GreaterThan(200), "The inner HTML container must start away from the top.");
+
+ Assert.That(GetStatus(), Does.Not.Contain("Refresh triggered"));
+
+ var fromY = webViewRect.Y + (webViewRect.Height * 35 / 100);
+ var toY = webViewRect.Y + (webViewRect.Height * 70 / 100);
+
+ DragInsideWebView(androidApp, x, fromY, x, toY);
+
+ var scrollTopAfterGesture = GetScrollTop();
+ Assert.That(scrollTopAfterGesture, Is.LessThan(initialScrollTop), "Dragging down inside the WebView should scroll the inner container upward.");
+ Assert.That(scrollTopAfterGesture, Is.GreaterThan(0), "The inner container should still be away from the top after a partial upward scroll.");
+ Assert.That(GetStatus(), Does.Not.Contain("Refresh triggered"));
+ }
+
+ [Test]
+ public void PullToRefreshStillWorksWhenInternalWebViewContainerStartsAtTop()
+ {
+ var androidApp = WaitForAndroidApp();
+
+ Assert.That(GetScrollTop(), Is.LessThan(1), "The inner HTML container should start at the top before pull-to-refresh begins.");
+
+ var webViewRect = App.WaitForElement(TestWebViewContainer).GetRect();
+ var x = webViewRect.CenterX();
+ var fromY = webViewRect.Y + (webViewRect.Height * 30 / 100);
+ var toY = webViewRect.Y + (webViewRect.Height * 85 / 100);
+
+ DragInsideWebView(androidApp, x, fromY, x, toY);
+
+ Assert.That(
+ App.WaitForTextToBePresentInElement(StatusLabel, "Refresh triggered", timeout: TimeSpan.FromSeconds(10)),
+ Is.True,
+ "Pulling down inside the WebView at scroll top should still trigger RefreshView.");
+ }
+
+ AppiumAndroidApp WaitForAndroidApp()
+ {
+ if (App is not AppiumAndroidApp androidApp)
+ {
+ Assert.Ignore("Issue #33510 is Android-specific.");
+ return null!;
+ }
+
+ Assert.That(
+ App.WaitForTextToBePresentInElement(StatusLabel, "WebView ready", timeout: TimeSpan.FromSeconds(30)),
+ Is.True,
+ "WebView never finished loading.");
+
+ return androidApp;
+ }
+
+ double GetScrollTop()
+ {
+ App.Tap(ReadScrollTopButton);
+ Assert.That(
+ App.WaitForTextToBePresentInElement(ScrollTopLabel, "ScrollTop:", timeout: TimeSpan.FromSeconds(5)),
+ Is.True,
+ "Scroll position was not reported.");
+
+ var status = App.FindElement(ScrollTopLabel).GetText() ?? string.Empty;
+ var match = Regex.Match(status, @"(\d+(\.\d+)?)");
+
+ Assert.That(match.Success, Is.True, $"Could not parse scroll position from '{status}'.");
+
+ return double.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture);
+ }
+
+ string GetStatus() =>
+ App.FindElement(StatusLabel).GetText() ?? string.Empty;
+
+ static void DragInsideWebView(AppiumAndroidApp androidApp, int fromX, int fromY, int toX, int toY)
+ {
+ var touchDevice = new OpenQA.Selenium.Appium.Interactions.PointerInputDevice(PointerKind.Touch);
+ var dragSequence = new ActionSequence(touchDevice, 0);
+
+ dragSequence.AddAction(touchDevice.CreatePointerMove(CoordinateOrigin.Viewport, fromX, fromY, TimeSpan.Zero));
+ dragSequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
+ dragSequence.AddAction(touchDevice.CreatePointerMove(CoordinateOrigin.Viewport, toX, toY, TimeSpan.FromMilliseconds(450)));
+ dragSequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
+
+ androidApp.Driver.PerformActions([dragSequence]);
+ }
+}
+#endif
diff --git a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
index 1d2553dde335..284ecb5270de 100644
--- a/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
+++ b/src/Core/src/Platform/Android/MauiSwipeRefreshLayout.cs
@@ -19,6 +19,9 @@ public class MauiSwipeRefreshLayout : SwipeRefreshLayout
readonly Context _context;
AView? _contentView;
bool _refreshEnabled = true;
+ AWebView? _activeTouchWebView;
+ bool _webViewOwnsGesture;
+ bool _touchStartedInWebView;
public MauiSwipeRefreshLayout(Context context) : base(context)
{
@@ -135,6 +138,36 @@ public override bool CanChildScrollUp()
return CanScrollUp(_contentView);
}
+ public override bool OnInterceptTouchEvent(MotionEvent? ev)
+ {
+ if (ev is null)
+ return false;
+
+ switch (ev.ActionMasked)
+ {
+ case MotionEventActions.Down:
+ _activeTouchWebView = FindWebView(_contentView, ev.GetX(), ev.GetY());
+ _touchStartedInWebView = _activeTouchWebView is not null;
+ _webViewOwnsGesture = _touchStartedInWebView &&
+ RefreshViewWebViewScrollCapture.TryGetCanScrollUp(_activeTouchWebView, out var canScrollUpAtStart) &&
+ canScrollUpAtStart;
+ break;
+ case MotionEventActions.Cancel:
+ case MotionEventActions.Up:
+ _activeTouchWebView = null;
+ _touchStartedInWebView = false;
+ _webViewOwnsGesture = false;
+ break;
+ }
+
+ if (_touchStartedInWebView && _webViewOwnsGesture)
+ {
+ return false;
+ }
+
+ return base.OnInterceptTouchEvent(ev);
+ }
+
bool CanScrollUp(AView? view)
{
if (!(view is ViewGroup viewGroup))
@@ -182,9 +215,36 @@ static bool CanScrollUpViewByType(AView? view)
#pragma warning restore XAOBS001 // Obsolete
if (view is AWebView webView)
- return webView.ScrollY > 0;
+ return RefreshViewWebViewScrollCapture.TryGetCanScrollUp(webView, out var canScrollUp) && canScrollUp;
return true;
}
+
+ static AWebView? FindWebView(AView? view, float x, float y)
+ {
+ if (view is null || view.Visibility != ViewStates.Visible)
+ return null;
+
+ if (x < view.Left || x > view.Right || y < view.Top || y > view.Bottom)
+ return null;
+
+ if (view is AWebView)
+ return (AWebView)view;
+
+ if (view is not ViewGroup viewGroup)
+ return null;
+
+ var localX = x - view.Left;
+ var localY = y - view.Top;
+
+ for (int i = viewGroup.ChildCount - 1; i >= 0; i--)
+ {
+ var webView = FindWebView(viewGroup.GetChildAt(i), localX, localY);
+ if (webView is not null)
+ return webView;
+ }
+
+ return null;
+ }
}
}
diff --git a/src/Core/src/Platform/Android/MauiWebView.cs b/src/Core/src/Platform/Android/MauiWebView.cs
index 7e9019100caf..13b2c3621d8a 100644
--- a/src/Core/src/Platform/Android/MauiWebView.cs
+++ b/src/Core/src/Platform/Android/MauiWebView.cs
@@ -35,6 +35,27 @@ protected override void OnAttachedToWindow()
// Re-evaluate ClipBounds when re-parented (e.g., wrapped in WrapperView for shadow)
UpdateClipBounds(Width, Height);
+
+ if (IsInsideSwipeRefreshLayout())
+ RefreshViewWebViewScrollCapture.Attach(this);
+ }
+
+ protected override void OnDetachedFromWindow()
+ {
+ RefreshViewWebViewScrollCapture.Detach(this);
+ base.OnDetachedFromWindow();
+ }
+
+ bool IsInsideSwipeRefreshLayout()
+ {
+ var parent = Parent;
+ while (parent is not null)
+ {
+ if (parent is MauiSwipeRefreshLayout)
+ return true;
+ parent = parent.Parent;
+ }
+ return false;
}
void UpdateClipBounds(int width, int height)
@@ -86,5 +107,13 @@ void IWebViewDelegate.LoadUrl(string? url)
LoadUrl(url ?? string.Empty);
}
}
+
+ protected override void Dispose(bool disposing)
+ {
+ if (disposing)
+ RefreshViewWebViewScrollCapture.Detach(this);
+
+ base.Dispose(disposing);
+ }
}
}
\ No newline at end of file
diff --git a/src/Core/src/Platform/Android/MauiWebViewClient.cs b/src/Core/src/Platform/Android/MauiWebViewClient.cs
index b24f3ab6aa13..abaac0f770ab 100644
--- a/src/Core/src/Platform/Android/MauiWebViewClient.cs
+++ b/src/Core/src/Platform/Android/MauiWebViewClient.cs
@@ -22,6 +22,8 @@ public override bool ShouldOverrideUrlLoading(WebView? view, IWebResourceRequest
public override void OnPageStarted(WebView? view, string? url, Bitmap? favicon)
{
+ RefreshViewWebViewScrollCapture.Reset(view);
+
if (!_handler.TryGetTarget(out var handler) || handler.VirtualView == null)
return;
@@ -64,6 +66,7 @@ public override void OnPageFinished(WebView? view, string? url)
handler.SyncPlatformCookiesToVirtualView(url);
handler?.PlatformView.UpdateCanGoBackForward(handler.VirtualView);
+ RefreshViewWebViewScrollCapture.InjectObserver(view);
base.OnPageFinished(view, url);
}
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
new file mode 100644
index 000000000000..83497c730377
--- /dev/null
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -0,0 +1,166 @@
+using Android.Webkit;
+using Java.Interop;
+using System.Diagnostics.CodeAnalysis;
+
+namespace Microsoft.Maui.Platform;
+
+internal static class RefreshViewWebViewScrollCapture
+{
+ const string JavaScriptInterfaceName = "mauiRefreshViewHost";
+ const int ScrollCaptureStateKey = 0x4D415549;
+
+ const string ObserverScript =
+ """
+ (function () {
+ if (window.__mauiRefreshViewObserverInstalled) {
+ return;
+ }
+
+ var host = window.mauiRefreshViewHost;
+ if (!host || typeof host.setCanScrollUp !== 'function') {
+ return;
+ }
+
+ window.__mauiRefreshViewObserverInstalled = true;
+
+ function isScrollableElement(node) {
+ if (!node || node.nodeType !== Node.ELEMENT_NODE) {
+ return false;
+ }
+
+ var style = window.getComputedStyle(node);
+ var overflowY = style ? style.overflowY : '';
+ return (overflowY === 'auto' || overflowY === 'scroll' || overflowY === 'overlay') &&
+ node.scrollHeight > node.clientHeight + 1;
+ }
+
+ function getScrollableElement(startNode) {
+ for (var node = startNode; node && node.nodeType === Node.ELEMENT_NODE; node = node.parentElement) {
+ if (isScrollableElement(node)) {
+ return node;
+ }
+ }
+
+ return document.scrollingElement || document.documentElement || document.body;
+ }
+
+ function getScrollTopForElement(element) {
+ if (!element) {
+ return 0;
+ }
+
+ if (element === document.body || element === document.documentElement || element === document.scrollingElement) {
+ return window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop || 0;
+ }
+
+ return element.scrollTop || 0;
+ }
+
+ function report(target) {
+ try {
+ var scrollable = getScrollableElement(target);
+ host.setCanScrollUp(getScrollTopForElement(scrollable) > 0);
+ } catch (e) {
+ }
+ }
+
+ document.addEventListener('touchstart', function (event) {
+ report(event.target);
+ }, true);
+
+ document.addEventListener('touchmove', function (event) {
+ report(event.target);
+ }, true);
+
+ report(document.body);
+ })();
+ """;
+
+ internal static void Attach(WebView webView)
+ {
+ if (GetState(webView) is not null)
+ return;
+
+ var state = new ScrollCaptureState();
+ webView.SetTag(ScrollCaptureStateKey, state);
+ webView.AddJavascriptInterface(state, JavaScriptInterfaceName);
+ }
+
+ internal static void Detach(WebView? webView)
+ {
+ if (webView is null)
+ return;
+
+ if (GetState(webView) is not ScrollCaptureState state)
+ return;
+
+ webView.RemoveJavascriptInterface(JavaScriptInterfaceName);
+ webView.SetTag(ScrollCaptureStateKey, null);
+ state.Dispose();
+ }
+
+ internal static void Reset(WebView? webView)
+ {
+ if (GetState(webView) is ScrollCaptureState state)
+ state.Reset();
+ }
+
+ internal static void InjectObserver(WebView? webView)
+ {
+ if (webView is null)
+ return;
+
+ webView.EvaluateJavascript(ObserverScript, null);
+ }
+
+ internal static bool TryGetCanScrollUp(WebView? webView, out bool canScrollUp)
+ {
+ if (webView is null)
+ {
+ canScrollUp = false;
+ return false;
+ }
+
+ var nativeCanScrollUp = webView.CanScrollVertically(-1) || webView.ScrollY > 0;
+
+ if (GetState(webView) is ScrollCaptureState state && state.HasReportedState)
+ {
+ canScrollUp = state.CanScrollUp || nativeCanScrollUp;
+ return true;
+ }
+
+ if (nativeCanScrollUp)
+ {
+ canScrollUp = true;
+ return true;
+ }
+
+ canScrollUp = false;
+ return false;
+ }
+
+ static ScrollCaptureState? GetState(WebView? webView) =>
+ webView?.GetTag(ScrollCaptureStateKey) as ScrollCaptureState;
+
+ sealed class ScrollCaptureState : Java.Lang.Object
+ {
+ internal bool CanScrollUp { get; private set; }
+
+ internal bool HasReportedState { get; private set; }
+
+ [JavascriptInterface]
+ [RequiresUnreferencedCode("Java.Interop.Export uses dynamic features.")]
+ [Export("setCanScrollUp")]
+ public void SetCanScrollUp(bool canScrollUp)
+ {
+ CanScrollUp = canScrollUp;
+ HasReportedState = true;
+ }
+
+ internal void Reset()
+ {
+ CanScrollUp = false;
+ HasReportedState = false;
+ }
+ }
+}
diff --git a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
index 59fed41b6da4..44cc4b54cd5f 100644
--- a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
+++ b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
@@ -16,3 +16,6 @@ override Microsoft.Maui.Platform.MauiHybridWebView.OnAttachedToWindow() -> void
override Microsoft.Maui.Platform.MauiHybridWebView.OnSizeChanged(int width, int height, int oldWidth, int oldHeight) -> void
override Microsoft.Maui.Platform.MauiWebView.OnAttachedToWindow() -> void
override Microsoft.Maui.Platform.MauiWebView.OnSizeChanged(int width, int height, int oldWidth, int oldHeight) -> void
+override Microsoft.Maui.Platform.MauiSwipeRefreshLayout.OnInterceptTouchEvent(Android.Views.MotionEvent? ev) -> bool
+override Microsoft.Maui.Platform.MauiWebView.Dispose(bool disposing) -> void
+override Microsoft.Maui.Platform.MauiWebView.OnDetachedFromWindow() -> void
From ae07f1e9790021c805ad5c9750b84fb4deebca84 Mon Sep 17 00:00:00 2001
From: BagavathiPerumal Top marker
");
+ html.AppendLine(rows);
+ html.AppendLine("Scrollable row {index}
"));
-
- var html = new StringBuilder();
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("Internal scroll container
");
- html.AppendLine("");
- html.AppendLine("
");
- html.AppendLine("");
- html.AppendLine("");
- html.AppendLine("");
-
- return html.ToString();
- }
}
diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
index 8aa2e6ea261c..6f6622188791 100644
--- a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
+++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
@@ -1,6 +1,4 @@
#if ANDROID // This test is Android-only because Issue #33510 (RefreshView triggering pull-to-refresh when scrolling inside a WebView) only reproduces on Android, and the test uses Android-specific Appium touch gesture APIs.
-using System.Globalization;
-using System.Text.RegularExpressions;
using NUnit.Framework;
using OpenQA.Selenium.Appium.Interactions;
using OpenQA.Selenium.Interactions;
@@ -13,11 +11,7 @@ namespace Microsoft.Maui.TestCases.Tests.Issues;
public class Issue33510 : _IssuesUITest
{
const string StatusLabel = "StatusLabel";
- const string ScrollWebViewButton = "ScrollWebViewButton";
- const string ScrollWebViewToTopButton = "ScrollWebViewToTopButton";
- const string ReadScrollTopButton = "ReadScrollTopButton";
- const string ScrollTopLabel = "ScrollTopLabel";
- const string TestWebViewContainer = "TestWebViewContainer";
+ const string TestRefreshView = "TestRefreshView";
public Issue33510(TestDevice device) : base(device)
{
@@ -25,56 +19,31 @@ public Issue33510(TestDevice device) : base(device)
public override string Issue => "[Android] RefreshView triggers pull-to-refresh immediately when scrolling up inside a WebView";
- protected override bool ResetAfterEachTest => true;
-
[Test]
- public void PullToRefreshWaitsUntilInternalWebViewContainerReachesTop()
+ public void PullToRefreshShouldNotTriggerWhenWebViewIsScrolledDown()
{
+ VerifyInternetConnectivity();
var androidApp = WaitForAndroidApp();
- var webViewRect = App.WaitForElement(TestWebViewContainer).GetRect();
- var x = webViewRect.CenterX();
- var upwardFromY = webViewRect.Y + (webViewRect.Height * 75 / 100);
- var upwardToY = webViewRect.Y + (webViewRect.Height * 30 / 100);
+ var refreshViewRect = App.WaitForElement(TestRefreshView).GetRect();
+ var x = refreshViewRect.CenterX();
- for (var attempt = 0; attempt < 3 && GetScrollTop() <= 200; attempt++)
+ // Scroll content down by swiping up inside the WebView
+ for (var i = 0; i < 3; i++)
{
- DragInsideWebView(androidApp, x, upwardFromY, x, upwardToY);
+ DragInsideWebView(androidApp, x,
+ refreshViewRect.Y + (refreshViewRect.Height * 70 / 100),
+ x,
+ refreshViewRect.Y + (refreshViewRect.Height * 25 / 100));
}
- var initialScrollTop = GetScrollTop();
- Assert.That(initialScrollTop, Is.GreaterThan(200), "The inner HTML container must start away from the top.");
-
- Assert.That(GetStatus(), Does.Not.Contain("Refresh triggered"));
-
- var fromY = webViewRect.Y + (webViewRect.Height * 35 / 100);
- var toY = webViewRect.Y + (webViewRect.Height * 70 / 100);
-
- DragInsideWebView(androidApp, x, fromY, x, toY);
-
- var scrollTopAfterGesture = GetScrollTop();
- Assert.That(scrollTopAfterGesture, Is.LessThan(initialScrollTop), "Dragging down inside the WebView should scroll the inner container upward.");
- Assert.That(scrollTopAfterGesture, Is.GreaterThan(0), "The inner container should still be away from the top after a partial upward scroll.");
- Assert.That(GetStatus(), Does.Not.Contain("Refresh triggered"));
- }
-
- [Test]
- public void PullToRefreshStillWorksWhenInternalWebViewContainerStartsAtTop()
- {
- var androidApp = WaitForAndroidApp();
-
- Assert.That(GetScrollTop(), Is.LessThan(1), "The inner HTML container should start at the top before pull-to-refresh begins.");
-
- var webViewRect = App.WaitForElement(TestWebViewContainer).GetRect();
- var x = webViewRect.CenterX();
- var fromY = webViewRect.Y + (webViewRect.Height * 30 / 100);
- var toY = webViewRect.Y + (webViewRect.Height * 85 / 100);
+ // Attempt pull-to-refresh (drag down) while content is still scrolled down
+ DragInsideWebView(androidApp, x,
+ refreshViewRect.Y + (refreshViewRect.Height * 30 / 100),
+ x,
+ refreshViewRect.Y + (refreshViewRect.Height * 80 / 100));
- DragInsideWebView(androidApp, x, fromY, x, toY);
-
- Assert.That(
- App.WaitForTextToBePresentInElement(StatusLabel, "Refresh triggered", timeout: TimeSpan.FromSeconds(10)),
- Is.True,
- "Pulling down inside the WebView at scroll top should still trigger RefreshView.");
+ Assert.That(App.FindElement(StatusLabel).GetText(), Does.Not.Contain("Refresh triggered"),
+ "RefreshView should not trigger while WebView content is not at the top.");
}
AppiumAndroidApp WaitForAndroidApp()
@@ -93,25 +62,6 @@ AppiumAndroidApp WaitForAndroidApp()
return androidApp;
}
- double GetScrollTop()
- {
- App.Tap(ReadScrollTopButton);
- Assert.That(
- App.WaitForTextToBePresentInElement(ScrollTopLabel, "ScrollTop:", timeout: TimeSpan.FromSeconds(5)),
- Is.True,
- "Scroll position was not reported.");
-
- var status = App.FindElement(ScrollTopLabel).GetText() ?? string.Empty;
- var match = Regex.Match(status, @"(\d+(\.\d+)?)");
-
- Assert.That(match.Success, Is.True, $"Could not parse scroll position from '{status}'.");
-
- return double.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture);
- }
-
- string GetStatus() =>
- App.FindElement(StatusLabel).GetText() ?? string.Empty;
-
static void DragInsideWebView(AppiumAndroidApp androidApp, int fromX, int fromY, int toX, int toY)
{
var touchDevice = new OpenQA.Selenium.Appium.Interactions.PointerInputDevice(PointerKind.Touch);
diff --git a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
index 4cfa7b92f5fd..402eb8d49a0b 100644
--- a/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
+++ b/src/Core/src/Platform/Android/RefreshViewWebViewScrollCapture.cs
@@ -146,23 +146,28 @@ internal static bool TryGetCanScrollUp(WebView? webView, out bool canScrollUp)
sealed class ScrollCaptureState : Java.Lang.Object
{
- internal bool CanScrollUp { get; private set; }
+ // These fields are written from the JavaBridge thread (via [JavascriptInterface])
+ // and read from the UI thread, so they must be volatile to ensure visibility on ARM.
+ volatile bool _canScrollUp;
+ volatile bool _hasReportedState;
- internal bool HasReportedState { get; private set; }
+ internal bool CanScrollUp => _canScrollUp;
+
+ internal bool HasReportedState => _hasReportedState;
[JavascriptInterface]
[RequiresUnreferencedCode("Java.Interop.Export uses dynamic features.")]
[Export("setCanScrollUp")]
public void SetCanScrollUp(bool canScrollUp)
{
- CanScrollUp = canScrollUp;
- HasReportedState = true;
+ _canScrollUp = canScrollUp;
+ _hasReportedState = true;
}
internal void Reset()
{
- CanScrollUp = false;
- HasReportedState = false;
+ _canScrollUp = false;
+ _hasReportedState = false;
}
}
}
From 62ecbf58d939c197d430c69c8953ded7c4c6499d Mon Sep 17 00:00:00 2001
From: BagavathiPerumal Top marker
");
- html.AppendLine(rows);
- html.AppendLine("
+
+
+
+ """;
}
diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
index 6f6622188791..f33deebba16d 100644
--- a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
+++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33510.cs
@@ -22,7 +22,6 @@ public Issue33510(TestDevice device) : base(device)
[Test]
public void PullToRefreshShouldNotTriggerWhenWebViewIsScrolledDown()
{
- VerifyInternetConnectivity();
var androidApp = WaitForAndroidApp();
var refreshViewRect = App.WaitForElement(TestRefreshView).GetRect();
var x = refreshViewRect.CenterX();
From 3ed45d7833dc1256344e9d39c8f970995a6a852a Mon Sep 17 00:00:00 2001
From: BagavathiPerumal Issue33510 Internal Scroll Host
+
+
+ Content 01
+ Content 02
+ Content 03
+ Content 04
+ Content 05
+ Content 06
+ Content 07
+ Content 08
+ Content 09
+ Content 10
+ Content 11
+ Content 12
+ Content 13
+ Content 14
+ Content 15
+ Content 16
+ Content 17
+ Content 18
+ Content 19
+ Content 20
+