Skip to content

Commit bceaa16

Browse files
gDINESH13gDINESH13
authored
fix(modal): height flickering issue (#22190)
* fix(modal): height flickering issue Signed-off-by: gDINESH13 <dinesh13g@gmail.com> * fix(modal): improve code coverage Signed-off-by: gDINESH13 <dinesh13g@gmail.com> --------- Signed-off-by: gDINESH13 <dinesh13g@gmail.com>
1 parent b9f08ad commit bceaa16

2 files changed

Lines changed: 217 additions & 7 deletions

File tree

‎packages/react/src/components/Modal/Modal-test.js‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,194 @@ describe.each([
707707
});
708708
});
709709
});
710+
711+
describe('Modal scroll content hysteresis', () => {
712+
it('should handle missing contentRef gracefully', () => {
713+
const { container } = render(
714+
<Component
715+
open
716+
modalHeading="Test Modal"
717+
primaryButtonText="Submit"
718+
secondaryButtonText="Cancel">
719+
<p>Test content</p>
720+
</Component>
721+
);
722+
723+
const modalContent = container.querySelector(`.${prefix}--modal-content`);
724+
725+
// Temporarily remove the ref to trigger the guard clause
726+
Object.defineProperty(modalContent, 'scrollHeight', {
727+
get: () => undefined,
728+
configurable: true,
729+
});
730+
731+
// Modal should still render without errors
732+
expect(modalContent).toBeInTheDocument();
733+
});
734+
735+
it('should set isScrollable to true when content is clearly scrollable (diff > 5)', () => {
736+
const { container, rerender } = render(
737+
<Component
738+
open
739+
modalHeading="Test Modal"
740+
primaryButtonText="Submit"
741+
secondaryButtonText="Cancel">
742+
<div style={{ height: '50px' }}>Small content</div>
743+
</Component>
744+
);
745+
746+
const modalContent = container.querySelector(`.${prefix}--modal-content`);
747+
748+
// Mock scrollHeight > clientHeight by more than 5px
749+
Object.defineProperty(modalContent, 'scrollHeight', {
750+
get: () => 500,
751+
configurable: true,
752+
});
753+
Object.defineProperty(modalContent, 'clientHeight', {
754+
get: () => 400, // diff = 100, which is > 5
755+
configurable: true,
756+
});
757+
758+
// Trigger re-render to execute useEffect
759+
rerender(
760+
<Component
761+
open
762+
modalHeading="Test Modal"
763+
primaryButtonText="Submit"
764+
secondaryButtonText="Cancel">
765+
<div style={{ height: '500px' }}>
766+
Large content that needs scrolling
767+
</div>
768+
</Component>
769+
);
770+
771+
// Should have scroll-content class
772+
expect(modalContent).toHaveClass(`${prefix}--modal-scroll-content`);
773+
});
774+
775+
it('should set isScrollable to false when content is clearly not scrollable (diff < -5)', () => {
776+
const { container, rerender } = render(
777+
<Component
778+
open
779+
modalHeading="Test Modal"
780+
primaryButtonText="Submit"
781+
secondaryButtonText="Cancel">
782+
<div style={{ height: '500px' }}>Large content</div>
783+
</Component>
784+
);
785+
786+
const modalContent = container.querySelector(`.${prefix}--modal-content`);
787+
788+
// First make it scrollable
789+
Object.defineProperty(modalContent, 'scrollHeight', {
790+
get: () => 500,
791+
configurable: true,
792+
});
793+
Object.defineProperty(modalContent, 'clientHeight', {
794+
get: () => 400,
795+
configurable: true,
796+
});
797+
798+
rerender(
799+
<Component
800+
open
801+
modalHeading="Test Modal"
802+
primaryButtonText="Submit"
803+
secondaryButtonText="Cancel">
804+
<div style={{ height: '500px' }}>Large content</div>
805+
</Component>
806+
);
807+
808+
// Now make it clearly not scrollable (diff < -5)
809+
Object.defineProperty(modalContent, 'scrollHeight', {
810+
get: () => 300,
811+
configurable: true,
812+
});
813+
Object.defineProperty(modalContent, 'clientHeight', {
814+
get: () => 400, // diff = -100, which is < -5
815+
configurable: true,
816+
});
817+
818+
// Trigger re-render
819+
rerender(
820+
<Component
821+
open
822+
modalHeading="Test Modal"
823+
primaryButtonText="Submit"
824+
secondaryButtonText="Cancel">
825+
<div style={{ height: '50px' }}>Small content</div>
826+
</Component>
827+
);
828+
829+
// Should NOT have scroll-content class
830+
expect(modalContent).not.toHaveClass(`${prefix}--modal-scroll-content`);
831+
});
832+
833+
it('should maintain current state when diff is in dead zone (-5 to 5)', () => {
834+
const { container, rerender } = render(
835+
<Component
836+
open
837+
modalHeading="Test Modal"
838+
primaryButtonText="Submit"
839+
secondaryButtonText="Cancel">
840+
<div style={{ height: '100px' }}>Content</div>
841+
</Component>
842+
);
843+
844+
const modalContent = container.querySelector(`.${prefix}--modal-content`);
845+
846+
// Start with scrollable state
847+
Object.defineProperty(modalContent, 'scrollHeight', {
848+
get: () => 400,
849+
configurable: true,
850+
});
851+
Object.defineProperty(modalContent, 'clientHeight', {
852+
get: () => 300, // diff = 100 > 5, so scrollable
853+
configurable: true,
854+
});
855+
856+
rerender(
857+
<Component
858+
open
859+
modalHeading="Test Modal"
860+
primaryButtonText="Submit"
861+
secondaryButtonText="Cancel">
862+
<div style={{ height: '400px' }}>Large content</div>
863+
</Component>
864+
);
865+
866+
const initialHasClass = modalContent.classList.contains(
867+
`${prefix}--modal-scroll-content`
868+
);
869+
870+
// Now change to dead zone (diff = 2, which is between -5 and 5)
871+
Object.defineProperty(modalContent, 'scrollHeight', {
872+
get: () => 302,
873+
configurable: true,
874+
});
875+
Object.defineProperty(modalContent, 'clientHeight', {
876+
get: () => 300, // diff = 2, in dead zone
877+
configurable: true,
878+
});
879+
880+
rerender(
881+
<Component
882+
open
883+
modalHeading="Test Modal"
884+
primaryButtonText="Submit"
885+
secondaryButtonText="Cancel">
886+
<div style={{ height: '302px' }}>Content in dead zone</div>
887+
</Component>
888+
);
889+
890+
const finalHasClass = modalContent.classList.contains(
891+
`${prefix}--modal-scroll-content`
892+
);
893+
894+
// Class should remain the same (hysteresis prevents change)
895+
expect(initialHasClass).toBe(finalHasClass);
896+
});
897+
});
710898
});
711899

