Skip to content

[SPPM-324] Show timeline tooltip in the top layer - #24893

Open
myabc wants to merge 2 commits into
devfrom
feature/agile-303-work-package-overview-on-sprint-report-page-alt
Open

[SPPM-324] Show timeline tooltip in the top layer#24893
myabc wants to merge 2 commits into
devfrom
feature/agile-303-work-package-overview-on-sprint-report-page-alt

Conversation

@myabc

@myabc myabc commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Ticket

https://community.openproject.org/wp/SPPM-324

What are you trying to accomplish?

The project timeline widget renders its hover tooltip via vis-timeline, which appends it inside .vis-timeline. That element sits in a dashboard grid cell (.grid--area) that both clips with overflow: hidden and owns a z-index stacking context, so a long milestone name was cut off and could be painted behind the widget below.

The tooltip now renders as a popover in the browser's top layer, which is immune to both ancestor clipping and ancestor stacking contexts. It stays fully visible regardless of the milestone name length or position, flips below the item near the top of the viewport, and shifts sideways at the viewport edges while keeping its caret over the hovered item.

Screenshots

Before After
Screenshot 2026-08-23 at 15 39 29 Screenshot 2026-08-23 at 15 40 32

Screen recording: different screen sizes

Screen.Recording.2026-08-23.at.15.41.08.mov

What approach did you choose and why?

vis-timeline hard-wires its tooltip to the timeline root (new Popup(this.body.dom.root, …)) and offers no container or callback option, so there is no library-native way to render it elsewhere. Instead of relocating vis's private tooltip element into a position: fixed body-level layer and reimplementing its geometry (#24700), this PR uses the library's documented off-switch (showTooltips: false) and renders the tooltip itself from the itemover/itemout events.

The tooltip is a Primer <anchored-position popover="manual"> wrapping a .Popover-message (for the caret), rendered with lit-html — the same pattern the time-entry calendar uses for its hover popovers (te-calendar.component.ts). The existing ProjectTimelineTooltipBuilder keeps building the content (vis still calls it through Item#getTitle(), which also covers clustered gates) and now also owns the popover markup (popoverTemplate), while the popover lifecycle (host, hover delay, caret, teardown) lives in a sibling class, ProjectTimelineTooltipPopover. The component only wires the two to the timeline.

Consequences of this choice:

  • No portal, no manual viewport maths, no z-index escalation and no scrollbar-gutter edge case: the top layer handles clipping and stacking, anchored-position handles flip/shift.
  • The popover remains a DOM child of the aria-hidden timeline- Primer's anchored-position does not expose which side it settled on (it can fall back to below, right or left of the anchor) and positions the popover in an animation frame it requests on open. The caret is derived once, one frame after opening, from the popover and anchor rectangles (caretPlacement() in project-timeline-tooltip-caret.ts): the side facing the anchor, and the offset along that edge clamped clear of the rounded corners. The tooltip closes on scroll and resize rather than following a repositioned popover; it also hides on timeline pan/zoom and before the data is replaced. One popover element lives for the component lifetime.t.ts`): the side facing the anchor, and the offset along that edge clamped clear of the rounded corners. One popover element lives for the component lifetime; the tooltip hides on timeline pan/zoom and before the data is replaced.
  • The only vis internal touched is timeline.itemSet.getItemById(id).getTitle() (typed via a small local interface), needed because the itemover event only carries the item id and cluster items exist solely inside vis.

Relaxing .grid--area { overflow: hidden } was considered and rejected: every dashboard widget relies on it.

Important

On viewports narrower than 768px the tooltip has no caret. Primer's .Popover-message mobile styles (@media (max-width: 767.98px)) hide the caret and lay the popover out as a sheet. This is left as is: hover tooltips are of little use on touch devices, and overriding Primer's mobile rules for a hover-only element is not worth the coupling.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@myabc
myabc requested review from HDinger and bsatarnejad and a lite review from Copilot August 23, 2026 23:36
@myabc
myabc marked this pull request as ready for review August 23, 2026 23:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the project timeline widget to render hover tooltips as a Primer anchored-position popover in the browser top layer (instead of vis-timeline’s internal tooltip), preventing clipping/stacking issues inside dashboard grid cells.

Changes:

  • Disables vis-timeline’s built-in tooltip rendering and implements a custom hover tooltip driven by itemover/itemout events.
  • Renders the tooltip via lit-html into an anchored-position popover="manual" and post-processes geometry to place the caret.
  • Updates tooltip styling to match Primer popover visuals and adds unit tests for tooltip behavior.

Reviewed changes

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

File Description
frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.ts Disables vis tooltips, adds custom popover tooltip rendering and caret alignment logic.
frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.spec.ts Adds unit tests covering tooltip open/close behavior and aria-hidden containment.
frontend/src/app/shared/components/project-timeline-graph/project-timeline-graph.component.sass Replaces vis-tooltip styling with styles for the new Primer popover-based tooltip.

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

@myabc myabc added bugfix needs review javascript Pull requests that update Javascript code labels Aug 23, 2026
@github-actions

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:1:1:1]
  • rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:3:2:1:1]
  • rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:3:2:3: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 #24893, linked for reference only):

