ListView: Update drop indicator line positioning to support rtl languages #51284
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
|
Size Change: +196 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
andrewserong
requested review from
noisysocks,
talldan,
apeatling,
ramonjd and
tellthemachines
apeatling
reviewed
Jun 7, 2023
Great catch! I'll look into it and see what's going on. |
Hrm, I'm having trouble reproducing this. @apeatling, which browser and browser version are you on, just in case that helps to isolate it? |
|
Update: I can sometimes reproduce via |
|
Another update: I finally got to the bottom of the issue — it turns out the This PR isn't quite ready for a final review. While working on that fix, I noticed that when running not in full screen, the left edge of the drop indicator needs a different calculation if there's horizontal scroll:
I'll look into a fix for that tomorrow. Thanks again for the review @apeatling, I don't think I'd have caught the Popover issue otherwise! |
|
Update: no update on this PR for today, as I've been looking at other Popover issues. I'll come back to this one next week for a final pass. |
andrewserong
force-pushed
the
fix/list-view-drop-indicator-line-position-in-rtl-languages
branch
from
411d457
to
30fa0f1
Compare
|
Update: I believe I've resolved all the outstanding issues, so this should be ready for a final review now. As an example, the left position and width of the drop indicator is now calculated correctly in all cases (I believe). Tested the following:
It appears to be working well for me so far. Here's an example of non-fullscreen in LTR now working correctly: 2023-06-14.15.07.29.mp4 |
andrewserong
commented
Jun 14, 2023
Comment on lines
-324
to
-353
| // If the user is dragging towards the bottom of the block check whether | ||
| // they might be trying to nest the block as a child. | ||
| // If the block already has inner blocks, and is expanded, this should be treated | ||
| // as nesting since the next block in the tree will be the first child. | ||
| // However, if the block is collapsed, dragging beneath the block should | ||
| // still be allowed, as the next visible block in the tree will be a sibling. | ||
| if ( | ||
| isDraggingBelow && | ||
| candidateBlockData.canInsertDraggedBlocksAsChild && | ||
| ( ( candidateBlockData.innerBlockCount > 0 && | ||
| candidateBlockData.isExpanded ) || | ||
| isNestingGesture( | ||
| position, | ||
| candidateRect, | ||
| candidateBlockParents.length | ||
| ) ) | ||
| ) { | ||
| // If the block is expanded, insert the block as the first child. | ||
| // Otherwise, for collapsed blocks, insert the block as the last child. | ||
| const newBlockIndex = candidateBlockData.isExpanded | ||
| ? 0 | ||
| : candidateBlockData.innerBlockCount || 0; | ||
|
|
||
| return { | ||
| rootClientId: candidateBlockData.clientId, | ||
| blockIndex: newBlockIndex, | ||
| dropPosition: 'inside', | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
This needed to be moved further up in the file as the nesting gesture needs to be checked for prior to the up gesture — otherwise, it's possible for a user to drag "below" an expanded block with children, which is visually unexpected as the next sibling position is lower than the next block in the list view.
|
Flaky tests detected in 30fa0f1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5263269499
|
apeatling
approved these changes
Jun 14, 2023
|
Thanks for the thorough testing! 🙇 |
|
After re-testing, I think I might have uncovered another subtle edge case when the list view depth is only just further horizontally than the width of the scrollbar container. There might be a tiny tweak I need to make to the logic... I'll dig in. |
Fixed in c448f0c. It turns out I needed to factor in the padding as well when checking if the scroll container is narrower than the list view item. I think this should be good to go now. |
andrewserong
force-pushed
the
fix/list-view-drop-indicator-line-position-in-rtl-languages
branch
from
c448f0c
to
6c549cd
Compare
|
That last commit was pretty minor, and I think fairly safe, so I'll merge this in now. Happy to look into any follow-ups if folks run into any issues! |
andrewserong
deleted the
fix/list-view-drop-indicator-line-position-in-rtl-languages
branch
25 tasks
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…ages (WordPress#51284) * ListView: Update drop indicator line positioning to support rtl languages * Update tests to include rtl equivalents * Ensure drop indicator line doesn't break out of scroll containers in both LTR and RTL languages * Users shouldn't be able to drag below a non-empty expanded block * Fix width issue when target is only slightly wider than the scroll container
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?
Update the
ListView's drop indicator line and drop zone logic to add support for RTL languages.Why?
In RTL languages the nesting for list view items is also displayed right-to-left, however prior to this PR, the drop indicator logic rendered as though it should always be left aligned. This would make the drag and drop behaviour appear to be broken in RTL languages.
How?
isRTL()in the drop zone logic and pass that value down, so that a) it's easier to write tests, and b) so that we're just checking a boolean in all the regular calls to check for RTL.Testing Instructions
Thank you for taking a look at this PR! There are lots of edge cases when it comes to testing list view drag and drop, so manually testing could take a while. ☕ ⌛
Some heavily nested block markup
Test the following in both RTL and LTR languages to ensure that RTL support is added, while not introducing any regressions for LTR languages.
Screenshots or screencast