712900
describe('state', () => {

‎packages/react/src/components/Modal/Modal.tsx‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import React, {
1212
useContext,
1313
useEffect,
1414
useRef,
15+
useState,
1516
type HTMLAttributes,
1617
type ReactNode,
1718
type RefObject,
@@ -518,13 +519,34 @@ const ModalDialog = React.forwardRef(function ModalDialog(
518519
[`${prefix}--modal-container--full-width`]: isFullWidth,
519520
});
520521

521-
/**
522-
* isScrollable is implicitly dependent on height, when height gets updated
523-
* via `useResizeObserver`, clientHeight and scrollHeight get updated too
524-
*/
525-
const isScrollable =
526-
!!contentRef.current &&
527-
contentRef?.current?.scrollHeight > contentRef?.current?.clientHeight;
522+
const currentScrollHeight = contentRef.current?.scrollHeight || 0;
523+
const currentClientHeight = contentRef.current?.clientHeight || 0;
524+
525+
// The CSS border-block-end: 2px can cause clientHeight to change when the class is applied
526+
const [isScrollable, setIsScrollable] = useState(
527+
currentScrollHeight > currentClientHeight
528+
);
529+
530+
useEffect(() => {
531+
if (!contentRef.current) {
532+
setIsScrollable(false);
533+
return;
534+
}
535+
536+
const scrollHeight = contentRef.current.scrollHeight;
537+
const clientHeight = contentRef.current.clientHeight;
538+
const diff = scrollHeight - clientHeight;
539+
540+
// Different thresholds for turning on vs off
541+
if (diff > 5) {
542+
// Clearly scrollable - turn on
543+
setIsScrollable(true);
544+
} else if (diff < -5) {
545+
// Clearly not scrollable - turn off
546+
setIsScrollable(false);
547+
}
548+
// If -5 <= diff <= 5: Keep current state (dead zone prevents oscillation)
549+
}, [currentScrollHeight, currentClientHeight]);
528550

529551
const contentClasses = classNames(`${prefix}--modal-content`, {
530552
[`${prefix}--modal-scroll-content`]: hasScrollingContent || isScrollable,

0 commit comments

Comments
 (0)