Skip to content

chore(components): update tooltip to have accessible name #5551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

myrta2302
Copy link
Contributor

@myrta2302 myrta2302 commented May 28, 2025

📄 Description

This PR updates the <post-tooltip> component to improve accessibility by moving the role attribute from the <post-tooltip> to the nested <post-popovercontainer> component so that the text inside is correctly recognized as the accessible name.

📝 Checklist

  • ✅ My code follows the style guidelines of this project
  • 🛠️ I have performed a self-review of my own code
  • ⚠️ My changes generate no new warnings or errors
  • ✔️ New and existing unit tests pass locally with my changes

@myrta2302 myrta2302 linked an issue May 28, 2025 that may be closed by this pull request
Copy link

changeset-bot bot commented May 28, 2025

🦋 Changeset detected

Latest commit: d5614c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@swisspost/design-system-components Patch
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components-react Patch
@swisspost/design-system-documentation Patch
@swisspost/design-system-components-angular Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Patch
@swisspost/design-system-tokens Patch
@swisspost/design-system-icons Patch
@swisspost/design-system-styles-primeng Patch
@swisspost/internet-header Patch
@swisspost/design-system-styles-primeng-workspace Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented May 28, 2025

Related Previews

@myrta2302 myrta2302 changed the title chore(components): update tooltip with accessible name chore(components): update tooltip to have accessible name Jun 2, 2025
myrta2302 and others added 2 commits June 2, 2025 09:59
@myrta2302 myrta2302 marked this pull request as ready for review June 2, 2025 10:36
@myrta2302 myrta2302 requested a review from a team as a code owner June 2, 2025 10:36
@myrta2302 myrta2302 requested a review from oliverschuerch June 2, 2025 10:36
Comment on lines 47 to 56
if (node.nodeType === Node.TEXT_NODE) {
return node.textContent?.trim() ?? '';
}
if (node.nodeType === Node.ELEMENT_NODE) {
const element = node as Element;
return Array.from(element.childNodes)
.map(child => this.extractText(child))
.join(' ');
}
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo a switch statement would fit your needs 100% here and also increase readability and performence.
https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Statements/switch

But also, where is this method used, I couldn't find any execution of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I had a different implementation before (that added the text in an aria-label) and forgot to remove this code.

@myrta2302 myrta2302 requested a review from oliverschuerch June 12, 2025 08:36
Copy link
Contributor

@oliverschuerch oliverschuerch left a comment

Choose a reason for hiding this comment

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

We did the exact opposite in the the commit b16ba09.
That's why I would like to hear @gfellerph opinion about this change.

@oliverschuerch oliverschuerch requested a review from gfellerph June 17, 2025 07:48
Copy link

sonarqubecloud bot commented Jul 3, 2025

Copy link
Contributor

@oliverschuerch oliverschuerch left a comment

Choose a reason for hiding this comment

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

Hi Myrta

As discussed before here, Philipp did the exact opposite in February 2024.

image

So my question still is, why did we change it back then and need to revert it now?
Will we fix one bug and open a new one with this change or should it fix the issue once and for all time?
Also we should explain the reason for the change in the changeset imo.

@myrta2302
Copy link
Contributor Author

myrta2302 commented Jul 18, 2025

We did the exact opposite in the the commit b16ba09. That's why I would like to hear @gfellerph opinion about this change.

image

I am not sure why it was moved before. The accessible name is visible in the Accessibility tab only when the role="tooltip" is set to the <popovercontainer>.

@gfellerph
Copy link
Member

When it's on the tooltip host, the tooltip is empty on startup because the popover is hidden. This gives an accessibility error of empty tooltip. With the role on the container, the thing works. My change was a regression.

Copy link

@myrta2302 myrta2302 merged commit da75f0c into main Aug 18, 2025
13 checks passed
@myrta2302 myrta2302 deleted the 5458-bug-tooltip-host-element-not-hidden-and-interfering-with-layout branch August 18, 2025 06:51
gfellerph pushed a commit that referenced this pull request Aug 18, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`main` is currently in **pre mode** so this branch has prereleases
rather than normal releases. If you want to exit prereleases, run
`changeset pre exit` on `main`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @swisspost/[email protected]

### Major Changes

- Renamed the following CSS custom properties: -
`--post-global-header-height` → `--post-global-header-expanded-height`
- `--post-local-header-height` → `--post-local-header-expanded-height`
- `--post-local-header-min-height` →
`--post-local-header-expanded-min-height` (by
[@alizedebray](https://github.com/alizedebray) with
[#5933](#5933))

### Minor Changes

- Internalized bootstrap z-index utilities. (by
[@hugomslv](https://github.com/hugomslv) with
[#5741](#5741))

## @swisspost/[email protected]

### Minor Changes

- Enabled use of the `post-header` component without requiring a
`post-mainnavigation`. (by
[@alizedebray](https://github.com/alizedebray) with
[#5933](#5933))

### Patch Changes

- Updated `<post-tooltip>` by moving the `role` attribute from the
`<post-tooltip>` to the nested `<post-popovercontainer>` component to
improve accessibility. (by [@myrta2302](https://github.com/myrta2302)
with [#5551](#5551))
-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Minor Changes

- Added icons: `2696` and `2697` (by
[@swisspost-bot](https://github.com/swisspost-bot) with
[#5993](#5993))

### Patch Changes

-   Updated icons:
    -   `help` (line & solid): 16, 24, 32, 40, 48 and 64px
    -   `info` (line & solid): 16, 24, 32, 40, 48 and 64px
    -   `success` (line & solid): 16, 24, 32, 40, 48 and 64px
- `warning` (line & solid): 16, 24, 32, 40, 48 and 64px (by
[@swisspost-bot](https://github.com/swisspost-bot) with
[#6005](#6005))

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]



## @swisspost/[email protected]

### Minor Changes

- Internalized bootstrap z-index utilities. (by
[@hugomslv](https://github.com/hugomslv) with
[#5741](#5741))

### Patch Changes

- Enabled use of the `post-header` component without requiring a
`post-mainnavigation`. (by
[@alizedebray](https://github.com/alizedebray) with
[#5933](#5933))
-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: Tooltip host element not hidden and interfering with layout
4 participants