- `rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:1:1:1]`
- `rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:3:2:1:1]`
- `rspec ./modules/documents/spec/features/attachment_upload_spec.rb[1:3:2:3: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 #24893. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24893 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.

@myabc myabc added this to the 17.9.x milestone Aug 24, 2026
@myabc
myabc force-pushed the feature/agile-303-work-package-overview-on-sprint-report-page-alt branch 2 times, most recently from a600c95 to a3d0efe Compare August 24, 2026 01:12

@HDinger HDinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am lacking a bit the history of that bug ticket. As far as I can see, there have been many iterations already, so I will not challenge the necessity to create a manual Popover element in JS (although I am not a big fan of it).
Functionalitywise, it looks fine to me. The only thing I'd like to challenge is the code placement. We already have a tooltipBuilder class (and file) for this widget. I guess, the new code belongs there.

@myabc
myabc force-pushed the feature/agile-303-work-package-overview-on-sprint-report-page-alt branch from a3d0efe to 5c363fa Compare September 3, 2026 11:08
@myabc
myabc requested a review from HDinger September 3, 2026 11:13
@myabc

myabc commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

@HDinger I've moved the tooltip code beside the builder: the popover markup is now ProjectTimelineTooltipBuilder#popoverTemplate, the lifecycle (host, delay, caret, teardown) a sibling ProjectTimelineTooltipPopover; the component only wires it up. I kept them separate since the builder is a stateless factory vis also calls for cluster titles. I could still fold that into one class if you prefer.

I've also dropped the MutationObserver, which drastically simplifies things: the caret position is derived once on open, tooltip now closes on scroll/resize.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new popover controller currently does not unregister its vis-timeline event listeners on teardown, which can cause stale callbacks and memory retention if the timeline outlives the controller.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]
  • rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]
🤖 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 #24893, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`
- `rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #24893. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24893 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.

@myabc
myabc force-pushed the feature/agile-303-work-package-overview-on-sprint-report-page-alt branch 2 times, most recently from 3fa6439 to 671a919 Compare September 3, 2026 19:02
vis-timeline renders its hover tooltip inside the timeline root, where
the dashboard grid cell clips it and paints it behind the next widget,
so long milestone names were cut off. Disables the vis tooltip and
renders a Primer anchored-position popover from the itemover/itemout
events instead: the top layer escapes both the clipping and the
stacking context, and the popover stays inside the aria-hidden
container so the sr-only list remains the only accessible version.

Primer's anchored-position does not announce which side it settled on,
so the caret side and offset are derived from the popover and anchor
rectangles once the popover has opened; the tooltip closes on scroll
and resize instead of following the repositioned popover.

https://community.openproject.org/wp/SPPM-324
The popover relied on the component destroying the timeline right
after it, which drops every listener. Removes its own handlers and
tolerates a hide after its host left the document, so the class no
longer depends on its caller's teardown order.
@myabc
myabc force-pushed the feature/agile-303-work-package-overview-on-sprint-report-page-alt branch from 671a919 to ff0c18f Compare September 4, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix javascript Pull requests that update Javascript code needs review

Development

Successfully merging this pull request may close these issues.

3 participants