Skip to content
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

fix(overlay): returning only the first element for slot assignedElements #4930

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zolar
Copy link

@zolar zolar commented Nov 11, 2024

Only return first element form assignedElements instead of an array of elements.

Description

This PR reverses a helper method that was used in the previous version of the OverLayTrigger. In that version, the slotchange handler uses assignedNodes instead of assignedElements. I think using assignedElements renders the slot so fast, that our dialog component has not completed loading, leading to forcing a reload, in which it does goes threw the same process.

private extractSlotContentFromEvent(event: Event): HTMLElement | undefined {

Related issue(s)

[Bug]: OverlayTrigger can get stuck in a render loop causing page to crash

Motivation and context

Resolves an issue related to endless render loop.

How has this been tested?

  • Did it pass in Desktop?
  • Did it pass in Mobile?
  • Did it pass in iPad?

Screenshots (if appropriate)

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@zolar zolar requested a review from a team as a code owner November 11, 2024 18:20
@Rajdeepc
Copy link
Contributor

We would like to see you use case here considering this is very specific. Can you add a video or a repro to show how does this solve your issue?

@zolar
Copy link
Author

zolar commented Nov 26, 2024

We would like to see you use case here considering this is very specific. Can you add a video or a repro to show how does this solve your issue?

@Rajdeepc Yes, I can add a video or we can do another debug session so I can walk threw it with you or you team?

@zolar
Copy link
Author

zolar commented Nov 27, 2024

@Rajdeepc This is a video of the component not rendering. The component container stays blank and you can see the console output going forever. It will eventually crash the browser.

Sequence.low.02.mp4

@Rajdeepc
Copy link
Contributor

@Rajdeepc This is a video of the component not rendering. The component container stays blank and you can see the console output going forever. It will eventually crash the browser.

Sequence.low.02.mp4

This seems to be a condition where the entire component is re-rendering multiple times due to state or property changes. Can you do a repro of your use case by any means?

Copy link

changeset-bot bot commented Nov 27, 2024

⚠️ No Changeset found

Latest commit: 5bb9e8b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@zolar
Copy link
Author

zolar commented Nov 27, 2024

@Rajdeepc This is a video of the component not rendering. The component container stays blank and you can see the console output going forever. It will eventually crash the browser.
Sequence.low.02.mp4

This seems to be a condition where the entire component is re-rendering multiple times due to state or property changes. Can you do a repro of your use case by any means?

@Rajdeepc
This issue is only repoing in Express app. We have an example / development application in which we do not see this issue. I think that the issue has to do with Express being so resource intensive. I think this is a race condition in which the Dialog is not completely finished rendering when the slothandler attaches to the slot content, which forces it to rerender. I think that using he nodes.find delays in just enough that the Dialog completes rendering. The issue can also be fixed using the current slot.assignedElements and a zero wait promise new Promise(resolve => setTimeout(resolve, 0));.

Express does use Service Workers but its still very resource heavy and I think its effecting the rendering of our component. We also use a lot of OverlayTriggers that might effect render changes also.

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.

2 participants