[DREAM-813] Share anchored popover caret placement by myabc · Pull Request #24928 · opf/openproject

myabc · 2026-08-25T05:12:54Z

Note

Stacked on #24893 — review only the commits after its head.

Ticket

https://community.openproject.org/wp/DREAM-813

What are you trying to accomplish?

Three tooltips hand-built a Primer .Popover-message with a caret: the project timeline widget (#24893), the time-entry calendar popover and the budget chart tooltips. Primer's anchored-position does not expose which side it settled on and repositions on scroll and resize, so the calendar's and the charts' fixed Popover-message--left-top caret pointed the wrong way whenever the popover flipped. The chart tooltips also positioned themselves by hand and rendered their lit root straight into document.body.

This PR extracts the timeline's caret handling into a small shared primitive and moves all three consumers onto it.

Screenshots

Calendar popover before/after (padding, offset and the flipped caret near the viewport edge):
Budget chart tooltip before/after near the right edge:

What approach did you choose and why?

frontend/src/app/shared/components/anchored-popover/ holds three pieces: caretPlacement() (pure geometry, moved from the timeline), popoverMessage() (a lit partial for the .Popover-message wrapper carrying the caret class and offset) and syncCaret() (a MutationObserver on the popover's inline style, which is where anchored-position writes its position). Consumers keep their own trigger lifecycle, popover type and ARIA roles — a generic hover-popover abstraction was deliberately not built, the three behave too differently.

The budget charts go one step further: instead of a fixed transform from the Chart.js caret point they use anchored-position anchored on a DOMRect at that point (Primer's getAnchoredPosition accepts Element | DOMRect; the custom element's setter only narrows the type). That removes the hand-rolled positioning and fade, and the tooltip now renders into a host element owned by the chart component.

The caret synchroniser clears its cached placement whenever the popover is observed closed, so reopening a tooltip on the same item re-applies the caret; each consumer disconnects it on teardown.

Important

Primer's .Popover-message mobile styles (< 768px) hide the caret for every consumer; unchanged, same note as #24893.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)
Manual QA — time-entry calendar: hover opens with caret; narrow window flips popover and caret; re-hover same entry keeps caret; Tab focus opens it
Manual QA — budget charts: hover bar → leave canvas → hover same bar again shows caret both times; pie slices in sequence; right-edge flip; leaving canvas closes; no anchored-position left under document.body
Manual QA — dashboard timeline: hover, leave, re-hover same phase keeps caret
Firefox pass on all three

The timeline tooltip's caret geometry is generic: it only needs the popover and anchor rectangles. Moves it under a shared anchored-popover directory so the calendar and budget chart tooltips can use it too. https://community.openproject.org/wp/DREAM-813

Renders the Popover-message wrapper with the caret modifier class and offset custom property from a caret placement, so consumers stop hand-writing the same markup.

Watches the popover's inline style, which is where anchored-position writes its position, and recomputes the caret placement after every reposition so flips and shifts never leave the caret pointing at nothing.

Moves the caret offset wiring out of the timeline widget so every consumer of the popover message partial gets the same caret behaviour.

Replaces the inline message template and observer with the shared partial and synchroniser. No behaviour change.

The time-entry popover carried a fixed left caret that pointed the wrong way whenever anchored-position flipped it. Renders it through the shared message partial and keeps the caret in sync on reposition. The popover now uses the Primer message padding and body font size and sits eight pixels further from the entry to make room for the caret.

The chart tooltips positioned themselves with a fixed transform from the Chart.js caret point, rendered their lit root straight into document.body and carried a caret that never flipped. Anchors them on a DOMRect at the caret point through Primer's anchored-position, renders into a host owned by the chart component and shares the caret handling with the other popovers.

The synchroniser kept the last applied placement for the element's lifetime, so reopening a tooltip on the same item with identical geometry never re-applied the caret. Clears it whenever the popover is observed closed.

Re-creates the chart tooltip when Angular swaps its host element, disconnects its caret synchroniser on destroy, and moves the pointer-events reset out of the shared host style so the calendar's dialog popover keeps selectable text.

github-actions · 2026-08-25T05:38:47Z

Warning

Flaky specs

rspec ./spec/features/work_packages/share/share_spec.rb[1:5:1]
rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]
rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:2]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24928, linked for reference only):

- `rspec ./spec/features/work_packages/share/share_spec.rb[1:5:1]`
- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]`
- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:2]`

Treat this as a standalone task, unrelated to PR #24928. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24928 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Copilot

Pull request overview

Refactors timeline, calendar, and budget-chart tooltips to share anchored-popover caret handling and positioning.

Changes:

