Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Sep 5, 2025

Fixes the issue where popovers inside nested shadow DOM scroll containers could not properly detect their scroll parents, causing them to scroll incorrectly with grandparent frames instead of staying positioned relative to their triggers.

How to test

Check Storybook out on: https://delightful-beach-055ecb503-1178.westeurope.azurestaticapps.net/?path=/story/uui-popover-container--inside-shadow-dom-scroll-container

Problem

When a popover was placed inside another scrolling target within shadow DOM boundaries, it would fail to bind to the correct scroll parents. This resulted in popovers that would move incorrectly when intermediate scroll containers were scrolled, as reported in #1161.

The issue manifested in scenarios like:

<custom-element>
  #shadow-root
    <div style="overflow: auto"> <!-- outer scroll -->
      <div style="overflow: auto"> <!-- inner scroll -->
        <uui-button popovertarget="my-popover">Open</uui-button>
      </div>
    </div>
    <uui-popover-container id="my-popover">Content</uui-popover-container>
</custom-element>

When scrolling the inner container, the popover would not follow the button correctly.

Root Causes

  1. Duplicate scroll parents: The #getScrollParents() method in uui-popover-container didn't reset the scroll parents array before repopulating it, causing duplicate event listeners and incorrect behavior when the popover was opened multiple times.

  2. Shadow DOM boundary traversal: The older uui-popover component only used parentElement for DOM traversal, which doesn't cross shadow DOM boundaries, missing scroll containers in parent shadow roots.

Solution

For uui-popover-container:

  • Added this.#scrollParents = []; at the beginning of #getScrollParents() to prevent accumulation of duplicate scroll parents
  • Implemented logic to recalculate if static parents should be skipped depending on their position state

For uui-popover:

  • Added the same scroll parent reset logic
  • Implemented _getAncestorElement() helper method that properly traverses shadow DOM boundaries by checking for shadow root hosts when parentElement is null
  • Updated _getScrollParents() to use the new traversal method

Closes #1161

Copilot AI and others added 4 commits September 5, 2025 07:51
Co-authored-by: iOvergaard <752371+iOvergaard@users.noreply.github.com>
- Clear scroll parents array before repopulating to prevent duplicates
- Add shadow DOM traversal support to old popover element
- Enhanced both uui-popover and uui-popover-container components

Co-authored-by: iOvergaard <752371+iOvergaard@users.noreply.github.com>
- Created test cases to verify scroll parent detection
- Added visual demo to showcase the fix
- Verified build and lint pass successfully

Co-authored-by: iOvergaard <752371+iOvergaard@users.noreply.github.com>
Remove demo and extra test files, keeping only the essential fix and enhanced tests

Co-authored-by: iOvergaard <752371+iOvergaard@users.noreply.github.com>
Copilot AI changed the title [WIP] We should find a solution to @umbraco/Umbraco.UI/issues/1161 - a fix has already been attempted once in @umbraco/Umbraco.UI/issues/1006 which has solved some of the issues, but there is still an issue where, if the popover is inside another scrolling t... Fix popover scroll parent detection in nested shadow DOM containers Sep 5, 2025
Copilot AI requested a review from iOvergaard September 5, 2025 08:06
Copilot finished work on behalf of iOvergaard September 5, 2025 08:06
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1178.westeurope.azurestaticapps.net

@iOvergaard iOvergaard marked this pull request as ready for review September 5, 2025 13:14
@iOvergaard iOvergaard added the bug Something isn't working label Sep 5, 2025
@iOvergaard iOvergaard enabled auto-merge (rebase) September 5, 2025 13:14
Copy link
Contributor

Copilot AI left a comment

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 fixes scroll parent detection issues in popovers within nested shadow DOM containers. The main problem was that popovers couldn't properly identify their correct scroll parents across shadow DOM boundaries, causing incorrect positioning when intermediate scroll containers were scrolled.

  • Prevents duplicate scroll parent accumulation when popovers are opened multiple times
  • Adds proper shadow DOM boundary traversal for scroll parent detection
  • Implements conditional logic for excluding static parents based on position state

Reviewed Changes

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

File Description
packages/uui-popover/lib/uui-popover.element.ts Adds scroll parent reset logic and shadow DOM traversal helper method
packages/uui-popover-container/lib/uui-popover-container.element.ts Fixes duplicate scroll parents and improves static parent exclusion logic
packages/uui-popover-container/lib/uui-popover-container.test.ts Adds comprehensive tests for nested shadow DOM scroll parent detection
packages/uui-popover-container/lib/uui-popover-container.story.ts Refactors story configuration and minor styling improvements

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1178.westeurope.azurestaticapps.net

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 5, 2025

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1178.westeurope.azurestaticapps.net

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2025

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-beach-055ecb503-1178.westeurope.azurestaticapps.net

@iOvergaard iOvergaard merged commit a88759c into main Oct 6, 2025
11 of 12 checks passed
@iOvergaard iOvergaard deleted the copilot/fix-4b97baae-004b-4cd9-935c-f63c41208434 branch October 6, 2025 08:25
@rammi987
Copy link
Contributor

rammi987 commented Oct 10, 2025

@nielslyngsoe @iOvergaard i have testede it again also with the detail comment her #1006 (comment) and it still is not fixed :(
I testede i Umbraco 16.3.0-rc2 and UUI 1.16.0rc

@iOvergaard
Copy link
Contributor

@rammi987 the proposed fix is set to release in Umbraco 16.4. Can I ask how did you manage to test the new version on 16.3, did you build from source?

@rammi987
Copy link
Contributor

@iOvergaard we Got told on the DR project by our umbraco contact it was a 16.3 fix.
How ever our project is with a lot of custom sections so our package.json have a reference to umbraco/uui - so I just target it for 1.16.0-rc0 and tested

@iOvergaard
Copy link
Contributor

@rammi987 The first part of the fix was in 16.2, but the part that was not fixed the first time around is in UI library 1.16, which will land in CMS 16.4 and 17.0. Since the CMS includes the UI library, it is not possible to overwrite it in the browser even though your package has a reference to it. You will need to build the whole CMS from source to bump up the version of the UI library in the browser.

The beta of 17.0, which also contains the supposed fix, will be available on NuGet next week. So, if have the chance to test it then, that would be very beneficial. We will then have the chance to fix up any further bugs before the final release.

@rammi987
Copy link
Contributor

@iOvergaard thx for the detailed answer it make more sense. I’ll put our internal task about the bug to 16.4 and test it again at that time.

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.

popover container does not scroll along in certain case

4 participants