Try fixing full-screen modal height so contents can scroll #73150
Conversation
tellthemachines added [Type] Bug 
| Size Change: +111 B (0%) Total Size: 2.51 MB
ℹ️ View Unchanged
|
| Flaky tests detected in a314add. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19520013725
|
andrewserong left a comment
andrewserong left a comment There was a problem hiding this comment.
Conceptually this sounds like a good idea for a fix to me! Since it changes the modal styles itself, I feel slightly cautious about approving, so keen to hear what other folks think, too.
| &.is-full-screen { | ||
| // When full screen, set explicit heights so contents can be scrollable. | ||
| .components-modal__content { | ||
| height: calc(100% - 72px); // Subtract header height |
There was a problem hiding this comment.
Tiny nit, but should we use the $header-height in case values change again any time in the future?
| The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| &.is-full-screen { | ||
| // When full screen, set explicit heights so contents can be scrollable. | ||
| .components-modal__content { | ||
| height: calc(100% - #{$header-height + $grid-unit-10}); // Subtract header height | ||
| } | ||
| .components-modal__children-container { | ||
| height: 100%; | ||
| } | ||
| } |
There was a problem hiding this comment.
With this implementation, I'm seeing unnecessary overflow:
Also it doesn't account for __experimentalHideHeader.
I haven't tested thoroughly, but could we try something in the vein of 👇?
diff --git a/packages/components/src/modal/style.scss b/packages/components/src/modal/style.scss index f494fdcb27..193575a1bc 100644 --- a/packages/components/src/modal/style.scss +++ b/packages/components/src/modal/style.scss @@ -199,6 +199,7 @@ // Modal contents. .components-modal__content { +display: flex; flex: 1; margin-top: $header-height + $grid-unit-10; // Small top padding required to avoid cutting off the visible outline when the first child element is focusable.If that ends up being sufficient, we could also remove the components-modal__children-container className that was added.
There was a problem hiding this comment.
Thanks for testing! I've updated with some styles for the hidden header variation.
I can't repro that scrollbar in your screenshot, how did you achieve it?
Unfortunately making the content area a flexbox won't change anything, because what we need is an explicit height set on the content container. Otherwise it can't become scrollable when its contents are longer than the modal, and we're not able to make sticky elements work inside it.
There was a problem hiding this comment.
I can see the scrollbar using the Modal storybook story, when setting it to "Fill":
There was a problem hiding this comment.
Oh, I see what the problem is now. The storybook modal contains an unstyled p tag, and its margins are collapsing into its container. The container being set to 100% means that it now takes up the space of its parent plus that extra margin.
Adding 1px padding-top to components-modal__children-container fixes the issue, stopping the margins from collapsing. Alternatively we could move the 4px padding top from components-modal__content to components-modal__children-container.
There was a problem hiding this comment.
Ah good catch, and that would explain why Lena's display: flex worked (for the issue of the scrollbar) because that switched off the margin collapse. If flex is no good, would display: flow-root work for .components-modal__children-container to resolve the issue with collapsing margins, but without having to opt-in to flex or add padding?
There was a problem hiding this comment.
Oh good idea, flow-root on children seems to work! Let's try that instead.
There was a problem hiding this comment.
Unfortunately making the content area a flexbox won't change anything, because what we need is an explicit height set on the content container. Otherwise it can't become scrollable when its contents are longer than the modal, and we're not able to make sticky elements work inside it.
I'm still a bit confused because it seems like independent scrolling containers within flex children is a pretty common thing to do. Something like this:
CleanShot.2025-11-18.at.08-57-50.mp4
diff --git a/packages/components/src/modal/stories/index.story.tsx b/packages/components/src/modal/stories/index.story.tsx index 3a7b817458..0e891413ae 100644 --- a/packages/components/src/modal/stories/index.story.tsx +++ b/packages/components/src/modal/stories/index.story.tsx @@ -63,26 +63,29 @@ const Template: StoryFn< typeof Modal > = ( { onRequestClose, ...args } ) => { </Button> { isOpen && ( <Modal onRequestClose={ closeModal } { ...args }> -<p> -Lorem ipsum dolor sit amet, consectetur adipiscing elit, -sed do eiusmod tempor incididunt ut labore et magna -aliqua. Ut enim ad minim veniam, quis nostrud -exercitation ullamco laboris nisi ut aliquip ex ea ea -commodo consequat. Duis aute irure dolor in -reprehenderit in voluptate velit esse cillum dolore eu -fugiat nulla pariatur. Excepteur sint occaecat cupidatat -non proident, sunt in culpa qui officia deserunt mollit -anim id est laborum. -</p> - -<InputControl -__next40pxDefaultSize -style={ { marginBottom: '20px' } } -/> - -<Button variant="secondary" onClick={ closeModal }> -Close Modal -</Button> +<div +style={ { +position: 'relative', +overflow: 'auto', +height: '100%', +} } +> +<div +style={ { +height: '2000px', +background: +'linear-gradient(to bottom, yellow, orange)', +} } +></div> +<div +style={ { +height: '80px', +backgroundColor: 'magenta', +position: 'sticky', +bottom: 0, +} } +></div> +</div> </Modal> ) } </> diff --git a/packages/components/src/modal/style.scss b/packages/components/src/modal/style.scss index c6395f091a..0767de7b33 100644 --- a/packages/components/src/modal/style.scss +++ b/packages/components/src/modal/style.scss @@ -189,6 +189,7 @@ // Modal contents. .components-modal__content { +display: flex; flex: 1; margin-top: $header-height + $grid-unit-10; // Small top padding required to avoid cutting off the visible outline when the first child element is focusable. @@ -208,3 +209,7 @@ outline-offset: -2px; } } + +.components-modal__children-container { +flex: 1; +}Is there a specific layout that can't be achieved?
There was a problem hiding this comment.
Is there a specific layout that can't be achieved?
Yes! The screenshot in the PR description shows it. When the DataViewsPicker is used inside a Modal, we need its footer to stick to the bottom of the modal, while its content scrolls. To achieve a sticky footer, the height of its containers has to be explicitly set. If you check the storybook example you can see that the footer isn't currently sticky. This PR aims to fix that.
There was a problem hiding this comment.
That seems addressable with the snippet I posted above. Not sure if I'm missing something, but this diff is sufficient to make it stick for me:
diff --git a/packages/components/src/modal/style.scss b/packages/components/src/modal/style.scss index c6395f091a..0767de7b33 100644 --- a/packages/components/src/modal/style.scss +++ b/packages/components/src/modal/style.scss @@ -189,6 +189,7 @@ // Modal contents. .components-modal__content { +display: flex; flex: 1; margin-top: $header-height + $grid-unit-10; // Small top padding required to avoid cutting off the visible outline when the first child element is focusable. @@ -208,3 +209,7 @@ outline-offset: -2px; } } + +.components-modal__children-container { +flex: 1; +} diff --git a/packages/dataviews/src/stories/dataviews-picker.story.tsx b/packages/dataviews/src/stories/dataviews-picker.story.tsx index 9c0777863e..227417aef9 100644 --- a/packages/dataviews/src/stories/dataviews-picker.story.tsx +++ b/packages/dataviews/src/stories/dataviews-picker.story.tsx @@ -269,7 +269,13 @@ export const WithModal = ( { isFullScreen={ false } size="fill" > -<div style={ { padding: '16px' } }> +<div +style={ { +padding: '16px', +height: '100%', +overflow: 'auto', +} } +> <DataViewsPickerContent perPageSizes={ perPageSizes } isMultiselectable={ isMultiselectable }CleanShot.2025-11-18.at.18-20-54.mp4
(The footer has background color issues, but that's a different problem.)
There was a problem hiding this comment.
Oh I see, looks like it works OK once flex: 1 is set on the child. Let me adjust that. Thanks!
There was a problem hiding this comment.
The fix is working pretty well for me, thanks for handling this!
The only thing I noticed is that when adding a lot of content in a modal to make it overflow and scrolling to the end, there doesn't seem to be padding at the bottom:
Not sure if that's a result of this change.
tellthemachines requested review from gigitux, ntsekouras and oandregal as code owners 
Yes it seems be. It only happens if content is added directly to the modal body, which makes I guess a fix for that could be to move the padding-bottom to |
Actually that doesn't work. Weirdly, adding a margin-bottom to |
| margin-bottom: $grid-unit-40; | ||
| padding-bottom: 0; | ||
| } | ||
| .components-modal__children-container { |
There was a problem hiding this comment.
This class should probably be removed from Modal component, and we should see if it's possible to adjust the styles to work without relying on this class. I haven't tried it yet, but maybe this PR could serve as inspiration to achieve the wanted result.
There was a problem hiding this comment.
That's a good point, I think it was only added to workaround the issue that this PR solves.
There was a problem hiding this comment.
I disagree with the idea of avoiding classnames as public APIs, especially in components where the markup is unlikely to change. It tends to make our life more difficult down the line, for no actual benefit.
However I do want this bug fixed ASAP so am prepared to do whatever it takes to get this PR merged 😇
There was a problem hiding this comment.
Ok I've looked into this and #73108 which added the class wasn't trying to solve exactly the same problem as this PR, so there's no straightforward way of removing the class while preserving the design that PR implements.
The custom modal styles in the HTML block are being applied not only to the fullScreen modal but to the regular one as well, making it always stretch to its max height:
Whereas the styles we're adding to this PR are scoped to the full screen modal, because for most other types of modals we only want them to be as tall as their content.
I think for the case of the HTML block, we could argue that the modal could just be full screen by default. It's not like having it occupy 2/3 of the screen instead is going to make much difference; you still can't see the content beneath. But I don't think that's a task for this PR.
What I can do is target :last-child for the styles I'm applying here instead of using the unwanted components-modal__children-container, and that'll make things easier if we want to explore removing the classname altogether in a subsequent PR.
talldan left a comment
talldan left a comment There was a problem hiding this comment.
This is testing well for me now, nice work!
What?
Closes
Fixes #70611.
I haven't thoroughly tested every single full-screen modal but this doesn't seem to break the ones I looked at. Imo it's preferable for the solution to not require adding props to control modal height, or the components using the modal to add custom styles. If we can get away with fixing modal so its children can control their height, that's the most straightforward path.
Testing Instructions
With the sticky footer, the modal bottom padding makes it look a bit weird.