-
Notifications
You must be signed in to change notification settings - Fork 29.5k
Fix text selection toolbar alignment for RTL languages #169854
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
Fix text selection toolbar alignment for RTL languages #169854
Conversation
…ility for both rtl and ltr
This change ensures that the Material text selection toolbar correctly positions its items (including the overflow button) according to the current `TextDirection` (LTR or RTL). Previously, the layout logic did not account for text direction, leading to incorrect placement of items in RTL layouts. The `_TextSelectionToolbarItemsLayout` and its corresponding `_RenderTextSelectionToolbarItemsLayout` now explicitly take a `textDirection` parameter. The `_placeChildren` method in the render object has been updated to arrange items from right-to-left when in an RTL context.
…ght-to-left in RTL This commit introduces tests to verify that: - Items in `TextSelectionToolbar` are correctly ordered from right-to-left when the `textDirection` is `TextDirection.rtl`. - The overflow menu in `TextSelectionToolbar` functions as expected in RTL mode, correctly displaying and hiding overflowed items.
…ght-to-left in RTL This commit introduces tests to verify that: - Items in `TextSelectionToolbar` are correctly ordered from right-to-left when the `textDirection` is `TextDirection.rtl`. - The overflow menu in `TextSelectionToolbar` functions as expected in RTL mode, correctly displaying and hiding overflowed items.
…l' into fix-text-selection-toolbar-in-rtl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for stepping in to fix this. This is a high quality PR fixing what must be a very annoying for RTL users. I appreciate the screenshots too.
The comments below are mostly small stuff, overall this looks good.
packages/flutter/lib/src/material/text_selection_toolbar_text_button.dart
Show resolved
Hide resolved
packages/flutter/test/material/text_selection_toolbar_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter/test/material/text_selection_toolbar_test.dart
Outdated
Show resolved
Hide resolved
|
@justinmc Is it good to add this line to // Don't hit test children aren't shown.
if (!childParentData.shouldPaint) {
return false;
}result be like: // Include the parent data offset in the hit test.
@override
bool hitTestChildren(BoxHitTestResult result, {required Offset position}) {
// The x, y parameters have the top left of the node's box as the origin.
final ToolbarItemsParentData childParentData = child!.parentData! as ToolbarItemsParentData;
// Don't hit test children aren't shown.
if (!childParentData.shouldPaint) {
return false;
}
return result.addWithPaintOffset(
offset: childParentData.offset,
position: position,
hitTest: (BoxHitTestResult result, Offset transformed) {
assert(transformed == position - childParentData.offset);
return child!.hitTest(result, position: transformed);
},
);
} |
…and vertical methods This change refactors the `_placeChildren` method in `TextSelectionToolbar` by extracting the logic for horizontal and vertical layouts into two new methods: `_placeChildrenHorizontally` and `_placeChildrenVertically`. This improves code readability and maintainability by clearly separating the distinct layout calculations.
The previous test for right-to-left (RTL) item ordering in `TextSelectionToolbar` incorrectly assumed items would be perfectly adjacent. This commit revises the test to accurately verify that items are positioned to the left of their preceding items in RTL, allowing for potential spacing or visual separation between them. The test now: - Adds `Text` widgets as children to `TestBox` for easier identification and position retrieval. - Verifies that `next.right` is less than or equal to `current.left` for each pair of items in RTL. - Additionally confirms the visual right-to-left order by comparing the sorted list of right edges with the actual list.
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see what happens with CI, hopefully when you update the PR it will go green.
About your hitTestChildren question in #169854 (comment). Is that the hitTestChildren in _TextSelectionToolbarTrailingEdgeAlignRenderBox? Yeah good optimization, I say add it.
packages/flutter/test/material/text_selection_toolbar_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter/test/material/text_selection_toolbar_test.dart
Outdated
Show resolved
Hide resolved
Yes it is, I forgot to mention the class 😁. Then i'll add and test it. Edit: unfortunately it causes tests mentioned below to fail:
with this common message:
If its priority isn't high, I'll work on this later and create a separate issue and PR for it. |
The comment incorrectly stated that item 2 should be rightmost in RTL. It has been corrected to leftmost.
This commit refactors the `_placeChildrenHorizontally` method in `TextSelectionToolbar` for improved clarity and efficiency. The changes include: - Renaming variables for better readability (e.g., `paintedContentItems` to `contentItems`, `totalHorizontalWidth` to `totalWidth`). - Simplifying the logic for collecting and positioning child items by iterating directly over children using `visitChildren`. - Using a boolean `showNavButton` derived from `_lastIndexThatFits` to determine if the navigation button should be painted, instead of a separate `navButtonIsPainted` flag.
This commit refactors the `_placeChildrenVertically` method within the `_TextSelectionToolbarContentLayout` class of the Material text selection toolbar. The changes include: - Using `visitChildren` for iterating over child render objects, replacing the manual linked-list traversal. - Renaming `maxItemWidth` to `maxWidth` for clarity. - Removing the unused `nextSize` variable. - Simplifying the logic for handling the navigation button. These changes improve the readability and maintainability of the vertical layout logic for the overflow menu in the Material text selection toolbar.
This commit refactors the logic within the `TextSelectionToolbar`'s `performLayout` method. Specifically, it streamlines the condition for determining whether a child should be painted. Instead of checking `_shouldPaintChild` and then setting `shouldPaint` to true or false, it now directly checks `!_shouldPaintChild` to set `shouldPaint` to false, with an `else` block to set it to true. This improves readability without changing the underlying behavior.
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if Google tests pass 👍 . Thanks for jumping in to help with RTL. It's a bit of a blindspot for me unfortunately.
(#169854 (comment)) If its priority isn't high, I'll work on this later and create a separate issue and PR for it.
Sounds good. I didn't expect a change of behavior from that optimization so I must have misread it. Makes sense to do it in a separate PR.
|
Thanks Justin, That's an honor to work on Flutter, I'll be contributing more in the future, especially on RTL. So count on me for RTL issues ✌🏻😃 |
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> **Fix text selection toolbar alignment for RTL (right-to-left) languages** This PR updates the Material text selection toolbar to properly support right-to-left (RTL) layouts. Previously, the toolbar and its buttons did not align or order correctly when using RTL text direction, leading to a broken or confusing user experience for RTL languages. **Summary of changes:** - Refactored layout logic in `text_selection_toolbar.dart` to handle RTL and LTR directions for both horizontal and overflow (vertical) arrangements. - Updated padding logic in `text_selection_toolbar_text_button.dart` to use `EdgeInsetsDirectional`, ensuring correct padding for both text directions. - Added new widget tests to verify that: - Toolbar items are ordered right-to-left in RTL contexts. - The overflow menu behaves correctly in RTL, showing items in the correct order and position. **Before:** - Toolbar items and overflow menus did not respect RTL layout, resulting in misaligned or reversed button order. - Padding was inconsistent in RTL. <details> <summary>Before Screenshots</summary> | LTR Not Expanded | LTR Expanded | RTL Not Expanded | RTL Expanded | |---|---|---|---| |  |  |  |  | </details> **After:** - Toolbar items and overflow menus are correctly ordered and aligned for RTL. - Button padding is now directionally correct. <details> <summary>After Screenshots</summary> | LTR Not Expanded | LTR Expanded | RTL Not Expanded | RTL Expanded | |---|---|---|---| |  |  |  |  | </details> This improves the usability and accessibility of text selection controls for users of RTL languages. Fixes flutter#59306 --- ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Fix text selection toolbar alignment for RTL (right-to-left) languages
This PR updates the Material text selection toolbar to properly support right-to-left (RTL) layouts. Previously, the toolbar and its buttons did not align or order correctly when using RTL text direction, leading to a broken or confusing user experience for RTL languages.
Summary of changes:
text_selection_toolbar.dartto handle RTL and LTR directions for both horizontal and overflow (vertical) arrangements.text_selection_toolbar_text_button.dartto useEdgeInsetsDirectional, ensuring correct padding for both text directions.Before:
Before Screenshots
After:
After Screenshots
This improves the usability and accessibility of text selection controls for users of RTL languages.
Fixes #59306
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.