[DREAM-813] Share anchored popover caret placement - #25112
Conversation
b8e17c1 to
b4ad30f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The timeline tooltip spec introduces real-time waits (600ms) that can significantly slow and destabilize the test suite and should be made deterministic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extracts and shares anchored-popover caret placement logic across multiple tooltip/popover consumers (timeline tooltip, time-entry calendar popover, and budget chart tooltips) so the caret follows anchored-position flip/shift behavior near viewport edges, while also moving Chart.js tooltip rendering into a component-owned host and avoiding document.body rendering.
Changes:
- Introduces a shared
anchored-popoverutility layer (caretPlacement,popoverMessage,popoverCaret) and shared styling. - Updates timeline tooltip, time-entry calendar hover popover, and budget chart tooltips to use the shared caret handling and markup.
- Refactors Chart.js external tooltip rendering to use
anchored-position+ a host container, and adds@primer/behaviorsas a direct dependency with new unit tests.
File summaries
| File | Description |
|---|---|
| frontend/src/global_styles/common/openproject-common.module.sass | Wires global SASS imports for the new anchored-popover and budget-graphs styles. |
| frontend/src/app/shared/components/project-timeline-graph/project-timeline-tooltip.popover.ts | Switches timeline tooltip caret calculation to the shared popoverCaret helper. |
| frontend/src/app/shared/components/project-timeline-graph/project-timeline-tooltip.builder.ts | Reuses shared popoverMessage and shared caret placement type. |
| frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.spec.ts | Updates expectations for shared caret CSS var and adjusts tooltip opening helper logic. |
| frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.sass | Removes tooltip-specific caret/host resets now covered by shared anchored-popover styles. |
| frontend/src/app/shared/components/budget-graphs/overview/budget-by-cost-type.component.ts | Injects a tooltip host and manages renderer lifecycle for chart tooltips. |
| frontend/src/app/shared/components/budget-graphs/overview/budget-by-cost-type.component.html | Adds the tooltip host element next to the canvas. |
| frontend/src/app/shared/components/budget-graphs/overview/actual-costs.component.ts | Same tooltip host + renderer lifecycle pattern for bar chart tooltips. |
| frontend/src/app/shared/components/budget-graphs/overview/actual-costs.component.html | Adds the tooltip host element next to the canvas. |
| frontend/src/app/shared/components/budget-graphs/chart.config.ts | Replaces manual tooltip positioning with an anchored-position-based ChartTooltip class using shared caret logic. |
| frontend/src/app/shared/components/budget-graphs/chart.config.spec.ts | Adds unit coverage for the new anchored-position-based chart tooltip renderer behavior. |
| frontend/src/app/shared/components/budget-graphs/budget-graphs.sass | Adds minimal chart tooltip message styling. |
| frontend/src/app/shared/components/anchored-popover/popover-message.ts | Adds shared lit helper for .Popover-message wrapper + caret class/offset var. |
| frontend/src/app/shared/components/anchored-popover/popover-message.spec.ts | Unit tests for shared popover message rendering and caret class/offset behavior. |
| frontend/src/app/shared/components/anchored-popover/popover-caret.ts | Adds shared synchronous caret derivation using Primer’s placement algorithm. |
| frontend/src/app/shared/components/anchored-popover/popover-caret.spec.ts | Unit tests for caret derivation (flip behavior, DOMRect anchors, agreement with element placement). |
| frontend/src/app/shared/components/anchored-popover/caret-placement.ts | Extracts and generalizes caret geometry/clamping logic. |
| frontend/src/app/shared/components/anchored-popover/caret-placement.spec.ts | Updates tests to target the new shared caret placement module. |
| frontend/src/app/shared/components/anchored-popover/anchored-popover.sass | Shared host resets and caret offset CSS variable wiring for all consumers. |
| frontend/src/app/features/calendar/te-calendar/te-calendar.component.ts | Refactors calendar popover rendering to use shared caret calculation and a shared lit template helper. |
| frontend/src/app/features/calendar/te-calendar/te-calendar.component.sass | Adds calendar popover message padding/font sizing consistent with updated markup. |
| frontend/src/app/features/calendar/te-calendar/te-calendar-popover.ts | New shared template helper for the time-entry anchored-position popover using popoverMessage. |
| frontend/src/app/features/calendar/te-calendar/te-calendar-popover.spec.ts | Unit tests for the new calendar popover template helper. |
| frontend/package.json | Adds direct dependency on @primer/behaviors. |
| frontend/package-lock.json | Locks @primer/behaviors version update. |
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 23/25 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dac3b91 to
0d36726
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Chart tooltip caret/placement can become stale on content-only updates, and chart components can retain global tooltip listeners when the tooltip host is removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 26/28 changed files
- Comments generated: 3
- Review effort level: Lite
d6b8ab0 to
29f58b3
Compare
The time-entry calendar popover and the budget chart tooltips hand-build a Primer Popover-message with a fixed caret that points the wrong way whenever anchored-position flips. Moves the timeline tooltip's caret geometry into a shared module and adds the lit partial and the placement helper the next two commits adopt. The helper runs Primer's own getAnchoredPosition and applies the result at once, since the element only does so in a later frame and never reports which side it chose; the caret is then read off the placed box. https://community.openproject.org/wp/DREAM-813
The time-entry popover carried a fixed left-top caret, so it pointed away from the entry whenever anchored-position placed the popover below or left of it. Derives the caret from the shared placement once the popover has opened and moves the markup into a stateless template. The anchor offset grows from 4px to 8px, the room the caret tip needs, replacing the margin the message used to carry for the same purpose. anchored-position moves an open popover when the window is resized, which would leave the caret stale, so the popover closes on resize as the timeline tooltip does.
The chart tooltips rendered their lit root straight into document.body and positioned themselves by hand with a fixed left-top caret. Renders them into a host owned by the chart component, lets anchored-position place them from the Chart.js caret point and derives the caret once the popover has opened; the tooltip closes on scroll and resize.
Chart.js anchors a bar tooltip on the top edge of the hovered segment, so the popover sat beside the segment's top corner. Anchors it on the segment's box instead, which centres the popover on the segment and points the caret at its middle.
The tooltip renderer was created on the first Chart.js callback and only destroyed with the component, so its scroll and resize listeners outlived the host once the chart lost its data. Creates and destroys the renderer from the host's presence instead.
63a5d01 to
584a8b5
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. |
Note
Stacked on #24893 (SPPM-324). Only the last four commits belong to this PR. Supersedes #24928.
Ticket
https://community.openproject.org/wp/DREAM-813
What are you trying to accomplish?
Two tooltips hand-build a Primer
.Popover-messagewith a fixedPopover-message--left-topcaret: the time-entry calendar popover and the budget chart tooltips. Primer'sanchored-positioncan place a popover on any side, so the caret pointed away from the anchor whenever the popover flipped near a viewport edge. The chart tooltips also rendered their lit root straight intodocument.bodyand positioned themselves by hand.This PR shares the timeline tooltip's caret handling from #24893 with both and moves the chart tooltip into a host owned by the chart component.
Screenshots
My spent time widget
Budgets widgets
What approach did you choose and why?
frontend/src/app/shared/components/anchored-popover/holds three stateless pieces:caretPlacement()— pure geometry, moved from the timeline widget: which popover edge faces the anchor and the offset along it, clamped clear of the rounded corners.popoverMessage()— a lit partial for the.Popover-messagewrapper carrying the caret modifier class and the offset variable.placePopover()— runs Primer's owngetAnchoredPositionright after the popover opens, applies the result to the element and reads the caret off the placed box.anchored-positionapplies the same result in its next animation frame but never reports which side it chose. Applying it at once also keeps the caret in viewport coordinates (a top-layer popover's computed position is document-relative, so a scrolled page would otherwise flip the caret) and pre-empts the frame, which Chart.js would otherwise push past the next paint.@primer/behaviors, which the Primer view components already depend on, becomes a direct dependency for this.There is no
MutationObserverand no frame timing. Each consumer keeps its own trigger lifecycle, popover type and ARIA:popover="hint", opened on hover and focus): derives the caret on open and closes on window resize, sinceanchored-positionmoves an open popover on resize. It stays open on scroll; [OP-20110] Anchor the popover on the visible entry #25124 (OP-20110) anchors it on the entry's visible part and re-places it on scroll, so the caret follows.anchored-positionanchored on aDOMRect— the hovered segment's box for bars, so the popover is centred on the segment (Chart.js itself anchors bar tooltips on the segment's top edge), the Chart.js caret point for pie slices — rendered into a hostdivnext to the canvas, closed on scroll, resize and when Chart.js fades it out. This removes thedocument.bodyrender and the hand-rolled transform.A generic hover-popover abstraction was not built; the three behave too differently.
The shared stylesheet does not set a width: the timeline keeps its
width: auto; max-width: 350px, calendar and chart popovers keep Primer's 232px. Below Primer's 768px breakpoint.Popover-messagehides its caret and forceswidth: autoitself; the former.Popoverbottom-sheet layout no longer applies since neither consumer uses the.Popoverwrapper (the same trade #24893 made for the timeline; the calendar popover is already disabled on mobile).Merge checklist
anchored-popover,te-calendar-popover,chart.config, TestBed specs forte-calendar,actual-costsandbudget-by-cost-type; timeline specs adjusted)Manual QA
document.body.