feat: adding webview support #168
Open
gmegidish wants to merge 22 commits into
Open
Conversation
Defines the protocol-layer interfaces for webview support. MobilewrightDriver gains an optional webViewBridge property so existing drivers compile unchanged.
Adds { kind: 'webview' } to LocatorStrategy with exact type matching for
WKWebView, XCUIElementTypeWebView, android.webkit.WebView, RCTWebView, and
RNCWebView. Includes unit tests for all five types, chaining, and empty results.
WebViewLocator extends Locator and adds page() which attaches to the bridge by matching the native webview index to bridge.listWebViews(). first/last/nth are overridden to preserve WebViewLocator type; other chaining methods return plain Locator. Page is a minimal stub to be filled in step 4.
Page exposes all v1 locator factories (getByRole, getByText, getByLabel, getByPlaceholder, getByTestId, getByAltText, getByTitle, locator) and page-level methods (url, title, goto, reload, evaluate, waitForURL, waitForLoadState, content, close). WebLocator defines WebLocatorStrategy and factory/collection methods; actions and queries come in step 5.
…implementation Introduces dom-selector-engine.ts — a browser-injectable JS string defining window.__mw with W3C-compliant ARIA role computation, accessible name algorithm, text matching (innermost-match semantics), and label detection. buildFindAll() now delegates to window.__mw.* instead of naive inline CSS/JS guesses. Page gains a static attach() factory that injects the engine once per session. WebLocator gains full query methods (isVisible, textContent, inputValue, getAttribute, count, waitFor, boundingBox) and action methods (click, fill, type, press, focus, hover, scrollIntoViewIfNeeded).
expect(page).toHaveURL/toHaveTitle — polls page.url()/title() with auto-retry. expect(webLocator).toBeVisible/Hidden/Enabled/Disabled/Checked, toHaveText, toContainText, toHaveValue, toHaveAttribute, toHaveCount — all with .not and configurable timeout. Exports Page, WebLocator, WebViewLocator, WebLocatorStrategy from the package index.
WebLocator gains getText()/getValue() aliases so LocatorAssertions can drive it. LocatorAssertions fields/helpers widened to LocatorLike interface and made protected. toHaveCount moved to LocatorAssertions (works for both locator types). WebLocatorAssertions now extends LocatorAssertions and only adds toHaveAttribute.
75 tests across page.test.ts and web-locator.test.ts covering Page.attach(), all page-level methods, locator factory JS generation, state queries, value queries, actions, waitFor, expect(page) and expect(webLocator) matchers with .not negation. Also fixes two bugs found during testing: - LocatorAssertions.not now uses this.constructor so subclasses (WebLocatorAssertions) get the correct type back without needing to override not - toHaveCount was missing negation in its predicate, causing not.toHaveCount to time out instead of resolving immediately
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…idy selector engine
| // Wrap a statement list in an IIFE with `el` bound to the first match. | ||
| // Use a `return` inside `body` to produce a value. | ||
| private firstElExpr(body: string): string { | ||
| return `(() => { const el = ${this.firstEl()}; ${body} })()`; |
Member
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements end-to-end WebView support: adds protocol WebView types, a mobilecli driver bridge to device.webview.* RPCs, a WebViewSession-backed Page abstraction, WebLocator and WebViewLocator implementations, a browser-injectable DOM selector engine with ARIA/name helpers, expect extensions for Page/WebLocator, runStep instrumentation, fake WebViewSession test doubles, comprehensive tests, and small packaging/script updates. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/mobilewright-core/src/web-locator.test.ts (1)
474-484: ⚡ Quick winAdd a chaining-scope assertion (not only step propagation).
Current chaining coverage checks
_stepFnpropagation but not that chained locators scope to the parent match. Adding one behavior assertion here would have caught the current chaining bug inWebLocator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/web-locator.test.ts` around lines 474 - 484, Add a test assertion that verifies chaining scopes queries to the parent locator (not just that _stepFn propagates). In the existing test that creates loc = new WebLocator(session, { kind: 'css', selector: '.form' }) (using sessionAlwaysReturning and recordingStepFn), add one check that loc.locator('.field') actually targets a descendant of '.form' (for example by arranging the test session to return nested/matching handles or by resolving the chained locator and asserting its matched element is contained within the parent match). Ensure the assertion exercises WebLocator.locator (and/or getByRole/nth/first) so the chaining-scope behavior is validated along with _stepFn propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mobilewright-core/src/dom-selector-engine.ts`:
- Around line 79-82: The RegExp passed into buildTextMatcher (and elsewhere in
findByAttr/findByRole) can be stateful (has /g or /y) so repeated .test() calls
advance lastIndex and cause incorrect matches; fix by resetting the regex state
before each test: in buildTextMatcher when textOrRegex is a RegExp, return a
matcher that sets textOrRegex.lastIndex = 0 before calling
textOrRegex.test(text), and make the same change in any direct uses in
findByAttr and findByRole where a RegExp's .test() is invoked (ensure lastIndex
is reset to 0 immediately before each .test() call).
In `@packages/mobilewright-core/src/expect.ts`:
- Around line 508-523: The predicate passed to retryAssertion inside
toHaveAttribute currently always checks for the positive match and ignores
this.negated; update that predicate (the second argument to retryAssertion in
toHaveAttribute) to return the match result XOR this.negated (i.e., if
this.negated is true the predicate should succeed when the positive match fails,
and vice versa). Ensure you still treat null as non-match when negated (null
should make positive match false, so negated should allow success only if
element exists and does not match), and keep the failure message construction in
the provided arrow function unchanged except it may use this.negated as already
done.
In `@packages/mobilewright-core/src/page.ts`:
- Around line 108-113: The retry predicate in Page.waitForURL currently calls
url.test(current) on the original RegExp instance which can be mutated by
global/sticky flags; change the predicate used in retryUntil to avoid
RegExp.lastIndex flakiness by creating a fresh, non-global RegExp for each test
(e.g., construct new RegExp(url.source, url.flags with 'g' and 'y' removed) and
call .test on that) or at minimum reset url.lastIndex = 0 before calling
url.test; update the predicate inside the waitForURL implementation (the
retryUntil lambda) so it uses the safe regex copy/reset instead of calling
url.test(current) directly.
In `@packages/mobilewright-core/src/web-locator.ts`:
- Around line 289-299: The click/fill (and similar) actions silently no-op when
the element disappears between visibility polling and the DOM operation; update
methods like click, fill, focus, scrollIntoViewIfNeeded, press, hover (and any
call sites using evalOnFirst/firstEl) to assert the element exists immediately
before performing the action and throw a clear error if it does not;
specifically, after pollUntilVisible(...) and inside the _step callback replace
one-line optional chaining evaluates (e.g.
session.evaluate(`${this.firstEl()}?.click()`) and fill's `if (el) { ... }`
eval) with an evaluation that checks for the element and throws (or returns an
error) when missing so the method rejects instead of silently succeeding —
reference functions: click, fill, pollUntilVisible, evalOnFirst, firstEl, and
_step to locate and update all similar patterns (also apply same change to lines
310-331).
- Around line 76-80: child() currently discards the existing locator strategy
and creates a fresh WebLocator with only the new strategy; change it to compose
the current locator's strategy (this._strategy) with the incoming strategy into
a single combined strategy (e.g., chain/compose function) and pass that composed
strategy to the WebLocator constructor so queries are scoped correctly,
preserving loc._stepFn; apply the same composition fix to other places that
construct new WebLocator instances in this file (the blocks that currently call
new WebLocator(this.session, ...) around the getBy*/filter methods) so all
chained locator creations maintain and combine prior strategies rather than
replacing them.
In `@packages/mobilewright-core/src/webview-locator.ts`:
- Around line 68-76: The current mapping logic in webview-locator.ts (variables
allNativeWebviews, selected from queryAll(roots, this.strategy), matched, index
and bridgeWebviews) can silently pick the wrong bridge webview; change it to
fail fast: after computing selected = queryAll(roots, this.strategy) require
exactly one selected match and find its index in allNativeWebviews (matched); if
selected.length !== 1 or matched === -1 then throw a descriptive Error
indicating the locator did not resolve uniquely (include this.strategy or
locator context); also when computing the bridge target, do not clamp the
index—if index is out of bounds of bridgeWebviews (index < 0 or index >=
bridgeWebviews.length) throw an Error instead of defaulting to first/last;
update any callers/uses such as nth() and getByWebView() to expect these throws.
---
Nitpick comments:
In `@packages/mobilewright-core/src/web-locator.test.ts`:
- Around line 474-484: Add a test assertion that verifies chaining scopes
queries to the parent locator (not just that _stepFn propagates). In the
existing test that creates loc = new WebLocator(session, { kind: 'css',
selector: '.form' }) (using sessionAlwaysReturning and recordingStepFn), add one
check that loc.locator('.field') actually targets a descendant of '.form' (for
example by arranging the test session to return nested/matching handles or by
resolving the chained locator and asserting its matched element is contained
within the parent match). Ensure the assertion exercises WebLocator.locator
(and/or getByRole/nth/first) so the chaining-scope behavior is validated along
with _stepFn propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13e8fab1-6d49-4fbd-a356-85f6bab4834d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
e2e/package.jsonpackages/driver-mobilecli/package.jsonpackages/driver-mobilecli/src/driver.test.tspackages/driver-mobilecli/src/driver.tspackages/mobilewright-core/src/dom-selector-engine.tspackages/mobilewright-core/src/expect.tspackages/mobilewright-core/src/fake-webview-session.tspackages/mobilewright-core/src/index.tspackages/mobilewright-core/src/locator.tspackages/mobilewright-core/src/page.test.tspackages/mobilewright-core/src/page.tspackages/mobilewright-core/src/query-engine.test.tspackages/mobilewright-core/src/query-engine.tspackages/mobilewright-core/src/screen.tspackages/mobilewright-core/src/stackTrace.tspackages/mobilewright-core/src/web-locator.test.tspackages/mobilewright-core/src/web-locator.tspackages/mobilewright-core/src/webview-locator.tspackages/protocol/src/driver.tspackages/protocol/src/types.ts
…r negation in toHaveAttribute
… and attribute negation
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/mobilewright-core/src/web-locator.ts (1)
338-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd pre-action visibility waiting for the remaining action methods.
Line 340, Line 346, Line 352, and Line 358 execute immediately against the first match, while
click/fill/typefirst wait for visibility. This inconsistency can cause avoidable flakiness when elements appear shortly after the call.Suggested patch
async press(key: string): Promise<void> { return this._step(`locator.press(${JSON.stringify(key)})`, async () => { + await this.pollUntilVisible(DEFAULT_TIMEOUT); await this.actOnFirst(`['keydown','keypress','keyup'].forEach(t => el.dispatchEvent(new KeyboardEvent(t, { key: ${JSON.stringify(key)}, bubbles: true })));`, 'locator.press()'); }); } async focus(): Promise<void> { return this._step('locator.focus()', async () => { + await this.pollUntilVisible(DEFAULT_TIMEOUT); await this.actOnFirst('el.focus();', 'locator.focus()'); }); } async hover(): Promise<void> { return this._step('locator.hover()', async () => { + await this.pollUntilVisible(DEFAULT_TIMEOUT); await this.actOnFirst('el.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); el.dispatchEvent(new MouseEvent("mouseenter", { bubbles: true }));', 'locator.hover()'); }); } async scrollIntoViewIfNeeded(): Promise<void> { return this._step('locator.scrollIntoViewIfNeeded()', async () => { + await this.pollUntilVisible(DEFAULT_TIMEOUT); await this.actOnFirst('el.scrollIntoView({ block: "nearest" });', 'locator.scrollIntoViewIfNeeded()'); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/web-locator.ts` around lines 338 - 359, The methods press, focus, hover, and scrollIntoViewIfNeeded run actOnFirst immediately and must mirror click/fill/type by waiting for the target to be visible first; update locator.press, locator.focus, locator.hover, and locator.scrollIntoViewIfNeeded to call the same pre-action visibility wait used by click/fill/type (the helper invoked before actOnFirst) inside their _step wrapper, then call actOnFirst as before (keep the existing descriptions passed to _step/actOnFirst). Reference the existing functions: press, focus, hover, scrollIntoViewIfNeeded, _step, and actOnFirst when making this change.
🧹 Nitpick comments (1)
packages/mobilewright-core/src/web-locator.test.ts (1)
291-298: ⚡ Quick winAssert runtime failure, not just generated source text.
This test verifies the injected string contains
throw, but it doesn’t proveloc.click()actually rejects when the element is missing. Add a rejection-path test to lock behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/web-locator.test.ts` around lines 291 - 298, Add a rejection-path test that ensures WebLocator.click actually rejects when the element is missing: create a session that simulates "no element" (e.g., use sessionAlwaysReturning(false) or equivalent), construct the same WebLocator (new WebLocator(session, { kind: 'css', selector: '.btn' })), call loc.click() and assert the Promise rejects (e.g., expect(loc.click()).rejects.toThrow or playwrightExpect(...).toReject) and that the error message contains "element not found"; also keep the existing check on evaluateCalls (find c => c.includes('.click()')) to verify the injected throw is present. Ensure you reference WebLocator, sessionAlwaysReturning, loc.click, and evaluateCalls in the test so behavior (not just generated source) is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/mobilewright-core/src/web-locator.ts`:
- Around line 338-359: The methods press, focus, hover, and
scrollIntoViewIfNeeded run actOnFirst immediately and must mirror
click/fill/type by waiting for the target to be visible first; update
locator.press, locator.focus, locator.hover, and locator.scrollIntoViewIfNeeded
to call the same pre-action visibility wait used by click/fill/type (the helper
invoked before actOnFirst) inside their _step wrapper, then call actOnFirst as
before (keep the existing descriptions passed to _step/actOnFirst). Reference
the existing functions: press, focus, hover, scrollIntoViewIfNeeded, _step, and
actOnFirst when making this change.
---
Nitpick comments:
In `@packages/mobilewright-core/src/web-locator.test.ts`:
- Around line 291-298: Add a rejection-path test that ensures WebLocator.click
actually rejects when the element is missing: create a session that simulates
"no element" (e.g., use sessionAlwaysReturning(false) or equivalent), construct
the same WebLocator (new WebLocator(session, { kind: 'css', selector: '.btn'
})), call loc.click() and assert the Promise rejects (e.g.,
expect(loc.click()).rejects.toThrow or playwrightExpect(...).toReject) and that
the error message contains "element not found"; also keep the existing check on
evaluateCalls (find c => c.includes('.click()')) to verify the injected throw is
present. Ensure you reference WebLocator, sessionAlwaysReturning, loc.click, and
evaluateCalls in the test so behavior (not just generated source) is asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b958457a-5627-421f-9c2c-d9635ebc849d
📒 Files selected for processing (5)
packages/mobilewright-core/src/dom-selector-engine.test.tspackages/mobilewright-core/src/page.test.tspackages/mobilewright-core/src/web-locator.test.tspackages/mobilewright-core/src/web-locator.tspackages/mobilewright-core/src/webview-locator.test.ts
Member
Author
|
@CodeRabbit another review please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/mobilewright-core/src/web-locator.ts (1)
338-342:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
press()should wait for the element to be visible before dispatching keyboard events.Unlike
click(),fill(), andtype()which callpollUntilVisible()before acting,press()immediately callsactOnFirst()without waiting. This inconsistency could cause flaky tests when pressing keys on elements that are still loading.🐛 Proposed fix
async press(key: string): Promise<void> { return this._step(`locator.press(${JSON.stringify(key)})`, async () => { + await this.pollUntilVisible(DEFAULT_TIMEOUT); await this.actOnFirst(`['keydown','keypress','keyup'].forEach(t => el.dispatchEvent(new KeyboardEvent(t, { key: ${JSON.stringify(key)}, bubbles: true })));`, 'locator.press()'); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/web-locator.ts` around lines 338 - 342, The press() method currently dispatches keyboard events immediately; update it to mirror click()/fill()/type() by waiting for visibility first. Inside press() (and its _step callback) call pollUntilVisible(...) for the same locator/context before calling actOnFirst(...), ensuring you use the same visibility timeout/arguments pattern as click()/fill()/type() so press() only dispatches events after the element is visible.packages/mobilewright-core/src/expect.ts (1)
516-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
not.toHaveAttribute()fails incorrectly when the attribute is missing.The predicate returns
falsewhenvalue === null, bypassing the negation logic. Fornot.toHaveAttribute('href', 'foo'), a missing attribute should satisfy the assertion (the element does NOT have that attribute with the expected value), but currently it causes the assertion to keep polling until timeout.🐛 Proposed fix
(value) => { - if (value === null) { return false; } + if (value === null) { return this.negated; } const matches = expected instanceof RegExp ? expected.test(value) : value === expected; return this.negated ? !matches : matches; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/expect.ts` around lines 516 - 519, The predicate inside the toHaveAttribute assertion wrongly returns false unconditionally when value === null, which prevents negated assertions from passing; update the predicate (the arrow function in packages/mobilewright-core/src/expect.ts used by toHaveAttribute) so that when value === null it returns this.negated (i.e., true for negated assertions and false for positive ones) and otherwise performs the existing RegExp/value comparison and applies this.negated to the match result.
🧹 Nitpick comments (1)
packages/mobilewright-core/src/web-locator.ts (1)
38-39: 💤 Low valueConsider using
CSS.escape()for the testId selector.The testId is embedded in a CSS attribute selector using
JSON.stringify. While JSON escaping handles most cases, CSS attribute selectors have different escaping semantics. If a testId ever contains characters like]or", the behavior depends on CSS parsing rules which differ from JSON.Since testIds are typically developer-controlled alphanumeric identifiers, this is low-risk, but using
CSS.escape()would be more robust:🔧 Suggested improvement
case 'testId': - return `Array.from(${root}.querySelectorAll('[data-testid=${JSON.stringify(strategy.testId)}]'))`; + return `Array.from(${root}.querySelectorAll('[data-testid="' + CSS.escape(${JSON.stringify(strategy.testId)}) + '"]'))`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/web-locator.ts` around lines 38 - 39, The attribute selector for the 'testId' case in web-locator.ts currently injects JSON.stringify(strategy.testId) into querySelectorAll; replace that with a CSS-escaped value so special CSS characters are handled correctly (e.g. use the attribute selector with a quoted, escaped value like data-testid=""+CSS.escape(strategy.testId)+""). Update the case 'testId' return expression to use CSS.escape(strategy.testId) (ensuring the escaped value is wrapped in quotes in the selector), and add a fallback/polyfill for environments that lack global CSS.escape or a small helper escape function so tests remain cross-platform; target the 'testId' branch and the use of strategy.testId and root to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mobilewright-core/src/web-locator.ts`:
- Around line 344-360: The focus, hover, and scrollIntoViewIfNeeded methods call
actOnFirst() immediately and should first wait for the element to be visible
like click()/fill()/type() do; update async focus(), hover(), and
scrollIntoViewIfNeeded() to await the same visibility-wait helper used by
click/fill/type (e.g., await this.waitForVisible('locator.focus()') or the
equivalent internal method used in those actions) before calling actOnFirst(),
keeping the existing _step and actOnFirst calls intact.
---
Duplicate comments:
In `@packages/mobilewright-core/src/expect.ts`:
- Around line 516-519: The predicate inside the toHaveAttribute assertion
wrongly returns false unconditionally when value === null, which prevents
negated assertions from passing; update the predicate (the arrow function in
packages/mobilewright-core/src/expect.ts used by toHaveAttribute) so that when
value === null it returns this.negated (i.e., true for negated assertions and
false for positive ones) and otherwise performs the existing RegExp/value
comparison and applies this.negated to the match result.
In `@packages/mobilewright-core/src/web-locator.ts`:
- Around line 338-342: The press() method currently dispatches keyboard events
immediately; update it to mirror click()/fill()/type() by waiting for visibility
first. Inside press() (and its _step callback) call pollUntilVisible(...) for
the same locator/context before calling actOnFirst(...), ensuring you use the
same visibility timeout/arguments pattern as click()/fill()/type() so press()
only dispatches events after the element is visible.
---
Nitpick comments:
In `@packages/mobilewright-core/src/web-locator.ts`:
- Around line 38-39: The attribute selector for the 'testId' case in
web-locator.ts currently injects JSON.stringify(strategy.testId) into
querySelectorAll; replace that with a CSS-escaped value so special CSS
characters are handled correctly (e.g. use the attribute selector with a quoted,
escaped value like data-testid=""+CSS.escape(strategy.testId)+""). Update the
case 'testId' return expression to use CSS.escape(strategy.testId) (ensuring the
escaped value is wrapped in quotes in the selector), and add a fallback/polyfill
for environments that lack global CSS.escape or a small helper escape function
so tests remain cross-platform; target the 'testId' branch and the use of
strategy.testId and root to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3882b12-47af-43f0-8770-312123331aa3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
e2e/package.jsonpackages/driver-mobilecli/package.jsonpackages/driver-mobilecli/src/driver.test.tspackages/driver-mobilecli/src/driver.tspackages/mobilewright-core/src/dom-selector-engine.test.tspackages/mobilewright-core/src/dom-selector-engine.tspackages/mobilewright-core/src/expect.tspackages/mobilewright-core/src/fake-webview-session.tspackages/mobilewright-core/src/index.tspackages/mobilewright-core/src/locator.tspackages/mobilewright-core/src/page.test.tspackages/mobilewright-core/src/page.tspackages/mobilewright-core/src/query-engine.test.tspackages/mobilewright-core/src/query-engine.tspackages/mobilewright-core/src/screen.tspackages/mobilewright-core/src/stackTrace.tspackages/mobilewright-core/src/web-locator.test.tspackages/mobilewright-core/src/web-locator.tspackages/mobilewright-core/src/webview-locator.test.tspackages/mobilewright-core/src/webview-locator.tspackages/protocol/src/driver.tspackages/protocol/src/types.ts
Comment on lines
+344
to
+360
| async focus(): Promise<void> { | ||
| return this._step('locator.focus()', async () => { | ||
| await this.actOnFirst('el.focus();', 'locator.focus()'); | ||
| }); | ||
| } | ||
|
|
||
| async hover(): Promise<void> { | ||
| return this._step('locator.hover()', async () => { | ||
| await this.actOnFirst('el.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); el.dispatchEvent(new MouseEvent("mouseenter", { bubbles: true }));', 'locator.hover()'); | ||
| }); | ||
| } | ||
|
|
||
| async scrollIntoViewIfNeeded(): Promise<void> { | ||
| return this._step('locator.scrollIntoViewIfNeeded()', async () => { | ||
| await this.actOnFirst('el.scrollIntoView({ block: "nearest" });', 'locator.scrollIntoViewIfNeeded()'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
focus(), hover(), and scrollIntoViewIfNeeded() should wait for visibility for consistency.
These actions immediately call actOnFirst() without waiting for the element to be visible, unlike click()/fill()/type(). This inconsistency could cause confusion and flaky tests.
🐛 Proposed fix
async focus(): Promise<void> {
return this._step('locator.focus()', async () => {
+ await this.pollUntilVisible(DEFAULT_TIMEOUT);
await this.actOnFirst('el.focus();', 'locator.focus()');
});
}
async hover(): Promise<void> {
return this._step('locator.hover()', async () => {
+ await this.pollUntilVisible(DEFAULT_TIMEOUT);
await this.actOnFirst('el.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); el.dispatchEvent(new MouseEvent("mouseenter", { bubbles: true }));', 'locator.hover()');
});
}
async scrollIntoViewIfNeeded(): Promise<void> {
return this._step('locator.scrollIntoViewIfNeeded()', async () => {
+ await this.pollUntilVisible(DEFAULT_TIMEOUT);
await this.actOnFirst('el.scrollIntoView({ block: "nearest" });', 'locator.scrollIntoViewIfNeeded()');
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mobilewright-core/src/web-locator.ts` around lines 344 - 360, The
focus, hover, and scrollIntoViewIfNeeded methods call actOnFirst() immediately
and should first wait for the element to be visible like click()/fill()/type()
do; update async focus(), hover(), and scrollIntoViewIfNeeded() to await the
same visibility-wait helper used by click/fill/type (e.g., await
this.waitForVisible('locator.focus()') or the equivalent internal method used in
those actions) before calling actOnFirst(), keeping the existing _step and
actOnFirst calls intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.