[2.x] fix: clamp the desktop container widths to the available space - #4905
Merged
Conversation
The desktop band widths introduced in #4869 are not tied to the breakpoints that open them: the hd band starts at 1100px but sets a 1200px width, so every viewport from 1100px to 1199px got a container up to 100px wider than the screen. That does not surface as a horizontal scrollbar. `body` hides the overflow and `margin-left/right: auto` cannot resolve to a negative value, so the container is pinned to the left and its right edge is silently clipped -- the layout stops being centred and part of the header runs off-screen. Each band is now clamped with `min(..., 100%)`. The hd band goes fluid between 1100px and 1199px and pins to 1200px from 1200px up, as intended. The xl/xxl bands are not affected today, but they are clamped too: the widths are custom properties so a theme can retune them from :root and reintroduce the same mismatch. `100%` rather than `100vw`, because `body` has `overflow-y: scroll` and 100vw would include the scrollbar gutter. It also behaves correctly when the discussion list pane is pinned, where the containing block is already narrowed by the pane width.
imorland
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The desktop band widths introduced in #4869 are not tied to the breakpoints that open them.
@screen-desktop-hdis1100px, but the hd band setswidth: var(--container-hd), which is1200px. So every viewport from 1100px to 1199px gets a container up to 100px wider than the screen.This does not surface as a horizontal scrollbar, which is why it slipped through.
bodyhasoverflow-x: hidden, andmargin-left/right: autocannot resolve to a negative value, so the container is pinned to the left and its right edge is silently clipped — the layout stops being centred and part of the header runs off-screen. Reproduce by sizing a window to ~1150px wide on any page where the discussion list pane is not pinned.(The pinned-pane path escapes it by accident:
DiscussionListPane.lessalready sets.container { max-width: 100% }in that case.)Changes proposed in this pull request:
Each desktop band is now clamped to the space actually available:
The hd band goes fluid between 1100px and 1199px and pins to 1200px from 1200px up, which is what #4869 intended. The xl and xxl bands are not broken today (1300px at a 1600px bound, 1600px at a 2000px bound), but they get the same guard because the widths are custom properties — a theme retuning
--container-xlfrom:rootcould reintroduce the identical mismatch, and core has no way to catch that.100%rather than100vw:bodyhasoverflow-y: scroll, so100vwwould include the scrollbar gutter and stay ~15px too wide.100%also does the right thing when the pane is pinned, where the containing block is already narrowed bymargin-left: var(--pane-width)— the clamp then means "the space left over", matching the existing override.Reviewer notes
The existing
max-width: 100%inDiscussionListPane.lessis now redundant but harmless, so I left it rather than widen the diff.