ResizableEditor: Add 2px to height to fix vertical scrollbar #66301
+3
−1
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
andrewserong
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. |
This was referenced Oct 22, 2024
|
Size Change: +2 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 067e1c2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11454021510
|
t-hamano
reviewed
Oct 22, 2024
There was a problem hiding this comment.
Thanks for the PR!
I think this is the least impactful approach in WP 6.7.
One thing I noticed is that 1px might not be enough to prevent overflow.
See the video below. Depending on the font size in the content, overflow may occur. The result seems to be also affected by the display size:
7f95463371c19a8f64c2ad1b473cd300.mp4
Maybe we need to change it to 2px?
|
I'm unable to review this PR in the next few days unfortunately. If someone from @WordPress/gutenberg-design can chime in, would appreciate it. I'm happy to defer to Aki above, for example. |
|
This seems a reasonable approach—especially for 6.7. I did open #66342 as an alternative that I think should also be okay for 6.7 and avoids a magic number (needs good testing of course). |
ramonjd
approved these changes
Oct 22, 2024
There was a problem hiding this comment.
| Then | Now |
|---|---|
 |
 |
I think this is the least impactful approach in WP 6.7.
One thing I noticed is that 1px might not be enough to prevent overflow.
I'm not sure but, in my testing, the iframe border affects the width a little.
So if I change the following line —
— to outline: 0.01px solid transparent; the effect is not so pronounced.
With border:
Kapture.2024-10-23.at.09.07.34.mp4
With outline:
Kapture.2024-10-23.at.09.08.19.mp4
With neither:
Kapture.2024-10-23.at.09.09.29.mp4
|
Thanks for the feedback and discussion, folks!
Just linking the discussion from #66041 (comment): it looks like we can't use I think it's a totally valid approach to revisit #66041 and find another way to go about fixing that. For now, though, I've updated this PR to use
Thanks for opening an alternate PR! That approach looks more correct and less hacky to me. I'm not too sure the implications of switching out to use the non-legacy API there, so just in case this PR feels safer to land, I'll leave this one open for now 🙂 |
|
Size Change: +2 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
andrewserong
changed the title
|
Thanks for the update. We’ll see how this shakes out, I think Ramon’s |
|
Thanks for the reviews here, folks! I'm going to close this out as I think the right fix will be #66390 — I found one bug with that implementation but from the sounds of it I think it should be a fairly straightforward fix. I'll close this out for now, but happy to re-open if we wind up needing to. |
getdave
removed
the
Backport to WP 6.8 Beta/RC
|
Looks like this wasn't merged to I removed the backport label. |
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 #66297, follows #66041
Fix the issue of a vertical scrollbar unintentionally being displayed in the resizable editor due to the height being set to just not tall enough when the iframe's size is not a rounded integer.
Note: this is a fairly naive fix, intended for 6.7. There might be a better way to go about this, so happy to close it out if folks have better ideas for a fix!
Why?
As of #66041 there is a sub-pixel border on the editor iframe to fix an issue with layout shifts during zooming. That fix caused the issue in #66297 as it turns out the resize observer that sets the height of the resizable editor rounds the height to an integer. This PR proposes adding
2pxof height clearance to the resizable editor to account for the sub pixel height difference.How?
1pxto the height of the resizable editor, to avoid a vertical scrollbarTesting Instructions
trunknote that there is a vertical scrollbar even if the pattern is short1pxdoesn't introduce any other visual issuesScreenshots or screencast