Repository navigation
Conversation
The gesture host and the modules on its touch path checked nodes and events with instanceof against this window's classes. A component rendered into another window's document, such as a same-origin iframe or a Document Picture-in-Picture window, has nodes and touches from that window, so every check failed. Its pans, taps and long presses never began, its finger scrolls read as the program's, and a held layer there held no touch. The checks now read the node type (utils/nodeType.ts), or test an HTML element against its own window's class. A jsdom iframe and a real iframe in Chromium and WebKit each recognize a finger there. The WebKit proofs' scope adds utils/nodeType, which their fixtures bundle. The group-press click test in an iframe now asserts the press was claimed. It passed on main only because the iframe's press never reached the host.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
|
/approve-spec 1d797ef |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this fix. Gestures now begin for a component rendered into another window's document, and the new iframe tests fail on main and pass here. Changes are requested for three issues:
isInsideScrollViewstill checksnode instanceof Element(packages/core/src/utils/scrollView.ts:101), so in another window's document the 150 ms press delay ignores scroll containers and fixed bars, againstspec:AST-094FR11.styleOfstill checksstyle instanceof Object(packages/core/src/utils/selectionLock.ts:711), so the selection lock writes no style in another window and the iOS WebKit press lock does nothing there, againstspec:AST-094FR41.utils/nodeType.tsduplicates the node-type guards inLayer/outsidePress.tsand the own-windowisHTMLElementinhooks/useFocusTrap.ts, so make those modules import the shared guards and delete their copies.
An iframe test for each of the first two would keep them fixed.
[Automated review]
| return ( | ||
| target instanceof Element && | ||
| isInsideScrollView(target, {readsTouchAction}) | ||
| isElement(target) && isInsideScrollView(target, {readsTouchAction}) |
There was a problem hiding this comment.
isInsideScrollView still walks ancestors with node instanceof Element (utils/scrollView.ts:101). For another window's nodes this answer then ignores scroll containers and fixed bars, so the 150 ms press delay is wrong there (spec:AST-094 FR11). Use isElement(node) there and add an iframe case.
There was a problem hiding this comment.
Fixed in 28c4762. The walk now uses isElement(node), and two iframe cases cover a scroll box and a fixed bar.
| } from './flatTree'; | ||
| import {subscribeInput} from './inputStream'; | ||
| import {isIosWebKit} from './touchQueries'; | ||
| import {isElement, isNode} from './nodeType'; |
There was a problem hiding this comment.
styleOf (line 711) still returns style instanceof Object ? style : null. Another window's style object fails that check, so lockStyles writes nothing there, and the iOS WebKit lock (line 111) is empty in an iframe (spec:AST-094 FR41). Check for a style object without instanceof, and add an iframe case.
There was a problem hiding this comment.
Fixed in 28c4762: styleOf now checks for an object. jsdom makes every window's style in this realm. So the new case uses a style object without a prototype, which fails instanceof Object the same way.
| export function isNode(value: unknown): value is Node { | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| typeof (value as {nodeType?: unknown}).nodeType === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Layer/outsidePress.ts:121-138 already has these isNode, isElement and isShadowRoot guards, and hooks/useFocusTrap.ts:144-152 has an own-window isHTMLElement. Make those modules import from here and delete their copies, so one definition remains.
There was a problem hiding this comment.
Done in 28c4762: Layer/outsidePress.ts and hooks/useFocusTrap.ts now import these guards, and their copies are gone.
A touch in another window's document still missed two checks. The scroll-view walk tested each ancestor with instanceof Element, so it skipped that window's scroll boxes and fixed bars, and the press delay inside a scroll view was wrong there. The selection lock tested the inline style with instanceof Object, so on iOS WebKit it wrote no lock there. Both now use checks that hold for any window. The outside-press and focus-trap modules import the shared node checks instead of keeping their own copies.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is ready for review, a maintainer adds the ci:run label so CI runs.
The three earlier findings are fixed: the scroll-view walk and the selection lock's style check now accept another window's nodes, each with an iframe test, and the outside-press and focus-trap modules use the shared guards. Gestures now begin for a component rendered into another window's document, and the new tests fail on main and pass here.
[Automated review]
|
/approve-spec 28c4762 |
Main's pan on its own scroller now takes a trackpad's scroll (meta#10212), and its lock asks whether the host styles the element for that recognizer. This change reads another window's nodes in the same check. The merge keeps both: the lock tests the view with isElement and passes the recognizer to managed.
There was a problem hiding this comment.
Changes requested. Gestures now begin on elements that another window's document creates, and the new iframe tests fail on main and pass here, but two things need fixing:
- Miss:
isHTMLElement(packages/core/src/utils/nodeType.ts:44-52) checks an element against its document's window, so an element the page creates and then moves into an iframe or a Picture-in-Picture window fails it, and in Chromium such an element loses its derivedtouch-action(pinch-zoomon main,autohere) while its switch label gains a second switch input; testnamespaceURI === 'http://www.w3.org/1999/xhtml'instead and add a moved-element case. - Miss:
isNodeTarget(packages/core/src/keyboard/keyboardHeight.ts:100) repeats the newisNode, andGestureHost.ts:1269keeps its owninstanceof view.HTMLElementcheck, so a fix to the shared guards misses them; have both useutils/nodeType.ts.
CI has not run on this head yet: the branch is in a fork, so a maintainer adds the ci:run label for CI to run.
[Automated review]
| export function isHTMLElement(value: unknown): value is HTMLElement { | ||
| if (!isElement(value)) { | ||
| return false; | ||
| } | ||
| const ownClass = | ||
| value.ownerDocument.defaultView?.HTMLElement ?? | ||
| (typeof HTMLElement === 'undefined' ? undefined : HTMLElement); | ||
| return ownClass != null && value instanceof ownClass; | ||
| } |
There was a problem hiding this comment.
In Chromium an element keeps the window that created it when it moves to another document. So for an element the page creates and then moves into an iframe or a Picture-in-Picture window, this answers false where main's instanceof HTMLElement answered true. In a real Picture-in-Picture window, such an element with a pan loses its derived touch-action (GestureHost.ts:1411: pinch-zoom on main, auto here), and such a switch label gains a second switch input (haptics.ts:116-118). Test the namespace instead, as Lab's NavigationStack.tsx:644-648 does:
return isElement(value) && value.namespaceURI === 'http://www.w3.org/1999/xhtml';Then add a moved-element case to the iframe browser spec.
| export function isNode(value: unknown): value is Node { | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| typeof (value as {nodeType?: unknown}).nodeType === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
isNodeTarget in packages/core/src/keyboard/keyboardHeight.ts:100-102 is the same typeof nodeType === 'number' test, and GestureHost.ts:1269 keeps its own instanceof view.HTMLElement. Make keyboardHeight.ts import isNode and drop its copy, and call isHTMLElement(target) at GestureHost.ts:1269, so a fix to these guards reaches every caller.
Main's finger-scroll hook now syncs in a layout effect (meta#10172). This change imports isDocument from the shared node checks there and drops the hook's own copy. The merge keeps main's import and the shared check.
An element the page makes and then moves into another window's document, as Document Picture-in-Picture does, keeps the page's classes. The HTML element check now reads the namespace, so such an element keeps its derived touch-action and its switch label. The gesture host's click retarget and blur checks, and the keyboard height reader, use the shared node checks too.
|
/approve-spec d78dd80 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request comes from a fork; a maintainer adds the ci:run label so CI runs.
Gestures now begin on elements in a same-origin iframe or a Document Picture-in-Picture window, under a mouse and a finger. The new iframe unit cases and the iframe pan browser case fail on main and pass here, and an element the page moves into that window keeps the behavior it had. No blocking findings remain.
[Automated review]
* fix(core): gestures read another window's nodes and touches Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com> * fix(core): gestures read another window's nodes and touches Takes #10211 at d78dd80, which fixes the moved-element case, merged with main. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com> * Merge branch 'main' into reland/gestures-another-window
|
Superseded by #10224, which landed this change. |
User impact
A component rendered into another window's document gets no gestures. That is a same-origin iframe or a Document Picture-in-Picture window the page's code renders or moves elements into. There its pans, taps, long presses and swipes never begin, under any pointer. Its finger scrolls read as the program's, and a held layer holds no touch. A press in a scroll box there gets the wrong delay, and on iOS a hold can start a selection. A page whose iframe loads its own Astryx is not affected.
Problem and solution fit
instanceofchecks test the page's classes, so it dropped that window's touches.Expected behavior and authority
family:overlay-dismissalFR8: trigger-anchored members render in the page, a same-origin iframe or a Document Picture-in-Picture window.spec:AST-094FR43: gestures attach for every pointer, with no limit to the page's own document.Smallest restoration
utils/nodeType.tsadds node-type guards. ItsisHTMLElementreads the namespace, so an element moved between windows' documents passes.Evidence
Scope
Testing
At this head, which merges main
62f5ebc5e4. Everything below ran on this code.pnpm test:ui: 21,569 passed in 867 files.pnpm test:node: 16,832 passed in 928 files.pnpm -F @astryxdesign/core typecheck: 0 errors.CI=true pnpm lint: 0 errors, and no warning in a changed file. Prettier andreview:pre: clean.gestures.interaction.browser.spec.tsandpresentedHitTesting.interaction.browser.spec.ts: Chromium 78 passed and 4 skipped; WebKit 70 passed and 12 skipped by design. That includes the trackpad case of fix(gestures): a pan on its own scroller takes a trackpad's scroll, as a finger's pan (spec:AST-094 FR43) #10212.isHTMLElement(touch-actionauto, notpinch-zoom). It passes 3 of 3 in both here. The two moved-element unit cases fail and pass the same way.PopoverSecondDocumentandDialogSecondDocument, in an iframe and a Picture-in-Picture window: Chromium 33 passed.