Adds shared caret geometry, rendering, synchronization, and styles.
Migrates tooltip consumers to anchored positioning.
Adds coverage for caret behavior and tooltip rendering.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Change	Final review comment
`frontend/src/global_styles/common/openproject-common.module.sass`	Imports shared popover and chart styles.	—
`frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.ts`	Uses shared caret synchronization.	—
`frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.spec.ts`	Tests caret reuse after reopening.	—
`frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.sass`	Applies shared tooltip styling.	—
`frontend/src/app/shared/components/budget-graphs/overview/budget-by-cost-type.component.ts`	Hosts pie-chart tooltips.	—
`frontend/src/app/shared/components/budget-graphs/overview/budget-by-cost-type.component.html`	Adds the pie tooltip host.	—
`frontend/src/app/shared/components/budget-graphs/overview/actual-costs.component.ts`	Hosts bar-chart tooltips.	—
`frontend/src/app/shared/components/budget-graphs/overview/actual-costs.component.html`	Adds the bar tooltip host.	—
`frontend/src/app/shared/components/budget-graphs/chart.config.ts`	Implements anchored chart tooltips.	Moderate (3 votes, L88): The synthetic `DOMRect` can become stale after scrolling or resizing. Nit (2 votes, L135): Add bar-renderer coverage for date/value output and lifecycle.
`frontend/src/app/shared/components/budget-graphs/chart.config.spec.ts`	Tests chart tooltip behavior.	—
`frontend/src/app/shared/components/budget-graphs/budget-graphs.sass`	Styles chart popovers.	—
`frontend/src/app/shared/components/anchored-popover/popover-message.ts`	Provides the shared popover message partial.	—
`frontend/src/app/shared/components/anchored-popover/popover-message.spec.ts`	Tests message and caret rendering.	—
`frontend/src/app/shared/components/anchored-popover/caret-sync.ts`	Synchronizes caret placement.	—
`frontend/src/app/shared/components/anchored-popover/caret-sync.spec.ts`	Tests synchronization lifecycle.	—
`frontend/src/app/shared/components/anchored-popover/caret-placement.ts`	Provides caret geometry.	—
`frontend/src/app/shared/components/anchored-popover/caret-placement.spec.ts`	Tests caret geometry.	—
`frontend/src/app/shared/components/anchored-popover/anchored-popover.sass`	Defines shared popover styles.	—
`frontend/src/app/features/calendar/te-calendar/te-calendar.component.ts`	Adopts shared popover rendering and synchronization.	Moderate (2 votes, L606): After the schema await, the event or component may be gone; verify the anchor and popover remain connected before observing, and clean up the map on teardown.
`frontend/src/app/features/calendar/te-calendar/te-calendar-popover.spec.ts`	Tests calendar popover markup and caret classes.	—

Suppressed comments (2)

frontend/src/app/features/calendar/te-calendar/te-calendar.component.ts:148

The old calendar message applied p-2 text-small, but this replacement passes only the shared wrapper and the calendar has no replacement rule for those values (the chart consumer adds one explicitly). The message therefore falls back to Primer's default padding and font size instead of retaining the compact calendar presentation. Preserve the calendar-specific padding and typography on the new message.

      ${popoverMessage(list, caret)}

frontend/src/app/shared/components/budget-graphs/chart.config.ts:90

This clears the rendered caret on every external tooltip update, but syncCaret retains its internal last placement. When Chart.js invokes the renderer again for the same active point, the placement can equal last, so the observer suppresses the callback and the tooltip remains without a caret after draw() removes it. Reset the synchronizer cache when clearing the caret, or keep the existing caret until a new placement is applied.

    this.caret = null;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    anchorEl.setAttribute('popovertarget', popoverId);

    const popoverEl = document.getElementById(popoverId)!;
+    const stopCaretSync = syncCaret(popoverEl, () => anchorEl.getBoundingClientRect(), draw);


+    }
+
+    const { left, top } = context.chart.canvas.getBoundingClientRect();
+    this.anchor = new DOMRect(Math.round(left + context.tooltip.caretX), Math.round(top + context.tooltip.caretY), 0, 0);


-  return function(context:TooltipContext<'bar'>) {
-    const { tooltip } = context;
-    const items = tooltip.dataPoints.map((dp, i) => {
+export function createBarTooltipRenderer(host:HTMLElement, formatCurrency:FormatCurrency) {


myabc added 9 commits August 23, 2026 20:04

Add a shared Primer popover message partial

34a2010

Renders the Popover-message wrapper with the caret modifier class and offset custom property from a caret placement, so consumers stop hand-writing the same markup.

Add a caret synchroniser for anchored popovers

c8ee8df

Watches the popover's inline style, which is where anchored-position writes its position, and recomputes the caret placement after every reposition so flips and shifts never leave the caret pointing at nothing.

Add shared styles for anchored popover carets

942dcc4

Moves the caret offset wiring out of the timeline widget so every consumer of the popover message partial gets the same caret behaviour.

Use the shared popover caret in timeline tooltip

710a596

Replaces the inline message template and observer with the shared partial and synchroniser. No behaviour change.

Reset the caret cache when a popover closes

e0df5ef

The synchroniser kept the last applied placement for the element's lifetime, so reopening a tooltip on the same item with identical geometry never re-applied the caret. Clears it whenever the popover is observed closed.

Tidy chart tooltip lifecycle and pointer policy

76b1166

Re-creates the chart tooltip when Angular swaps its host element, disconnects its caret synchroniser on destroy, and moves the pointer-events reset out of the shared host style so the calendar's dialog popover keeps selectable text.

myabc changed the title ~~feature/dream 813 anchored popover caret~~ [DREAM-813] Share anchored popover caret placement Aug 25, 2026

myabc added the DO NOT MERGE label Aug 25, 2026

myabc requested a lite review from Copilot August 25, 2026 05:39

Copilot started reviewing on behalf of myabc August 25, 2026 05:42 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Navigation Menu

Uh oh!

[DREAM-813] Share anchored popover caret placement - #24928

[DREAM-813] Share anchored popover caret placement#24928
myabc wants to merge 9 commits into
feature/agile-303-work-package-overview-on-sprint-report-page-altfrom
feature/dream-813-anchored-popover-caret

myabc commented Aug 25, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

myabc commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

myabc commented Aug 25, 2026 •

edited

Loading