Model a scroll container in the drag-and-drop spec fixtures - #24622
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. |
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. |
9d6b6e8 to
2cab1f4
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. |
8b24961 to
177a1bf
Compare
177a1bf to
16506df
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. |
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.
2064209 to
75eb667
Compare
Ticket
No work package — frontend test-output hygiene, noticed while working on #AGILE-361.
Stacked on #24621, which is itself stacked on #24525.
What are you trying to accomplish?
Removes the ~43
Auto scrolling has been attached to an element that appears not to be scrollablelines from the frontend Vitest output, each one dragging a full DOM dump behind it.Unlike the hoisting warnings in the PR below this one, these are accurate. Pragmatic's warning is dev-only, fires at
autoScrollForElements()registration, and tests computedoverflow-x/overflow-yforauto/scroll— it never measures real scrollability, and there is no opt-out flag.In production,
sortable-lists.directive.ts:232resolvesscrollContainer() ?? closestScrollableAncestorOf(host) ?? undefined, so a real scroll container is always found. In the specs the fixtures sat in a plain container with no scrollable ancestor, so the engine fell back toscrollContainer ?? element(sortable-lists-engine.ts:459) and attached to the list element itself. Pragmatic was correctly reporting that the fixture did not model a scroll container.So this is not warning-suppression: the fixtures were wrong, and they are now shaped like production.
What approach did you choose and why?
Give the element auto-scroll actually attaches to a computed
overflow-y: auto, with no height.Because Pragmatic checks computed overflow and nothing else, that is sufficient. It also leaves layout alone apart from establishing a block formatting context — which matters here, because these specs assert drop edges and pointer offsets from element rects. A height cap, or
overflow-y: scroll's reserved scrollbar gutter, would have moved the numbers those assertions depend on.The attachment point differs per spec, so it was resolved file by file rather than blanket-applied:
sortable-lists.directive.behavior.spec.ts— the directive walks up to the closest scrollable ancestor, so nine host templates gained a.scroll-hostwrapper.drag-and-drop.service.spec.ts— this service registers on the container it is handed, with no ancestor walk, so the container itself takes the overflow.wp-card-drag-and-drop.service.spec.ts— one wrapper covers the root and both card lists; each one's ancestor walk climbs to it.sortable-lists-engine.preview.spec.ts— callscreateSortableRootdirectly, so the root itself takes the overflow.What the exempt fixtures are, and why they still warn nothing
sortable-lists.directive.behavior.spec.tscontainsit('warns when the collapsed root has neither an input nor a scrollable ancestor'), which spies onconsole.warnfor the literal'not to be scrollable'. Its three fixtures deliberately have no scrollable ancestor, and they are untouched here — giving them one would have left that test passing while proving nothing. It is silent in the run output because the spy swallows the warning, not because it stopped happening.The
explicit-list scroll-container fallbackdescribe is likewise untouched: its.ancestorand.scrollable-listelements are the subject of its owndata-auto-scrollableassertions.A constraint for DREAM-786's deferred scroll-container decision
The obvious version of this change — one shared wrapper per fixture — collided with a real engine limit, worth recording even though nothing hits it today.
In the two nested-root fixtures a single wrapper made the outer and inner
opSortableListsroots resolve the same element.scrollRegistrationsis scoped per root (sortable-lists-engine.ts:150) and nothing dedupes across roots, so Pragmatic's element-keyed registry took two registrations on one node — its "You have already registered autoScrolling on the same element" warning, whose second-order effect the engine's own comment at:142-149spells out: the survivor silently loses autoscroll once either side cleans up. The fixtures resolve it with a nested.inner-scroll-hostso each root gets a distinct ancestor.To be clear about the blast radius: no consumer is exposed. Every surface has exactly one root per page — Backlogs (one root, two distinct
sortable-lists--scrollablecolumns), Boards (addScrollContainerpointing at the root's own element, which is the case the engine comment was written for), anddraggable-autocompleter(single instance in all seven call sites). Within one root the dedupe works. So this is not a latent production bug and doesn't warrant a ticket of its own.It is, however, a constraint on the decision #24504 defers — "wiring
sortable-lists--scrollableneeds a scroll-container decision". That page is single-root by design (DREAM-786's first acceptance criterion is one root hosting both item types), so it is safe as drawn. Two things to keep in view when the decision is made: it stops being safe if a second root ever lands inside the same scrollable ancestor, and Backlogs' outlet selector — its only precedent — is page-scoped (#backlogs_container [data-controller~='sortable-lists--scrollable']), so a second root anywhere under that container would match both scrollable elements and collide without any nesting involved. Scoping the selector to the root's own subtree costs nothing now.Merge checklist
Verification
Full suite run three times on chromium.
Auto scrolling has been attached0 / 0 / 0already registered autoScrolling0 / 0 / 0Warning: A vi.0 / 0 / 0 (from the PR below)The geometry-sensitive assertions are the canary for this change and were left untouched: the preview spec's exact pointer offsets (
{ x: 12, y: 18 }and the clamped{ x: 100, y: 20 }), and the container-append test's drop-edge maths."Warning-free" here means the two targeted families. Still present, unchanged from baseline and out of scope: the
stimulus-useuseDispatchdeprecation (10),NG0912(1), the Lit dev-mode banner (2), andError updating activities list(2).