fix(modal): height flickering issue#22190
Conversation
Signed-off-by: gDINESH13 <dinesh13g@gmail.com>
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22190 +/- ##
==========================================
- Coverage 95.51% 95.49% -0.03%
==========================================
Files 582 582
Lines 50320 50332 +12
Branches 6769 6717 -52
==========================================
Hits 48063 48063
- Misses 2125 2137 +12
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @gDINESH13, could you add more tests to cover this changes? 
|
Signed-off-by: gDINESH13 <dinesh13g@gmail.com>
|
Hello, @heloiselui Thanks for checking. I'd added some TC to improve coverage but those can't be considered to claim the functionality works. Mentioned this in PR desc, please lmk what you think. |
| }); | ||
|
|
||
| describe('Modal scroll content hysteresis', () => { | ||
| it('should handle missing contentRef gracefully', () => { |
There was a problem hiding this comment.
Test name says ‘missing contentRef’, but contentRef.current is not actually null here. This case stubs scrollHeight on an existing node instead. Could we either rename the test to match behavior or add a case that truly exercises !contentRef.current? -not a blocker
| if (diff > 5) { | ||
| // Clearly scrollable - turn on | ||
| setIsScrollable(true); | ||
| } else if (diff < -5) { |
There was a problem hiding this comment.
In normal layout wouldn't scrollHeight generally bottom out at clientHeight for non-overflowing content?
I ask cause thinking about the case of if modal content shrinks, diff usually becomes 0, stays in the dead zone, and keeps cds--modal-scroll-content forever.
That would leave the fade/mask, extra last-child margin, tabIndex, and region semantics on content that is no longer scrollable.
There was a problem hiding this comment.
Hi @tay1orjones I understand your concern, I emulated a test case on what you mentioned to have
scrollHeight === clientHeight and it retained the cds--modal-scroll-content at that point.
The class cds--modal-scroll-content adds this 2px border block every time.
So every time this class is applied on modal the clientHeight gets reduced by 2px and when class is removed the clientHeight is increased by 2px.
And that's one base reason why modal flickering happens, this change in clientHeight.
I can reduce the -5 diff to -2px but if I add diff<=0 the flicker happens again, clearly. Can you give some ideas on how to tackle this?
Also, this retaining will happen for of a range of 2px difference right, is that a serious issue?
| // Now make it clearly not scrollable (diff < -5) | ||
| Object.defineProperty(modalContent, 'scrollHeight', { | ||
| get: () => 300, | ||
| configurable: true, | ||
| }); | ||
| Object.defineProperty(modalContent, 'clientHeight', { | ||
| get: () => 400, // diff = -100, which is < -5 | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
This technically covers it but a better test would start scrollable, shrink content until scrollHeight === clientHeight, and assert the scroll class is removed.
This might be something to test in a real browser in e2e/components/Modal/Modal-test.avt.e2e.js. The .avt.e2e.js are generally a11y-focused files but can be used for one-off stuff like this.
bceaa16
Closes #22153
The modal height was flickering when user interactions caused dynamic content changes. This occurred because the isScrollable calculation created a feedback loop: when scrollHeight was close to clientHeight, adding the cds--modal-scroll-content class would apply a 2px border, changing the clientHeight, which would remove the class, which would remove the border, causing rapid oscillation.
Changelog
New
Changed
Removed
Testing / Reviewing
I have tested with writing a playwright e2e test. I haven't committed it because it requires a new story to be added in storybook.
But from storybook point of view it may look like duplication as many variants of modal is already present.
Writing a unit test case for this fix is not capturing the bug. I would like to get some suggestions on how should I show my testing.
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
More details can be found in the pull request guide