Global Styles: fix overflow caused by RangeControl tooltip #65875
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
ciampo
added
[Type] Bug
|
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. |
2 tasks
|
Size Change: +104 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
commented
Oct 4, 2024
Comment on lines
+60
to
+63
| export const Wrapper = styled( 'div', { | ||
| shouldForwardProp: ( prop: string ) => | ||
| ! [ 'color', '__nextHasNoMarginBottom', 'marks' ].includes( prop ), | ||
| } )< WrapperProps >` |
There was a problem hiding this comment.
This change is not related to the fix, but rather an improvement that I applied while debugging the DOM rendered by RangeControl. I noticed that the value of the color prop was being applied as an attribute, which is not expected.
t-hamano
reviewed
Oct 4, 2024
There was a problem hiding this comment.
Thanks for the PR!
In my testing, the tooltip disappearance animation doesn't seem to work in Firefox. How about in your environment?
Chrome
3ce9b3a911c2c7ccb74c678d76f41c3f.mp4
Firefox
f56e474f347060a71050a816fa6cff38.mp4
Maybe it's because Firefox only partially supports the @starting-style rule?
https://developer.mozilla.org/en-US/docs/Web/CSS/@starting-style#browser_compatibility
Werid — I'd expect the "entry" animation not to work and the exit to work. I get the same behaviour on MacOS firefox. Although I wouldn't consider it blocking, since the animation is pure cosmetic here, and it should work when Firefox will add support for these APIs. On the other hand, though, |
|
I'm testing updating |
ciampo
force-pushed
the
fix/global-styles-range-control-screen-overflow
branch
2 times, most recently
from
d866d7f to
d02c68b
Compare
|
Flaky tests detected in d02c68b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11181695095
|
Confirmed I see that in Firefox, although I think this isn't related to this PR, is it? |
I think it's related to this PR. It might be that diff --git a/packages/components/src/range-control/styles/range-control-styles.ts b/packages/components/src/range-control/styles/range-control-styles.ts
index 6e9c68ace9..10175f57c6 100644
--- a/packages/components/src/range-control/styles/range-control-styles.ts
+++ b/packages/components/src/range-control/styles/range-control-styles.ts
@@ -265,13 +265,11 @@ export const InputRange = styled.input`
const tooltipShow = ( { show }: TooltipProps ) => {
return css`
- display: ${ show ? 'inline-block' : 'none' };
+ display: inline-block;
opacity: ${ show ? 1 : 0 };
@media not ( prefers-reduced-motion ) {
- transition:
- opacity 120ms ease,
- display 120ms ease allow-discrete;
+ transition: opacity 120ms ease;
}
@starting-style {However, I think it's okay to proceed with this PR in the hope that Firefox will fully support it in the future. |
ciampo
force-pushed
the
fix/global-styles-range-control-screen-overflow
branch
from
d02c68b to
91bff18
Compare
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…#65875) * RangeControl: do not render tooltip in the DOM when hidden * RangeControl: do not apply react props as attributes on wrapper element * Global Styles: add bottom padding to allow for tooltip in font size screen * CHANGELOG --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
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.
What?
Fixes #65825
This PR fixes an involuntary overflow in the font size screen, in the Global Styles sidebar
Why?
Fixing a visual glitch
How?
As @ciampo and @t-hamano debugged in #65825, the cause for the overflow is the
RangeControl's tooltip.The tooltip is a custom implementation (context for this decision can be found in #19916 and #23006).
This PR applies two separate fixes:
display: nonewhen hidden. This seems safe because the tooltip already has aaria-hiddenattribute set, and because it only replicates the same information that assistive technology can parse from the nativeinputelement. We can still animate the tooltip thanks to the recent@starting-styleandallow-discreteCSS apis, which should work in most browsers (and can be considered as progressive enhancement);Note
RangeControlcan clearly benefit from some love. We should see if we can use theTooltipcomponent, instead of a custom implementation. And potentially remove a lot of code, given the advancements in CSS styling of native inputs. Obviously not in scope for this PR. And potentially, we could just look at overhauling the whole set of slider-related components (see #40507)Testing Instructions
Follow instructions in #65825 — the overflow bug should be gone.
Screenshots or screencast
Kapture.2024-10-04.at.12.24.25.mp4