Skip to content

fix(tree): prevent tooltip for non-truncated option text (#DS-4931) - #1910

Open
KamilEmeleev wants to merge 2 commits into
mainfrom
fix/DS-4931
Open

fix(tree): prevent tooltip for non-truncated option text (#DS-4931)#1910
KamilEmeleev wants to merge 2 commits into
mainfrom
fix/DS-4931

Conversation

@KamilEmeleev

@KamilEmeleev KamilEmeleev commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Tree options showed a tooltip even when the text was not truncated. KbqTreeOption exposed only
parentTextElement, so KbqTitleDirective measured the .kbq-option-text container against the
option host — which is always bigger, so every option looked overflown.

Reported on kbq-username, but it is unchanged and correct: plain-text options were affected too.

List of notable changes:

  • fixed tooltip without truncation, by exposing textElement so the clipping box is measured against itself
  • added two unit tests: no tooltip when the text fits, tooltip when it is clipped

What should reviewers focus on?

Content marking its own #kbqTitleText keeps priority, so tree-select and filter-bar should be unaffected.

Trade-off: with parent === child a sub-pixel clip is no longer reported for tree options that do not
mark their own #kbqTitleText. Previously that case reported truncation unconditionally, so it goes
from always shown to never shown. Content marking its own text keeps both the sub-pixel branch and the
hasEllipsis check intact.

@KamilEmeleev KamilEmeleev added the bug Something isn't working label Aug 17, 2026
@KamilEmeleev KamilEmeleev changed the title fix(tree): prevent tooltip for non-truncated option text fix(tree): prevent tooltip for non-truncated option text (#DS-4931) Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Visit the preview URL for this PR (updated for commit 482a6d4):

https://koobiq-next--prs-1910-ab389bc7.web.app

(expires Thu, 27 Aug 2026 12:04:06 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c

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

Fixes incorrect kbq-title tooltip activation for KbqTreeOption when option text is not truncated. The issue came from KbqTitleDirective falling back to measuring the option host element (larger due to padding/checkbox), which made non-truncated text appear “overflown”.

Changes:

  • Exposes textElement on KbqTreeOption so KbqTitleDirective measures overflow against the .kbq-option-text clipping container.
  • Adds unit tests covering “no tooltip when text fits” and “tooltip when text is clipped”.
  • Updates the public API snapshot for the Tree component.

Reviewed changes

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

File Description
tools/public_api_guard/components/tree.api.md Public API snapshot updated to include KbqTreeOption.textElement.
packages/components/tree/tree-selection.component.spec.ts Adds Jest tests validating overflow detection behavior for kbq-title on tree options.
packages/components/tree/tree-option.component.ts Adds textElement view query to allow KbqTitleDirective to measure the correct clipping element.

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

@NikGurev NikGurev 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.

Pointing both parentTextElement and textElement at the same #kbqTitleContainer isn't a mistake — it's parent.offsetWidth < child.scrollWidth degenerating into el.offsetWidth < el.scrollWidth on one element, which is exactly the check needed here.

@artembelik

Copy link
Copy Markdown
Contributor

/redeploy-preview

@github-actions

Copy link
Copy Markdown

🔄 Redeploying documentation preview.

@artembelik

Copy link
Copy Markdown
Contributor

@lskramarov lskramarov 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.

Reviewed the fix. The approach is sound and I verified both new tests are genuine regression tests — they fail when textElement is removed and pass with it. prettier, eslint and the API-guard entry are clean.

Eight notes below: 7 inline, plus one that lands in a file outside this diff and so has to live here.


packages/components/title/title.directive.ts:181-184 — the hasEllipsis rationale is now stale

The JSDoc justifies checking both elements like this:

Both elements are checked because text-overflow is not inherited and consumers place it differently: KbqOption/KbqDropdownItem style the measured child, whereas KbqTreeOption styles the parent container that wraps the child.

KbqTreeOption was the only cited case for the elementHasEllipsis(this.child) || elementHasEllipsis(this.parent) disjunction — and after this PR its parent is its child, so nothing wraps anything. The sentence describes an arrangement that no longer exists, and the || is redundant for the one component it was written for.

AGENTS.md ("Comments"): "Update or remove comments that become outdated because of your changes."

Comment thread packages/components/tree/tree-option.component.ts Outdated
Comment thread packages/components/tree/tree-option.component.ts Outdated
Comment thread packages/components/tree/tree-option.component.ts Outdated
Comment thread packages/components/tree/tree-option.component.ts Outdated
Comment thread packages/components/tree/tree-selection.component.spec.ts
Comment thread packages/components/tree/tree-selection.component.spec.ts
Comment thread packages/components/tree/tree-selection.component.spec.ts Outdated
@lskramarov
lskramarov requested a balanced review from Copilot August 24, 2026 09:43

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants