Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tabs: Fix indicator misalignment when the browser width changes in RTL languages #64965

Closed
wants to merge 2 commits into from

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Sep 1, 2024

Part of #64963

What?

This PR fixes the tab indicator misalignment when the browser width changes in RTL languages.

Why?

The horizontal position of the indicator is determined by the CSS left. However, in RTL languages, the tab buttons are located on the right side.

When we change the browser width, the left offset position of the button relative to the parent element will change, but the indicator position is fixed by the CSS left, so there is a misalignment between the button and the indicator.

How?

I think in RTL languages ​​we should determine the offset position via right instead of left.

First, to calculate the right offset position, I used the following logic:

  • Get the parent element's width, including decimal points.
  • Subtract the button element's width from its parent element. Round this value and convert it to an integer pixel value.
  • Subtract the element's offsetLeft from this value. This is the basic right value for the indicator.
  • Similar to the approach taken in #61979, subtract 1 to prevent unintended overflow. If the right value is negative, convert it to 0.

Next, apply the calculated value as CSS.

We can also use the CSS right instead of left in RTL languages. However, in this PR, I replaced the CSS left with inset-inline-start. Unlike the CSS left, this CSS applies a physical offset according to the document direction. This CSS is also supported by all browsers.

Testing Instructions

Storybook

Before

storybook-before.mp4

After

storybook-after.mp4

Site Editor

The easiest way to confirm is to look at the Font Library modal.

  • Access the Site Editor > Global Styles > Typography > Manage Fonts button
  • Make sure the indicator stays aligned with the tab buttons when the browser is resized

Before

font-library-modal-before.mp4

After

font-library-modal-after.mp4

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components l10n Localization and translations best practices labels Sep 1, 2024
@t-hamano t-hamano self-assigned this Sep 1, 2024
@@ -24,7 +24,7 @@ export const TabListWrapper = styled.div`

@media not ( prefers-reduced-motion: reduce ) {
&.is-animation-enabled::after {
transition-property: left, top, width, height;
transition-property: inset-inline-start, top, width, height;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is necessary so that the indicator animations continue to work as before.

Comment on lines +128 to +132
/**
* The distance from the right edge of the offset parent to the right edge of
* the element.
*/
right: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR, the right value is calculated in the getElementOffsetRect() function, but if the right value is inappropriate for the getElementOffsetRect() function itself, we can also calculate the right value directly in the Tabs component.

@t-hamano t-hamano requested a review from a team September 1, 2024 09:53
@t-hamano t-hamano marked this pull request as ready for review September 1, 2024 10:12
@t-hamano t-hamano requested a review from ajitbohra as a code owner September 1, 2024 10:12
Copy link

github-actions bot commented Sep 1, 2024

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 <wildworks@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

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

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

Thank you, @t-hamano !

@DaniGuardiola is already working on an iteration for this animation in #64926, so I'm not sure if it makes sense to keep this PR open.

At the same time, some of the fixes that you're proposing may be useful for Dani, since his new implementation still doesn't support RTL writing direction.

@DaniGuardiola
Copy link
Contributor

Thank you @t-hamano! Closing as per #64926 (comment), but bookmarking as the work will be helpful to me in the near future :)

@t-hamano t-hamano deleted the tabs/rtl-position-and-animation branch October 25, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n Localization and translations best practices [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants