fix: Optimize alert operations - #1193
Conversation
|
@Dan-Maor Could you please help testing this PR? |
There was a problem hiding this comment.
Pull request overview
Reworks FBAlert alert detection and interaction to reduce accessibility round trips by using snapshot-based traversal, and updates alert APIs/callers to use typed exceptions (instead of BOOL + NSError**) so routing can map failures to appropriate HTTP statuses.
Changes:
- Replaces query-based alert discovery with a single application snapshot walk (including improved Alert/Sheet/ScrollView prioritization and Safari web-alert handling).
- Updates alert interaction APIs (
accept/dismiss/clickAlertButton:/typeText:) tovoidmethods that throw typed exceptions; wires them intoFBExceptionHandler. - Updates routing, monitors, pasteboard handling, and integration tests to the new throwing API.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| WebDriverAgentTests/IntegrationTests/FBSafariAlertTests.m | Updates Safari alert test to use XCTAssertNoThrow([alert accept]). |
| WebDriverAgentTests/IntegrationTests/FBIntegrationTestCase.m | Updates clearAlert to call throwing dismiss and ignore “no alert” via exception. |
| WebDriverAgentTests/IntegrationTests/FBAlertTests.m | Updates alert tests for throwing alert APIs and removes direct alertElement test. |
| WebDriverAgentLib/Utilities/FBPasteboard.m | Switches pasteboard alert handling to use FBAlert alertWithApplication: + throwing accept. |
| WebDriverAgentLib/Utilities/FBAlertsMonitor.m | Updates monitoring to construct FBAlert from applications and use isPresent. |
| WebDriverAgentLib/Routing/FBSession.m | Routes auto-click and default alert actions through new FBAlert APIs and exception reasons. |
| WebDriverAgentLib/Routing/FBExceptions.m | Adds new alert exception name constants. |
| WebDriverAgentLib/Routing/FBExceptions.h | Declares new alert exception name constants and documents their meaning. |
| WebDriverAgentLib/Routing/FBExceptionHandler.m | Maps new alert exception types to no such alert, invalid element state, and unsupported operation. |
| WebDriverAgentLib/FBAlert.m | Implements snapshot-based detection + UID-based element resolution; converts actions to throw typed exceptions; adds iOS < 18 button-lookup fallback. |
| WebDriverAgentLib/FBAlert.h | Updates the public alert API to throwing void methods and documents behavior. |
| WebDriverAgentLib/Commands/FBAlertViewCommands.m | Simplifies alert routes by relying on exception mapping instead of manual NSError plumbing. |
| WebDriverAgentLib/Categories/XCUIApplication+FBAlert.m | Implements in-memory snapshot traversal for alert detection and adds limited-access-prompt application lookup. |
| WebDriverAgentLib/Categories/XCUIApplication+FBAlert.h | Updates category API to return alert snapshots and expose limited-access-prompt application helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @try { | ||
| [[FBAlert alertWithApplication:self.testedApplication] dismiss]; | ||
| } @catch (NSException *) { | ||
| // No alert is present, nothing to clear | ||
| } |
There was a problem hiding this comment.
Good catch, fixed in 39131af — now only ignores FBAlertNotPresentException and rethrows anything else.
| @try { | ||
| [[FBAlert alertWithApplication:XCUIApplication.fb_systemApplication] accept]; | ||
| } @catch (NSException *) { | ||
| // No alert is present on this tick - expected on most iterations of this poll loop | ||
| } |
There was a problem hiding this comment.
Good catch, fixed in 39131af — now only ignores FBAlertNotPresentException and rethrows anything else.
Some system alerts nest buttons deep enough to exceed FBConfiguration.snapshotMaxDepth from the application root, causing the in-memory snapshot walk to miss them regardless of iOS version. Try the cheap snapshot walk first, then fall back to resolving the live alert element and querying it directly, which gets a fresh depth budget. Also address Copilot review comments on PR #1193: the @catch blocks in FBIntegrationTestCase and FBPasteboard were swallowing all exceptions instead of only the expected "no alert open" case, which could mask real dismiss/accept failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI showed the "always try the snapshot walk first" change regressed iphone/ipad Min_Xcode (iOS 17.5): FBAlertTests.testNotificationAlert started failing deterministically (4/4 retries) on both device types, while the prior commit with @available(iOS 18.0, *) gating passed cleanly. Root cause: on iOS < 18, sibling buttons in a system alert can sit at different snapshot depths, so a depth-truncated snapshot walk doesn't reliably come back empty - it can return a partial, wrong button set instead. That silently picks the wrong button to tap, which is unsafe. Restore the iOS 18.0 gate so iOS < 18 always resolves the live element and queries buttons from it (the previously verified-safe path), while keeping the "fall back to a live query if the snapshot walk finds nothing" safety net for iOS 18+, where it's known safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
I’ll run some tests tomorrow, don’t have access to my devices today. |
Summary
Reworks
FBAlert's alert detection/interaction pipeline to minimize accessibility round trips and to use typed exceptions instead ofBOOL+NSError**, consistent with how the rest of the routing layer already reports errors.Started from a narrow bug —
detectionSnapshotre-scanning the whole app even when an alert was already constructed from a known parent element — and widened after finding the constructor causing that bug (alertWithElement:) had no real reason to exist, plus several other correctness/performance issues in the surrounding code.Changes
Detection: one snapshot, walked in-memory
XCUIApplication+FBAlert: dropped theXCUIElementQuery-basedfb_alertElement/fb_alertElementFromSafariWithScrollView:(each round trip could cost seconds while the target app's main thread is busy, e.g. a blocked JS alert in Safari).fb_alertSnapshotnow takes one application snapshot and walks it entirely viaenumerateDescendantsUsingBlock:— no further AX round trips.fb_findAlertSnapshotInApplicationSnapshot:was picking whichever of{Alert, Sheet, ScrollView}it hit first in tree order, with no type priority — a Sheet or ScrollView elsewhere in the tree (e.g. springboard's own UI) could permanently shadow the real Alert, since only one candidate was ever kept. Now it collects candidates by type in a single pass and applies real priority: an Alert wins outright; otherwise the first Sheet that passes the iPad popover-containment check; otherwise the first ScrollView that resolves to a genuine Safari web alert.Interaction: one detection snapshot + at most one targeted element resolution
alertSnapshotFromApplication:is the sole detection path (triessystemApp, thenself.applicationif different, then the iOS 18+ limited-access-prompt app).elementForSnapshot:inApplication:turns an already-found snapshot into a live, tappable element via a single targetedfb_uid-predicate query instead of re-walking the tree with an attribute/type query. (fb_stableInstanceWithUid:couldn't be reused for this — it short-circuits and returnsselfwhenever called on anXCUIApplicationinstance, so the underlying uid-predicate query is replicated directly.)isPresent/text/buttonLabelsneed no element resolution at all now.accept/dismiss/clickAlertButton:/typeText:resolve only the specific button/field they're about to tap, not the whole alert container — except when a custom class-chain selector is configured, which still needs a live container to query against.FBStaleElementException(the UI can change mid-resolution), folding it into a plain "not present" outcome instead of letting it propagate uncontrolled.iOS < 18 compatibility fix (found via CI)
CI failed only on the Min-Xcode jobs (iOS 17.5), consistently and reproducibly, with a large cascade of unrelated test failures following
FBAlertTests. Root cause: the snapshot-based button lookup (buttonSnapshotsInAlertSnapshot:) is bounded byFBConfiguration.snapshotMaxDepth, measured from the application root. The previousXCUIElementQuery-based code resolved the alert element live first and queried buttons from that element, which gets its own fresh depth budget. On iOS 17.5, some system alert button hierarchies apparently sit deep enough from the app root to exceed that budget (while the alert's own type/text near the top of its subtree is still reachable), soaccept/dismisssilently found no button, left the alert on screen, and broke every subsequent test in the run.Fix:
accept/dismiss/clickAlertButton:'s default (non-custom-selector) button lookup now branches on@available(iOS 18.0, *)— the snapshot-only fast path on 18+, and on <18 it resolves the live alert element first and queries buttons from it (restoring the old depth-budget-reset behavior) via a newfb_buttonInAlertSnapshot:inApplication:preferLast:/matchingLabel:helper.API: exceptions instead of BOOL + NSError**
accept,dismiss,clickAlertButton:,clickElementMatchingClassChain:, andtypeText:are nowvoidand@throwone of three new typed exceptions (FBAlertNotPresentException,FBAlertActionFailedException,FBAlertSetTextFailedException), wired intoFBExceptionHandleralongside the existing exception types (FBStaleElementException,FBTimeoutException, etc.) so they map to the right HTTP status automatically.FBAlertViewCommandsshrank accordingly — no more manualNSErrorplumbing or redundantisPresentpre-checks duplicating the presence check the action methods already do internally.alertWithElement:— its only two callers (FBAlertsMonitor,FBPasteboard) were pre-resolving the interactive element themselves before wrapping it, defeating the point of the fast detection path. Both now usealertWithApplication:+isPresent/accept, resolving the interactive element lazily only if something actually needs tapping.alertElementis now fully private (lives only inFBAlert's own class extension) since nothing outsideFBAlert.mneeds it anymore.clickElementMatchingClassChain:(public) to replaceFBSession's direct use ofalert.alertElementfor theautoClickAlertSelectorfeature.XCUIApplication+FBAlertgainedfb_limitedAccessPromptApplication(cheap.state-gated app lookup) so callers can avoid alert resolution entirely when that prompt process isn't foregrounded.Tests
FBAlertTests,FBSafariAlertTests, andFBIntegrationTestCase.clearAlertfor the newvoid-throwing signatures (XCTAssertNoThrow/XCTAssertThrowsinstead of checking a returnedBOOL/NSError).testAlertElement, which exercised the now-privatealertElementaccessor directly with no public equivalent; other tests already cover the underlying resolution viaaccept/dismiss/text/buttonLabels.Test plan
WebDriverAgentLibandIntegrationTests_1build cleanly.iphone_int_test_1_Min_Xcode,ipad_int_test_1_Min_Xcode) against a clean master baseline run, root-caused it to the snapshot-depth issue above, and fixed it with a version-gated fallback.