Model a scroll container in the drag-and-drop spec fixtures - #24622
Model a scroll container in the drag-and-drop spec fixtures#24622myabc wants to merge 3 commits into
Conversation
|
Warning This pull request does not link an OpenProject work package. Please add a link to the work package in the description, or reference it in the |
There was a problem hiding this comment.
Pull request overview
This PR updates frontend drag-and-drop spec fixtures to better model production by ensuring Pragmatic’s auto-scroll attaches to an element that appears scrollable (via computed overflow), eliminating noisy Vitest warnings and large DOM dumps in test output.
Changes:
- Adds scroll-container semantics to multiple spec fixtures via
overflow-y:auto/overflow: autowrappers. - Adjusts several Angular test templates to include a
.scroll-host(and in nested-root cases an additional.inner-scroll-host) so ancestor-walk logic resolves a scrollable element. - Updates spec comments around why Pragmatic is not mocked and why these fixtures must model scrollability.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/common/drag-and-drop/sortable-lists-engine.preview.spec.ts | Makes the fixture root look scrollable so engine-level auto-scroll registration doesn’t warn. |
| frontend/src/app/shared/helpers/drag-and-drop/drag-and-drop.service.spec.ts | Ensures the service’s provided container is treated as a scroll container by Pragmatic. |
| frontend/src/app/shared/directives/sortable-lists/sortable-lists.directive.behavior.spec.ts | Wraps multiple directive fixtures in scrollable ancestors so closest-scrollable-ancestor resolution matches production. |
| frontend/src/app/features/work-packages/components/wp-card-view/services/wp-card-drag-and-drop.service.spec.ts | Adds a scrollable wrapper for the two-list card-view directive fixture to avoid autoscroll warnings. |
c24f965 to
76bb224
Compare
529d5c3 to
9d6b6e8
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
76bb224 to
e7a6126
Compare
9d6b6e8 to
2cab1f4
Compare
e7a6126 to
f22e7c6
Compare
2cab1f4 to
8b24961
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
f22e7c6 to
5a2889c
Compare
8b24961 to
177a1bf
Compare
5a2889c to
f1f3f93
Compare
177a1bf to
16506df
Compare
f1f3f93 to
a3f233c
Compare
16506df to
2064209
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
a3f233c to
9577b7a
Compare
2064209 to
75eb667
Compare
9577b7a to
d5bd278
Compare
75eb667 to
de57346
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
d5bd278 to
9f2a00b
Compare
de57346 to
86f4119
Compare
86f4119 to
69d9bbc
Compare
9f2a00b to
c3c7239
Compare
69d9bbc to
5f7883d
Compare
c3c7239 to
c694439
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
c694439 to
d2402c9
Compare
5f7883d to
1f415fa
Compare
d2402c9 to
2eba20d
Compare
1f415fa to
626653a
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
2eba20d to
294f54a
Compare
626653a to
d7e2f31
Compare
294f54a to
2eba20d
Compare
d7e2f31 to
626653a
Compare
2eba20d to
294f54a
Compare
626653a to
d7e2f31
Compare
294f54a to
90c3b50
Compare
d7e2f31 to
bccc6d2
Compare
The Angular unit-test builder bundles each spec into a __commonJS wrapper, so Vitest's hoist validator walks that wrapper instead of our source and reports top-level vi.mock calls as nested. Only mock, unmock and hoisted are hoist-checked, so vi.doMock sidesteps it. These specs already import the mocked packages and the controller under test lazily in beforeAll, which is the ordering doMock needs. A note above each call records that, since doMock's position above the imports no longer carries the guarantee vi.mock's hoisting gave it. The root spec also claimed it read back the combine, prevent-unhandled and drag-preview options. It never did; they keep the mounted item controller's side effects inert.
vi.doMock only affects later imports, so the statically imported createSortableRoot would have pulled in the real auto-scroll module first and the mock would have stopped applying silently. A type import plus a lazy binding restores the ordering in both engine specs, and drops vi.hoisted with it. The preview spec also loses the file-split rationale that cited a shared module registry, which isolate:true removed. The split itself stands: the sibling renders previews for real.
Pragmatic warns when auto-scroll attaches to an element whose computed overflow is visible. Production always resolves a real scroll ancestor, so the warning was accurate: the fixtures had none. A wrapper carrying overflow but no height satisfies the check without shifting the geometry these specs measure drop edges from. Where the overflow belongs differs. The directive walks up to the closest scrollable ancestor, so its hosts and the card-view root get a wrapper; the drag service and the preview spec register on the element they are handed, so that element takes it directly. Nested-root fixtures need a second, inner wrapper. Two independent root engines resolving one shared host collide on Pragmatic's element-keyed registry, which dedupes per root and not across them. The opSortableListsScrollContainer suite keeps its unscrollable hosts. It asserts that warning on purpose.
bccc6d2 to
0899bb2
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Ticket
No work package — frontend test-output hygiene, noticed while working on AGILE-361.
What are you trying to accomplish?
Removes ~43
Auto scrolling has been attached to an element that appears not to be scrollablelines from the Vitest output, each dragging a full DOM dump behind it.The warnings were accurate, so this is not suppression. In production
sortable-lists.directive.ts:232always resolves a real scroll container; the fixtures had no scrollable ancestor, so the engine fell back toscrollContainer ?? element(sortable-lists-engine.ts:459) and attached to the list itself. Pragmatic was correctly reporting that the fixtures did not model a scroll container. They do now.What approach did you choose and why?
Give the element auto-scroll attaches to a computed
overflow-y: auto, with no height. Pragmatic checks computed overflow and nothing else, so that suffices, and it leaves layout alone apart from establishing a block formatting context. That matters because these specs assert drop edges and pointer offsets from element rects — a height cap, oroverflow-y: scroll's reserved scrollbar gutter, would have moved the numbers they depend on. The geometry-sensitive assertions were deliberately left untouched as the canary.The attachment point differs per spec, so it was resolved file by file: a
.scroll-hostwrapper for the nine templates insortable-lists.directive.behavior.spec.ts(the directive walks to the closest scrollable ancestor), the container itself indrag-and-drop.service.spec.ts(no ancestor walk), one shared wrapper inwp-card-drag-and-drop.service.spec.ts, and the root itself insortable-lists-engine.preview.spec.ts. Two fixture groups are exempt and still warn nothing: thewarns when the collapsed root has neither an input nor a scrollable ancestortest needs its fixtures unscrollable, and theexplicit-list scroll-container fallbackelements are the subject of their own assertions.One thing worth recording: a single shared wrapper per fixture does not work for the two nested-root cases, because both roots then resolve the same element and
scrollRegistrationsis scoped per root (sortable-lists-engine.ts:150) with no cross-root dedupe — so autoscroll is silently lost when either side cleans up, per the engine's comment at:142-149. The fixtures use a nested.inner-scroll-hostinstead. No consumer is exposed today (every surface has exactly one root per page), but it is a constraint on the scroll-container decision #24504 defers: it stops holding if a second root lands inside the same scrollable ancestor, and Backlogs' outlet selector is page-scoped, so scoping it to the root's own subtree costs nothing now.Merge checklist