Tabs: Prevent accidental overflow in indicator

[t-hamano]

What?

This PR takes the decimal point into account when calculating the indicator's style (top, left, width, height) to prevent the indicator from overflowing.

Why?

My understanding is that offset{top|left|width|height} rounds the decimal point. This error may cause the indicator to overflow.

One problem I've seen is that inspector controls overflow on Windows OS.

The reasons why this occurs are as follows.

• The sidebar including inspector controls is 280px wide.
• If the inspector control's content height is higher than the box height, a vertical scrollbar will appear.
• In my Chrome, the scrollbar width is 17px. As a result, the width of the inspector control is 263px.
  
• Tab button width is 131.5px.
  
• The current logic rounds the decimal point in the indicator's style calculation, so when the button on the right is selected, the CSS variable has a value like this:
  
• The left and top values should be 131.5px, but they are actually 132px, so the indicator overflows by 1px.

How?

This could easily be solved by applying overflow:hidden to the TabListWrapper, but I chose to use getBoundingClientRect() to generate an accurate indicator style that takes decimal points into account.

In response to the feedback, a different approach was adopted - see the comments starting here for more details.

Testing Instructions

On MacOS, scrollbars have no physical width, so you won't be able to reproduce this problem by default. However, you should be able to reproduce the horizontal scroll bar issue on MacOS by following the steps below.

• Set the value here to 279
• Change OS settings to always display scroll bars
• Select any block and open the block sidebar
• Select the "Style" tab
  • trunk: CSS variable has the following value and causes an overflow.
    
  • This PR: CSS variables contain decimal values and overflow should not occur
    

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: DaniGuardiola <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Flaky tests detected in 7d82754.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9234935079
📝 Reported issues:

• [Flaky Test] We show no results message if there are no comments #42384 in /test/e2e/specs/editor/blocks/comments.spec.js

[DaniGuardiola]

Good catch, this won't work though. The use of offset sizes is intentional since the bounding rect approach causes glitches when combined with transformations and such. Offset sizing properties are not affected by this, and though subpixel precision would be nice it doesn't really seem that important. As a fix for this, I would simply propose doing -1 to the height and width so there's never an accidental overflow.

Thanks for noticing and sending a fix! Want to try my suggestion here?

[t-hamano]

Want to try my suggestion here?

Of course, please feel free to push 👍

[t-hamano]

@DaniGuardiola Is it possible to move the PR forward? I believe this issue needs to be fixed in WP6.6.

[DaniGuardiola]

@t-hamano ah, yes, missed your last comment. Happy to push my proposed fix in a bit.

[DaniGuardiola]

@t-hamano I've pushed the "naive" solution I proposed, but I am exploring a better solution where we could still have subpixel precision: https://stackblitz.com/edit/solidjs-templates-oerxnc?file=index.html,src%2FApp.tsx,src%2Findex.tsx

[DaniGuardiola]

@t-hamano okay, this is the best I could do. I've tested it and it fixes the issue. I wish we could find a "perfect" solution with completely accurate values, but this'll have to do.

FYI, if you want to give it a last try, the goal is to get a computed "top" value that is accurate in the stackblitz I created (linked in the previous comment) - has to remain accurate also when transformed.

[tyxla]

Code-wise this makes sense to me 👍 Thanks!

@t-hamano if you could confirm this resolves the horizontal scroll issue in your testing, feel free to 🚢 .

[t-hamano]

@DaniGuardiola Thanks for the update!

In my environment, the width of the sidebar with the scrollbar is 263px, and I think it works as expected. The values ​​of each CSS variable changed by this PR are correct, right?

Before

After

[DaniGuardiola]

@t-hamano seems correct, yes.

[ellatrix]

This was cherry-picked to the wp/6.6 branch.