Add top:0 to hiddenContentStyles #33289
Open
+171
−62
Conversation
This file contains 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
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
… into hidden-styles-fix
smikula
requested review from
behowell,
a team,
spmonahan,
ThomasMichon,
khmakoto,
phkuo and
dzearing
as code owners
|
What is the risk of regression making this fairly fundamental change? If folks have overridden this behavior, will they run into issues at runtime. CSS changes are some of the worst as they result in customer incidents not build failures. If this was a regression or an issue introduced recently perhaps the risk is lower. |
@JustSlone — valid questions, and I don't have a great answer. I feel pretty good about the change given that the intent of the style is that the element should be hidden, and if the element is effectively invisible, it shouldn't matter where it gets positioned. But I can't account for assumptions or overrides that a consumer might make. What's the philosophy here? I could hack around it in my own product to solve my own problem, but my preference is to fix the root cause and (hopefully) benefit everybody. Do we avoid making the right long-term fix because of the short term pain it will cause? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previous Behavior
hiddenContentStylessetsposition: absolutebut does not specifytopor any other positioning property. In this case, the element gets positioned wherever it would fall in the static flow. This can, in some cases cause the scroll height of the container to get inflated. E.g. consider the following pseudocode:You would expect
containernot to scroll, because its height (100px) matches its content (max-height: 100px). Buthidden-divgets defaulted to a position oftop: 200px, meaningcontainerhas 200px of scrollable range (and the bottom 100px of it are empty space!)New Behavior
By specifying
top: 0the hidden div is guaranteed to be positioned at the top of the container and won't affect the scroll height.