这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@SalehTZ
Copy link
Contributor

@SalehTZ SalehTZ commented Jun 2, 2025

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.
Before Screenshots
LTR Not Expanded LTR Expanded RTL Not Expanded RTL Expanded
before_ltr_non_expanded jpg before_ltr_expanded jpg before_rtl_non_expanded before_rtl_expanded

After:

  • Toolbar items and overflow menus are correctly ordered and aligned for RTL.
  • Button padding is now directionally correct.
After Screenshots
LTR Not Expanded LTR Expanded RTL Not Expanded RTL Expanded
after_ltr_non_expanded after_ltr_expanded after_rtl_not_expanded after_rtl_expanded

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.

SalehTZ added 5 commits June 2, 2025 08:46
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.
@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jun 2, 2025
@SalehTZ SalehTZ marked this pull request as ready for review June 2, 2025 12:15
@dkwingsmt dkwingsmt requested a review from justinmc June 4, 2025 18:31
Copy link
Contributor

@justinmc justinmc left a 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.

@SalehTZ
Copy link
Contributor Author

SalehTZ commented Jun 7, 2025

@justinmc Is it good to add this line to hitTestChildren method?

// 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);
      },
    );
  }

SalehTZ added 2 commits June 7, 2025 15:37
…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.
Copy link
Contributor

@justinmc justinmc left a 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.

@SalehTZ
Copy link
Contributor Author

SalehTZ commented Jun 10, 2025

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.

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:

  • Overflowed menu expands children horizontally
  • puts children in an overflow menu if they overflow
  • puts children in an overflow menu if they overflow in RTL

with this common message:

The following assertion was thrown running a test:
Finder specifies a widget that would not receive pointer events.
A call to tap() with finder "Found 1 widget with widget matching predicate descending from widget
with type "MaterialApp": [
_TextSelectionToolbarOverflowButton-[<StandardComponentType.moreButton>],
]" derived an Offset (Offset(732.0, 70.0)) that would not hit test on the specified widget.
Maybe the widget is actually off-screen, or another widget is obscuring it, or the widget cannot
receive pointer events.

If its priority isn't high, I'll work on this later and create a separate issue and PR for it.

SalehTZ and others added 5 commits June 10, 2025 12:35
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.
@SalehTZ SalehTZ requested a review from justinmc June 12, 2025 04:22
Copy link
Contributor

@justinmc justinmc left a 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.

@SalehTZ
Copy link
Contributor Author

SalehTZ commented Jun 14, 2025

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 ✌🏻😃

@dkwingsmt dkwingsmt requested a review from victorsanni June 18, 2025 18:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 1, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
@SalehTZ SalehTZ deleted the fix-text-selection-toolbar-in-rtl branch July 10, 2025 06:04
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
<!--
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 |
|---|---|---|---|
| ![before_ltr_non_expanded
jpg](https://github.com/user-attachments/assets/bccf719e-c95a-46b1-b924-d2fb229e4ada)
| ![before_ltr_expanded
jpg](https://github.com/user-attachments/assets/a947690a-62fb-4f41-863c-d3822294bb66)
|
![before_rtl_non_expanded](https://github.com/user-attachments/assets/1e3440ee-d80c-4554-aeda-7632f2b166ca)
|
![before_rtl_expanded](https://github.com/user-attachments/assets/364a3b2b-d068-4488-afbd-61224e74c30a)
|

</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 |
|---|---|---|---|
|
![after_ltr_non_expanded](https://github.com/user-attachments/assets/b63bf625-0d37-4304-9ba5-af017c197b15)
|
![after_ltr_expanded](https://github.com/user-attachments/assets/6a95c9b7-e343-4e7e-93ae-f18de6d38a2e)
|
![after_rtl_not_expanded](https://github.com/user-attachments/assets/8a93b08b-fcf2-45a9-8f5a-390133c0aec4)
|
![after_rtl_expanded](https://github.com/user-attachments/assets/5cbdffd4-3943-44cc-98d9-fa470e11b942)
|

</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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Material text selection menu wrong item order in RTL

3 participants