Grid: In RTL languages, the resize handles point in the opposite direction #64995

rohitmathur-7 · 2024-09-03T05:41:09Z

What?

Fix the resize handle direction for RTL languages.
Fixes: Grid: In RTL languages, the resize handles point in the opposite direction #64931

Why?

To fix horizontal resizing for RTL languages.

How?

Fix the direction for RTL languages.

Testing Instructions

Insert a Grid block.
Switch to a RTL language.
Resize the inner block horizontally.

github-actions · 2024-09-03T05:41:24Z

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: rohitmathur-7 <rohitmathur7@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>

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

t-hamano

Thanks for the review!

This PR works fine horizontally, but seems to cause unintended issues vertically. The resizable box stretches from the center, not the top or bottom:

28794d3c28e6aeea417de5b4f2605c3d.mp4

I think the reason is that none of the conditions here are met when the direction is top or bottom. I think it will work if we make the following changes:

diff --git a/packages/block-editor/src/components/grid/grid-item-resizer.js b/packages/block-editor/src/components/grid/grid-item-resizer.js
index 0b63580149..7bd5fae530 100644
--- a/packages/block-editor/src/components/grid/grid-item-resizer.js
+++ b/packages/block-editor/src/components/grid/grid-item-resizer.js
@@ -137,6 +137,8 @@ function GridItemResizerInner( {
                                                        setResizeDirection( 'right' );
                                                } else if ( 'right' === direction ) {
                                                        setResizeDirection( 'left' );
+                                               } else {
+                                                       setResizeDirection( direction );
                                                }
                                        } else {
                                                /*

rohitmathur-7 · 2024-09-03T15:02:35Z

Thanks for the review!

This PR works fine horizontally, but seems to cause unintended issues vertically. The resizable box stretches from the center, not the top or bottom:

28794d3c28e6aeea417de5b4f2605c3d.mp4
I think the reason is that none of the conditions here are met when the direction is top or bottom. I think it will work if we make the following changes:

diff --git a/packages/block-editor/src/components/grid/grid-item-resizer.js b/packages/block-editor/src/components/grid/grid-item-resizer.js
index 0b63580149..7bd5fae530 100644
--- a/packages/block-editor/src/components/grid/grid-item-resizer.js
+++ b/packages/block-editor/src/components/grid/grid-item-resizer.js
@@ -137,6 +137,8 @@ function GridItemResizerInner( {
                                                        setResizeDirection( 'right' );
                                                } else if ( 'right' === direction ) {
                                                        setResizeDirection( 'left' );
+                                               } else {
+                                                       setResizeDirection( direction );
                                                }
                                        } else {
                                                /*

Hii @t-hamano ,
Yeah sorry I missed adding the else condition. 🙇🏻‍♂️
Have added the else condition.
Can you please check now.
Thanks.

t-hamano

LGTM! Just to be sure, I would appreciate additional feedback from @noisysocks and @tellthemachines.

andrewserong · 2024-09-10T06:19:52Z

Just to be sure, I would appreciate additional feedback

I haven't tested the change, but conceptually this PR looks like it's in the right direction to me! The resizeDirection then sets the justifyContent here, so handling rtl in this way seems correct to me:

gutenberg/packages/block-editor/src/components/grid/grid-item-resizer.js

Lines 89 to 92 in ad917d5

    
           ...( justification[ resizeDirection ] && { 
        
           	justifyContent: justification[ resizeDirection ], 
        
           } ), 
        
           ...( alignment[ resizeDirection ] && {

t-hamano · 2024-09-10T07:13:27Z

@andrewserong Thanks for the feedback!

The resizeDirection then sets the justifyContent here, so handling rtl in this way seems correct to me

I have considered this approach as well. In the trunk branch, we can make the following changes to solve the problem as well:

diff --git a/packages/block-editor/src/components/grid/grid-item-resizer.js b/packages/block-editor/src/components/grid/grid-item-resizer.js
index da3eb824fe..ffa93d8c01 100644
--- a/packages/block-editor/src/components/grid/grid-item-resizer.js
+++ b/packages/block-editor/src/components/grid/grid-item-resizer.js
@@ -3,6 +3,7 @@
  */
 import { ResizableBox } from '@wordpress/components';
 import { useState, useEffect } from '@wordpress/element';
+import { isRTL } from '@wordpress/i18n';
 
 /**
  * Internal dependencies
@@ -73,8 +74,8 @@ function GridItemResizerInner( {
        }, [ blockElement, rootBlockElement ] );
 
        const justification = {
-               right: 'flex-start',
-               left: 'flex-end',
+               right: isRTL() ? 'flex-end' : 'flex-start',
+               left: isRTL() ? 'flex-start' : 'flex-end',
        };
 
        const alignment = {

However, flex-{start|end} in a flex layout should automatically take into account the logical direction of the document. So, I feel we need to keep this as flex-start, regardless of whether it's an RTL language or not.

I suspect the root cause is that the drag handle of the ResizableBox component is based on the physical direction rather than the logical direction (the source that fixes the drag handle to the physical direction).

Therefore, I feel that the direction detected by the ResistableBox component needs to be inverted to match the logical direction of the document.

andrewserong · 2024-09-11T05:09:21Z

I suspect the root cause is that the drag handle of the ResizableBox component is based on the physical direction rather than the logical direction (the source that fixes the drag handle to the physical direction).
Therefore, I feel that the direction detected by the ResistableBox component needs to be inverted to match the logical direction of the document.

Good point, that makes sense to me now. Since the onResizeStart gets a direction with left and right values, do you think this PR would be an okay fix for now, or would you prefer to handle it in ResizableBox?

t-hamano · 2024-09-11T08:09:56Z

Since the ResizableBox component itself is designed to change the visual size of an element, I don't think it's a good idea to change the behavior of the onResizeStart prop itself.

I considered a different approach, but found that the problem could be solved by making the following changes in trunk:

diff --git a/packages/block-editor/src/components/grid/grid-item-resizer.js b/packages/block-editor/src/components/grid/grid-item-resizer.js
index da3eb824fe..1277ac2a23 100644
--- a/packages/block-editor/src/components/grid/grid-item-resizer.js
+++ b/packages/block-editor/src/components/grid/grid-item-resizer.js
@@ -73,8 +73,8 @@ function GridItemResizerInner( {
        }, [ blockElement, rootBlockElement ] );
 
        const justification = {
-               right: 'flex-start',
-               left: 'flex-end',
+               right: 'left',
+               left: 'right',
        };
 
        const alignment = {

This means that the GridItemResizerInner component will consistently use logic based on physical direction instead of logical direction, as shown below:

The active side is determined here based on the physical position.
When we operate the resize handle, which handle was operated is determined via the onResizeStart prop based on the physical placement.
The justification variable determines the direction in which the element is physically placed.

This seems like a simpler approach than having to add logic to consider whether the language is RTL or not. What do you think?

andrewserong · 2024-09-12T01:04:24Z

This seems like a simpler approach than having to add logic to consider whether the language is RTL or not. What do you think?

Oh, I really like that idea. This makes sense, too, because we're dealing with a component that's about moving something visually, which is conceptually different to RTL language support 👍

t-hamano · 2024-09-12T03:13:38Z

@andrewserong Thanks for the feedback!

@rohitmathur-7 Making the changes suggested in this comment seems the best option for now, could you update this PR?

rohitmathur-7 · 2024-09-12T05:13:06Z

@andrewserong Thanks for the feedback!

@rohitmathur-7 Making the changes suggested in this comment seems the best option for now, could you update this PR?

Yes sure @t-hamano ,
I will update the PR soon!
Thanks for the feedback everyone. 🙇

This reverts commit 39cfde5.

rohitmathur-7 · 2024-09-12T16:08:29Z

Hii @t-hamano ,
I have updated the PR to add changes of this comment.
Can you please check now.
Thanks. 🙇

t-hamano

LGTM! Thank for the update 👍

Fixed the RTL resizing issue

39cfde5

rohitmathur-7 requested a review from ellatrix as a code owner September 3, 2024 05:41

Removed comment, fix spacing

0dbfff8

t-hamano self-requested a review September 3, 2024 13:16

t-hamano reviewed Sep 3, 2024

View reviewed changes

t-hamano added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. l10n Localization and translations best practices labels Sep 3, 2024

t-hamano requested review from noisysocks and tellthemachines September 3, 2024 13:33

Added else condition

27e7550

rohitmathur-7 requested a review from t-hamano September 3, 2024 15:02

t-hamano approved these changes Sep 3, 2024

View reviewed changes

t-hamano mentioned this pull request Sep 10, 2024

Grid Layout Improvements #57478

Open

37 tasks

rohitmathur-7 added 2 commits September 12, 2024 21:29

Revert "Fixed the RTL resizing issue"

c0d233c

This reverts commit 39cfde5.

Updated left and right values for justification

52dd04c

rohitmathur-7 requested a review from t-hamano September 12, 2024 16:08

t-hamano approved these changes Sep 13, 2024

View reviewed changes

t-hamano merged commit 1664022 into WordPress:trunk Sep 13, 2024
61 checks passed

github-actions bot added this to the Gutenberg 19.3 milestone Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grid: In RTL languages, the resize handles point in the opposite direction #64995

Grid: In RTL languages, the resize handles point in the opposite direction #64995

rohitmathur-7 commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

t-hamano left a comment

rohitmathur-7 commented Sep 3, 2024

t-hamano left a comment

andrewserong commented Sep 10, 2024

t-hamano commented Sep 10, 2024

andrewserong commented Sep 11, 2024

t-hamano commented Sep 11, 2024

andrewserong commented Sep 12, 2024 •

edited

Loading

t-hamano commented Sep 12, 2024

rohitmathur-7 commented Sep 12, 2024

rohitmathur-7 commented Sep 12, 2024

t-hamano left a comment

Grid: In RTL languages, the resize handles point in the opposite direction #64995

Grid: In RTL languages, the resize handles point in the opposite direction #64995

Conversation

rohitmathur-7 commented Sep 3, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Sep 3, 2024 • edited Loading

t-hamano left a comment

Choose a reason for hiding this comment

rohitmathur-7 commented Sep 3, 2024

t-hamano left a comment

Choose a reason for hiding this comment

andrewserong commented Sep 10, 2024

t-hamano commented Sep 10, 2024

andrewserong commented Sep 11, 2024

t-hamano commented Sep 11, 2024

andrewserong commented Sep 12, 2024 • edited Loading

t-hamano commented Sep 12, 2024

rohitmathur-7 commented Sep 12, 2024

rohitmathur-7 commented Sep 12, 2024

t-hamano left a comment

Choose a reason for hiding this comment

rohitmathur-7 commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

andrewserong commented Sep 12, 2024 •

edited

Loading