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(OverlayTrigger): conditionally attach slotchange listener #4690

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

Conversation

jcmitch
Copy link
Contributor

@jcmitch jcmitch commented Aug 24, 2024

Conditionally attaching slot change listener in OverlayTrigger to avoid issues where it is possible to get caught in an endless render loop.

Description

Conditionally attach slot change listeners only if the slot element is going to be populated.

Related issue(s)

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

Motivation and context

Resolves an issue related to endless render loop.

How has this been tested?

Tested on desktop in a standalone simple app using SWC OverlayTrigger.

  • 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.

@jcmitch jcmitch requested a review from a team as a code owner August 24, 2024 00:01
Copy link

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Copy link

github-actions bot commented Aug 24, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.99 0.98
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 228.342 kB 216.437 kB 214.625 kB 🏆
Scripts 57.74 kB 51.887 kB 49.963 kB 🏆
Stylesheet 34.477 kB 30.071 kB 🏆 30.191 kB
Document 6.227 kB 5.46 kB 5.442 kB 🏆
Font 126.941 kB 126.604 kB 🏆 126.678 kB

Request Count

Category Latest Main Branch
Total 52 52 52
Scripts 41 41 41
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

Copy link

github-actions bot commented Aug 24, 2024

Tachometer results

Chrome

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 500 kB 51.06ms - 52.42ms - faster ✔
3% - 6%
1.35ms - 3.06ms
branch 476 kB 53.43ms - 54.46ms slower ❌
3% - 6%
1.35ms - 3.06ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 701 kB 138.49ms - 141.49ms - faster ✔
4% - 7%
6.10ms - 10.46ms
branch 659 kB 146.69ms - 149.85ms slower ❌
4% - 8%
6.10ms - 10.46ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 658 kB 65.94ms - 66.95ms - faster ✔
7% - 9%
5.09ms - 6.91ms
branch 616 kB 71.69ms - 73.20ms slower ❌
8% - 10%
5.09ms - 6.91ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 657 kB 64.14ms - 65.29ms - faster ✔
7% - 10%
5.23ms - 7.15ms
branch 615 kB 70.14ms - 71.68ms slower ❌
8% - 11%
5.23ms - 7.15ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 847 kB 1872.97ms - 1875.58ms - unsure 🔍
-0% - +0%
-1.16ms - +2.70ms
branch 802 kB 1872.09ms - 1874.92ms unsure 🔍
-0% - +0%
-2.70ms - +1.16ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 845 kB 1873.54ms - 1876.64ms - unsure 🔍
-0% - -0%
-4.67ms - -0.68ms
branch 801 kB 1876.51ms - 1879.02ms unsure 🔍
+0% - +0%
+0.68ms - +4.67ms
-

breadcrumbs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 718 kB 494.44ms - 499.40ms - faster ✔
0% - 2%
1.15ms - 8.09ms
branch 676 kB 499.11ms - 503.97ms slower ❌
0% - 2%
1.15ms - 8.09ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 763 kB 37.30ms - 37.76ms - faster ✔
2% - 4%
0.94ms - 1.59ms
branch 718 kB 38.56ms - 39.02ms slower ❌
2% - 4%
0.94ms - 1.59ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 763 kB 390.20ms - 396.19ms - faster ✔
2% - 4%
7.34ms - 16.97ms
branch 718 kB 401.59ms - 409.11ms slower ❌
2% - 4%
7.34ms - 16.97ms
-

contextual-help permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 689 kB 50.58ms - 52.75ms - faster ✔
4% - 9%
1.98ms - 4.76ms
branch 644 kB 54.17ms - 55.90ms slower ❌
4% - 9%
1.98ms - 4.76ms
-

menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 494 kB 208.00ms - 211.47ms - faster ✔
1% - 3%
3.13ms - 7.19ms
branch 470 kB 213.84ms - 215.95ms slower ❌
1% - 3%
3.13ms - 7.19ms
-

overlay permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 716 kB 437.36ms - 440.95ms - unsure 🔍
-1% - +2%
-5.10ms - +10.11ms
branch 698 kB 429.26ms - 444.04ms unsure 🔍
-2% - +1%
-10.11ms - +5.10ms
-

directive-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 822 kB 22.39ms - 22.87ms - faster ✔
11% - 13%
2.75ms - 3.48ms
branch 778 kB 25.47ms - 26.02ms slower ❌
12% - 15%
2.75ms - 3.48ms
-

element-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 812 kB 352.41ms - 355.11ms - faster ✔
3% - 4%
9.87ms - 13.67ms
branch 767 kB 364.19ms - 366.87ms slower ❌
3% - 4%
9.87ms - 13.67ms
-

lazy-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 609 kB 41.81ms - 42.75ms - faster ✔
6% - 9%
2.58ms - 4.11ms
branch 564 kB 45.02ms - 46.23ms slower ❌
6% - 10%
2.58ms - 4.11ms
-

picker permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 568 kB 507.37ms - 514.37ms - faster ✔
2% - 4%
12.89ms - 23.56ms
branch 526 kB 525.07ms - 533.12ms slower ❌
3% - 5%
12.89ms - 23.56ms
-

popover permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 395 kB 11.37ms - 11.53ms - faster ✔
1% - 3%
0.14ms - 0.38ms
branch 373 kB 11.62ms - 11.80ms slower ❌
1% - 3%
0.14ms - 0.38ms
-

split-button permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 780 kB 1865.76ms - 1870.42ms - unsure 🔍
-0% - +0%
-3.38ms - +2.80ms
branch 736 kB 1866.35ms - 1870.41ms unsure 🔍
-0% - +0%
-2.80ms - +3.38ms
-

tooltip permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 618 kB 33.98ms - 34.61ms - faster ✔
5% - 7%
1.70ms - 2.64ms
branch 579 kB 36.12ms - 36.81ms slower ❌
5% - 8%
1.70ms - 2.64ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 579 kB 23.26ms - 23.90ms - faster ✔
7% - 10%
1.73ms - 2.60ms
branch 537 kB 25.45ms - 26.03ms slower ❌
7% - 11%
1.73ms - 2.60ms
-

test-element permalink

Version Bytes Avg Time vs remote vs branch
npm latest 704 kB 52.15ms - 53.13ms - faster ✔
5% - 8%
2.70ms - 4.38ms
branch 659 kB 55.49ms - 56.87ms slower ❌
5% - 8%
2.70ms - 4.38ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 680 kB 42.38ms - 43.47ms - faster ✔
6% - 10%
2.77ms - 4.48ms
branch 635 kB 45.89ms - 47.20ms slower ❌
6% - 10%
2.77ms - 4.48ms
-

truncated permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 545 kB 58.42ms - 62.78ms - unsure 🔍
-7% - +2%
-4.41ms - +1.20ms
branch 519 kB 60.44ms - 63.97ms unsure 🔍
-2% - +7%
-1.20ms - +4.41ms
-
Firefox

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 500 kB 111.61ms - 117.23ms - faster ✔
1% - 8%
1.26ms - 9.10ms
branch 476 kB 116.87ms - 122.33ms slower ❌
1% - 8%
1.26ms - 9.10ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 701 kB 276.95ms - 282.01ms - faster ✔
10% - 12%
32.20ms - 38.92ms
branch 659 kB 312.82ms - 317.26ms slower ❌
11% - 14%
32.20ms - 38.92ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 658 kB 136.33ms - 138.51ms - slower ❌
0% - 2%
0.15ms - 2.93ms
branch 616 kB 135.01ms - 136.75ms faster ✔
0% - 2%
0.15ms - 2.93ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 657 kB 127.81ms - 131.51ms - faster ✔
2% - 6%
2.74ms - 7.58ms
branch 615 kB 133.26ms - 136.38ms slower ❌
2% - 6%
2.74ms - 7.58ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 847 kB 1887.38ms - 1895.34ms - unsure 🔍
-0% - +1%
-1.19ms - +9.55ms
branch 802 kB 1883.58ms - 1890.78ms unsure 🔍
-1% - +0%
-9.55ms - +1.19ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 845 kB 1892.08ms - 1896.72ms - unsure 🔍
-0% - +0%
-6.75ms - +1.19ms
branch 801 kB 1893.96ms - 1900.40ms unsure 🔍
-0% - +0%
-1.19ms - +6.75ms
-

breadcrumbs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 718 kB 786.54ms - 795.58ms - faster ✔
2% - 4%
14.55ms - 35.81ms
branch 676 kB 806.62ms - 825.86ms slower ❌
2% - 5%
14.55ms - 35.81ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 763 kB 60.25ms - 63.27ms - unsure 🔍
-5% - +0%
-3.03ms - +0.23ms
branch 718 kB 62.54ms - 63.78ms unsure 🔍
-0% - +5%
-0.23ms - +3.03ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 763 kB 713.05ms - 721.71ms - unsure 🔍
-0% - +2%
-1.28ms - +15.68ms
branch 718 kB 702.88ms - 717.48ms unsure 🔍
-2% - +0%
-15.68ms - +1.28ms
-

contextual-help permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 689 kB 107.86ms - 112.98ms - faster ✔
2% - 9%
2.16ms - 10.28ms
branch 644 kB 113.49ms - 119.79ms slower ❌
2% - 9%
2.16ms - 10.28ms
-

menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 494 kB 430.26ms - 442.58ms - faster ✔
0% - 4%
1.23ms - 18.29ms
branch 470 kB 440.27ms - 452.09ms slower ❌
0% - 4%
1.23ms - 18.29ms
-

overlay permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 819 kB 634.68ms - 652.32ms - slower ❌
7% - 10%
38.78ms - 56.90ms
branch 774 kB 593.58ms - 597.74ms faster ✔
6% - 9%
38.78ms - 56.90ms
-

directive-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 822 kB 47.71ms - 48.37ms - faster ✔
4% - 7%
2.19ms - 3.41ms
branch 778 kB 50.33ms - 51.35ms slower ❌
5% - 7%
2.19ms - 3.41ms
-

element-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 812 kB 659.58ms - 665.86ms - slower ❌
3% - 5%
21.85ms - 29.55ms
branch 767 kB 634.79ms - 639.25ms faster ✔
3% - 4%
21.85ms - 29.55ms
-

lazy-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 609 kB 98.46ms - 107.26ms - slower ❌
4% - 14%
3.79ms - 12.77ms
branch 564 kB 93.70ms - 95.46ms faster ✔
4% - 12%
3.79ms - 12.77ms
-

picker permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 568 kB 994.23ms - 1016.53ms - faster ✔
4% - 6%
37.05ms - 63.99ms
branch 526 kB 1048.35ms - 1063.45ms slower ❌
4% - 6%
37.05ms - 63.99ms
-

popover permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 395 kB 29.10ms - 32.18ms - unsure 🔍
-7% - +7%
-2.24ms - +2.04ms
branch 373 kB 29.25ms - 32.23ms unsure 🔍
-7% - +7%
-2.04ms - +2.24ms
-

split-button permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 780 kB 1872.22ms - 1876.54ms - unsure 🔍
-0% - +0%
-5.05ms - +0.65ms
branch 736 kB 1874.71ms - 1878.45ms unsure 🔍
-0% - +0%
-0.65ms - +5.05ms
-

tooltip permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 704 kB 78.75ms - 82.13ms - slower ❌
8% - 13%
5.95ms - 9.69ms
branch 659 kB 71.80ms - 73.44ms faster ✔
8% - 12%
5.95ms - 9.69ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 579 kB 47.01ms - 48.35ms - faster ✔
5% - 12%
2.37ms - 6.23ms
branch 537 kB 50.17ms - 53.79ms slower ❌
5% - 13%
2.37ms - 6.23ms
-

test-element permalink

Version Bytes Avg Time vs remote vs branch
npm latest 704 kB 119.84ms - 127.12ms - unsure 🔍
-1% - +6%
-1.06ms - +6.94ms
branch 659 kB 118.87ms - 122.21ms unsure 🔍
-6% - +1%
-6.94ms - +1.06ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 680 kB 89.72ms - 95.08ms - faster ✔
5% - 11%
4.46ms - 11.18ms
branch 635 kB 98.20ms - 102.24ms slower ❌
5% - 12%
4.46ms - 11.18ms
-

truncated permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 545 kB 103.39ms - 109.61ms - unsure 🔍
-6% - +2%
-6.58ms - +2.18ms
branch 519 kB 105.62ms - 111.78ms unsure 🔍
-2% - +6%
-2.18ms - +6.58ms
-

@Rajdeepc
Copy link
Contributor

@jcmitch Thanks for doing this! Can you please help us with some corner case test cases to help in testing this issue which you faced in your environment. We want to capture all these corner cases and run these in isolated environments to reduce such issues going forward.

@coveralls
Copy link
Collaborator

coveralls commented Aug 29, 2024

Pull Request Test Coverage Report for Build 10931182364

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • 267 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.8%) to 97.452%

Files with Coverage Reduction New Missed Lines %
packages/overlay/src/OverlayPopover.ts 2 85.09%
packages/overlay/src/OverlayDialog.ts 5 87.0%
packages/overlay/src/Overlay.ts 8 96.68%
packages/dialog/src/DialogBase.ts 8 91.51%
packages/overlay/src/OverlayStack.ts 11 86.81%
packages/overlay/src/HoverController.ts 24 87.56%
packages/overlay/src/OverlayTrigger.ts 50 79.43%
packages/overlay/src/LongpressController.ts 159 22.64%
Totals Coverage Status
Change from base Build 10905285957: -0.8%
Covered Lines: 32290
Relevant Lines: 32976

💛 - Coveralls

@najikahalsema najikahalsema linked an issue Oct 8, 2024 that may be closed by this pull request
1 task
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]: OverlayTrigger can get stuck in a render loop causing page crash
4 participants