Fixes tooltip directive attribute leak

Miguel Rinconrequested to merge
fix-tooltip-directive-attribute-leak into main

What does this MR do?

The Bootstrap Vue tooltip directive (v-gl-tooltip) copied attributes from a nearby, unrelated component onto the tooltip element. Test selectors and accessibility attributes therefore landed in the wrong place. This MR adds an isDirective flag to tell the directive usage apart from the component usage, and stops the attribute copy for the directive.

The problem

The tooltip renders a <div>. The code that builds that element's attributes (bv-tooltip-template.js, in templateAttributes()) spread this.bvParent.bvParent.$attrs — the attributes of the component two levels up the Vue parent chain.

For the component usage this is correct. For the directive usage there is no tooltip component in the chain at all:

Usage bvParent bvParent.bvParent Attributes copied from it
Component (<gl-tooltip>, <gl-popover>) BVTooltip BTooltip, the tooltip component itself (inheritAttrs: false) Intended. This is what puts attributes set on <gl-tooltip> on the tooltip.
Directive (v-gl-tooltip) BVTooltip the application component that holds the target element Unrelated. This is the bug.

So the tooltip received the fallthrough attributes of whichever component happened to hold the target element. Attributes such as data-testid and aria-label, meant for a different element, appeared on the tooltip. Because the tooltip renders as a child of <body>, a test selector could match it instead of the intended element. aria-* attributes on the tooltip are also an accessibility problem.

The intended component behaviour comes from upstream Bootstrap Vue issue https://github.com/bootstrap-vue/bootstrap-vue/issues/5836, where <b-popover data-test="test"> dropped data-* attributes. That behaviour is kept.

Vue 3 makes the leak worse, because it forwards fallthrough attributes automatically through chains of single-root child components, so an attribute travels further than on Vue 2. When consumers stop using Vue 2 compatibility mode, class and style also become fallthrough attributes and would leak the same way.

A real failure this caused: https://gitlab.com/gitlab-org/gitlab/-/jobs/16018296488

The fix

Three vendored files, six lines:

  • bv-tooltip.js: added isDirective: false to the templateData object. It sits next to the existing html flag, which is already documented as used by directives only.
  • directives/tooltip/tooltip.js: the directive now sets isDirective: true.
  • bv-tooltip-template.js: the spread is now ...(this.bvParent.isDirective ? {} : this.bvParent.bvParent.$attrs).

The templateData → updateData channel was chosen on purpose. It is the one path already proven to behave the same under Vue 2 and under @vue/compat. BVTooltip declares no props, so all of its state already flows through templateData.

The component path is untouched. Popover needs no change: BVPopoverTemplate extends BVTooltipTemplate and BPopover extends BTooltip, so isDirective stays false. There is no popover directive in the vendored tree.

Tests

A regression test was added to the vendored directive spec. A host component receives data-testid and aria-label, and wraps the element that carries the directive. The test asserts those attributes do not reach the tooltip. It includes a positive control asserting the attributes do land on the host's own element, so it cannot pass for the wrong reason.

A guard test was added to the vendored component spec. It asserts that an attribute set on <b-tooltip> still reaches the tooltip. Nothing covered that upstream behaviour before, so it was unprotected.

The local helper waitForTooltipAfterClickOn gained an optional second argument for the root wrapper. On Vue 3, find('button') returns a plain element wrapper with no vm. Existing callers are unchanged.

Both new tests were written before the fix and confirmed to fail first. Worth noting: the regression test fails on Vue 2 as well as Vue 3. The issue described this as a Vue 3 problem, but Vue 3 only widens an existing leak.

The full @gitlab/ui unit suite passes on both runtimes:

  • Vue 2: 236 of 236 suites, 4180 tests passed.
  • Vue 3: 235 of 236 suites, 4174 tests passed. The one skipped suite is a pre-existing describe.skip in src/directives/tooltip/tooltip.spec.js, unrelated to this change.

Consumer impact

A changeset is included, as a minor bump. Tooltips created by the directive no longer carry the copied attributes in their markup. A project whose test selected a directive tooltip through such a leaked attribute must update that test.

Follow-up in gitlab-org/gitlab, not part of this MR: once a release with this fix lands there, a workaround in spec/features/work_items/issues/issue_sidebar_spec.rb can be reverted. It currently qualifies a scope by tag name, within('section[data-testid="work-item-labels"]'), to avoid matching the tooltip.

Fixes #3623 (closed)

Screenshots or screen recordings

No visual change. The change only removes attributes from the tooltip's markup in the directive case.

Integrations

An integration merge request is open to check this change against GitLab's own test suite. It must not be merged. Any pipeline fix it needs will be cherry-picked into the Renovate dependency update merge request, after the new @gitlab/ui version is published.

Does this MR meet the acceptance criteria?

Toggle the acceptance checklist

Conformity

  • The “What does this MR do?” section in the MR description is filled out, explaining the reasons for and scope of the proposed changes, per “Say why not just what”.
  • Relevant label(s) are applied to the MR.
  • The MR is added to a milestone.
  • Added the ~"component:*" label(s) if applicable.
  • A changeset is added when this MR will do a patch, minor, or major update. More information can be found in doc/changesets.md.

Components

Accessibility

  • All content is presented in text or with a text equivalent. This MR removes aria-* attributes that were applied to the wrong element.
Edited by Miguel Rincon

Merge request reports