[RenderParagraph] - Should call markNeedsLayout in the textAlign setter

matthew-carroll commented 10 months ago

Steps to reproduce

Use the provided reproduction code
Tap on the text to change the alignment from left to right

Expected results

The LayoutBuilder surrounding the Text widget should build again, because the layout of its descendantText changed.

Actual results

The LayoutBuilder doesn't run again because the descendant Text didn't report a layout change. It only reported dirty paint.

Code sample

```dart import 'package:flutter/material.dart'; const Color darkBlue = Color.fromARGB(255, 18, 32, 47); void main() { runApp(MyApp()); } class MyApp extends StatelessWidget { @override Widget build(BuildContext context) { return MaterialApp( theme: ThemeData.dark().copyWith( scaffoldBackgroundColor: darkBlue, ), debugShowCheckedModeBanner: false, home: Scaffold( body: MyWidget(), ), ); } } class MyWidget extends StatefulWidget { @override State createState() => _MyWidgetState(); } class _MyWidgetState extends State { @override Widget build(BuildContext context) { return LayoutBuilder( builder: (context, constraints) { print("Running LayoutBuilder"); return SizedBox( width: double.infinity, child: TextAlignChanger(), ); } ); } } class TextAlignChanger extends StatefulWidget { @override State createState() => _TextAlignChangerState(); } class _TextAlignChangerState extends State { TextAlign _textAlign = TextAlign.left; @override Widget build(BuildContext context) { return GestureDetector( onTap: () => setState(() { print("Changing text alignment to the right"); _textAlign = TextAlign.right; }), child: Text( 'Hello, World!', textAlign: _textAlign, style: Theme.of(context).textTheme.headlineMedium, ), ); } } ```

Screenshots or Video

No response

Logs

No response

Flutter Doctor output

Reproduced in Dart Pad: Based on Flutter 3.16.5 Dart SDK 3.2.3

moffatman commented 10 months ago

Just to clarify, why is this change needed, because text align change shouldn't change the size of text. Just the distribution of words within each line.

matthew-carroll commented 10 months ago

why is this change needed

To repaint a caret at the appropriate location within the text.

text align change shouldn't change the size of text. Just the distribution of words within each line

Layout can be more than just bounding dimensions. RenderParagraph includes queries related to character boxes, it's therefore reasonable to consider "text layout" to be part of "Flutter layout" and give ancestors an opportunity to react when those details change.

dam-ease commented 10 months ago

Reproduced this on the latest master and stable channels following the steps highlighted.

stable, master flutter doctor -v

``` [!] Flutter (Channel stable, 3.16.5, on macOS 14.2.1 23C71 darwin-arm64, locale en-NG) • Flutter version 3.16.5 on channel stable at /Users/damilolaalimi/sdks/flutter ! Warning: `dart` on your path resolves to /opt/homebrew/Cellar/dart/3.1.5/libexec/bin/dart, which is not inside your current Flutter SDK checkout at /Users/damilolaalimi/sdks/flutter. Consider adding /Users/damilolaalimi/sdks/flutter/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision 78666c8dc5 (2 weeks ago), 2023-12-19 16:14:14 -0800 • Engine revision 3f3e560236 • Dart version 3.2.3 • DevTools version 2.28.4 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0) • Android SDK at /Users/damilolaalimi/Library/Android/sdk • Platform android-34, build-tools 34.0.0 • ANDROID_HOME = /Users/damilolaalimi/Library/Android/sdk • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 15.1) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 15C65 • CocoaPods version 1.14.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.2) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) [!] Android Studio (version unknown) • Android Studio at /Users/damilolaalimi/Downloads/Android Studio Preview.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart ✗ Unable to determine Android Studio version. • Java version OpenJDK Runtime Environment (build 17.0.7+0-17.0.7b1000.6-10550314) [✓] VS Code (version 1.85.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.78.0 [✓] VS Code (version 1.83.1) • VS Code at /Users/damilolaalimi/Downloads/Visual Studio Code.app/Contents • Flutter extension version 3.78.0 [✓] Connected device (5 available) • sdk gphone64 arm64 (mobile) • emulator-5554 • android-arm64 • Android 14 (API 34) (emulator) • Damilola’s iPhone (mobile) • 00008110-001964480AE1801E • ios • iOS 17.1.1 21B91 • iPhone 15 (mobile) • F17D2919-6D61-4295-8408-1719692FE958 • ios • com.apple.CoreSimulator.SimRuntime.iOS-17-0 (simulator) • macOS (desktop) • macos • darwin-arm64 • macOS 14.2.1 23C71 darwin-arm64 • Chrome (web) • chrome • web-javascript • Google Chrome 120.0.6099.129 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 2 categories. ``` ``` [!] Flutter (Channel master, 3.18.0-18.0.pre.102, on macOS 14.2.1 23C71 darwin-arm64, locale en-NG) • Flutter version 3.18.0-18.0.pre.102 on channel master at /Users/damilolaalimi/fvm/versions/master ! Warning: `flutter` on your path resolves to /Users/damilolaalimi/sdks/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /Users/damilolaalimi/fvm/versions/master. Consider adding /Users/damilolaalimi/fvm/versions/master/bin to the front of your path. ! Warning: `dart` on your path resolves to /opt/homebrew/Cellar/dart/3.1.5/libexec/bin/dart, which is not inside your current Flutter SDK checkout at /Users/damilolaalimi/fvm/versions/master. Consider adding /Users/damilolaalimi/fvm/versions/master/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision da62d4a86d (2 hours ago), 2024-01-04 00:57:26 -0500 • Engine revision 7d5a120a60 • Dart version 3.3.0 (build 3.3.0-276.0.dev) • DevTools version 2.31.0-dev.0 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0) • Android SDK at /Users/damilolaalimi/Library/Android/sdk • Platform android-34, build-tools 34.0.0 • ANDROID_HOME = /Users/damilolaalimi/Library/Android/sdk • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 15.1) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 15C65 • CocoaPods version 1.14.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.2) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) [!] Android Studio (version unknown) • Android Studio at /Users/damilolaalimi/Downloads/Android Studio Preview.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart ✗ Unable to determine Android Studio version. • Java version OpenJDK Runtime Environment (build 17.0.7+0-17.0.7b1000.6-10550314) [✓] VS Code (version 1.85.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.78.0 [✓] VS Code (version 1.83.1) • VS Code at /Users/damilolaalimi/Downloads/Visual Studio Code.app/Contents • Flutter extension version 3.78.0 [✓] Connected device (5 available) • sdk gphone64 arm64 (mobile) • emulator-5554 • android-arm64 • Android 14 (API 34) (emulator) • Damilola’s iPhone (mobile) • 00008110-001964480AE1801E • ios • iOS 17.1.1 21B91 • iPhone 15 (mobile) • F17D2919-6D61-4295-8408-1719692FE958 • ios • com.apple.CoreSimulator.SimRuntime.iOS-17-0 (simulator) • macOS (desktop) • macos • darwin-arm64 • macOS 14.2.1 23C71 darwin-arm64 • Chrome (web) • chrome • web-javascript • Google Chrome 120.0.6099.129 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 2 categories. exit code 0 ```

SelaseKay commented 10 months ago

Hi. Please can I work on resolving this issue?

SelaseKay commented 10 months ago

@matthew-carroll I tried out your suggested fix but it doesn't seem to resolve the issue. Calling markNeedsLayout in the textAlign setter does not trigger a rebuild in the LayoutBuilder when alignment of the Text widget changes.

matthew-carroll commented 10 months ago

@SelaseKay - Ok, the minimal repro might need to be a bit different. I know for sure that adding markNeedsLayout() solves my problem, but my problem exists in a custom text field implementation, which includes a huge amount of code, so I can't paste that here as a minimal reproduction.

Here's another code repro. Try this one instead. In this case, after you change to markNeedsLayout(), you should see "Running layout around text." appear every time you change the text alignment.

import 'package:flutter/material.dart';

const Color darkBlue = Color.fromARGB(255, 18, 32, 47);

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData.dark().copyWith(
        scaffoldBackgroundColor: darkBlue,
      ),
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        body: MyWidget(),
      ),
    );
  }
}

class MyWidget extends StatefulWidget {
  @override
  State<MyWidget> createState() => _MyWidgetState();
}

class _MyWidgetState extends State<MyWidget> {
  @override
  Widget build(BuildContext context) {
    return CustomMultiChildLayout(
      delegate: ChildLayoutDelegate(),
      children: [
        LayoutId(
          id: "1",
          child: SizedBox(
                width: double.infinity, 
                child: TextAlignChanger(),
              ),
        ),
      ],
    );
  }
}

class ChildLayoutDelegate extends MultiChildLayoutDelegate {
  @override
  void performLayout(Size size) {
    print("Running layout around text.");
    layoutChild("1", BoxConstraints.loose(size));
  }

  @override
  bool shouldRelayout(covariant MultiChildLayoutDelegate oldDelegate) {
    return true;
  }

}

class TextAlignChanger extends StatefulWidget {
  @override
  State<TextAlignChanger> createState() => _TextAlignChangerState();
}

class _TextAlignChangerState extends State<TextAlignChanger> {
  TextAlign _textAlign = TextAlign.left;

  @override
  Widget build(BuildContext context) {
    return GestureDetector(
      onTap: () => setState(() {
        print("Changing text alignment to the right");
        _textAlign = TextAlign.right;
      }),
      child: Text(
        'Hello, World!',
        textAlign: _textAlign,
        style: Theme.of(context).textTheme.headlineMedium,
      ),
    );
  }
}

SelaseKay commented 10 months ago

Alright. I'll try it out

SelaseKay commented 10 months ago

@matthew-carroll It works fine for the current code repro. I want to create a PR for this but I'm not really sure on how to write a test for it.

matthew-carroll commented 10 months ago

You can take the code I posted, put it in a test, increment a counter in the layout method, change the text alignment, and then expect that the counter ran 2 times: one time for initial layout, and one time after the text alignment changed.

LongCatIsLooong commented 10 months ago

I don't think text justification changes should cause RenderParagraph or RenderEditable to redo layout, as it doesn't affect line breaking.

The LayoutBuilder surrounding the Text widget should build again, because the layout of its descendantText changed.

LayoutBuilder only rebuilds when the incoming constraints change. Even if you call markNeedsLayout in the text align setter, there's a chance the markNeedsLayout call won't propagate far enough to reach the LayoutBuilder.

give ancestors an opportunity to react when those details change.

You could set up custom painting logic in the ancestor render object to do that. But right now not all text layout info is available / it doesn't notify you when the text layout changes.

matthew-carroll commented 10 months ago

@LongCatIsLooong if you feel that what I'm requesting shouldnt' be done, then please be sure to provide clear instructions for how to achieve the desired goal without that change. I need to rerun layout for things that are based on text layout and I get no notification to respond to. How do I trigger rebuilds of text-layout-aware widgets when RenderParagraph doesn't re-run layout upon text alignmetn chnage?

As I mentioned above, bounding boxes are not the only thing relevant to layout. We call it "text layout" for a reason - it's layout. When character boxes move, I should be able to respond to that layout change with layout changes of my own. But Flutter isn't treating text layout as layout, thus the problem.

LongCatIsLooong commented 10 months ago

I don't think there's a reliable way to get notified when the text layout changes right now. I've been thinking about introducing something to RenderEditable / RenderParagraph for that, to decouple cursor stuff from RenderEditable for example. Could you elaborate a bit more on your use case, so I can factor that in?

Also if you created that render paragraph then likely you have access to the textAlign value. In that case you don't have to rely on markNeedsLayout to get notified.

matthew-carroll commented 10 months ago

I don't think there's a reliable way to get notified when the text layout changes right now

For this ticket there's a very reliable way. You just call markNeedsLayout() in set textAlign instead of markNeedsPaint().

Could you elaborate a bit more on your use case, so I can factor that in?

My use case is all text editor and text field use-cases.

Also if you created that render paragraph then likely you have access to the textAlign value. In that case you don't have to rely on markNeedsLayout to get notified.

No, this isn't true at all. I'm building a widget tree, not a render tree. I need other widgets to have rebuild triggered (or at least relayout).

to decouple cursor stuff from RenderEditable for example

If "cursor" is just an example, that's fine. But please don't engineer things based on just the "cursor". I've mentioned many times, and filed many tickets, related to arbitrary content being placed in relation to text. Yes, a cursor/careet is one such thing. So are selection boxes, drag handles, magnifiers, toolbars, squiggly spelling underlines, etc, etc, etc. Please try to structure text layout as something that can be generally used by the rest of the widget tree to align, position, and size arbitrary content. Hacking around this limitation at various levels has been one of the biggest sources of code within SuperEditor and SuperTextField.

That said, what I'm looking for right now is an adjustment so that changing text alignment causes reflowing of layout in other related widgets. That's my current problem.

LongCatIsLooong commented 10 months ago

markNeedsLayout doesn't always force the parent Render Object to relyaout. It's only reliable if there are no relayout boundaries between the layout builder and the text.

matthew-carroll commented 10 months ago

markNeedsLayout doesn't always force the parent Render Object to relyaout. It's only reliable if there are no relayout boundaries between the layout builder and the text.

I'm not sure what you're looking for here. I don't know what percentage of layouts that care about text layout have layout boundaries in between. I do know that my case doesn't. I'm guessing many others don't as well. I don't think it's helpful to let one case ruin the situation for all cases.

I think my points in this thread are pretty simple and pretty clear:

Text layout is layout
Text alignment changes text layout
Changing text alignment should invalidate layout like any other aspect of a render object that impacts layout

SelaseKay commented 10 months ago

@matthew-carroll Should I close my current PR for this fix or we should wait for @LongCatIsLooong feedback?

matthew-carroll commented 10 months ago

@SelaseKay that's up to you and whoever is reviewing your PR.

LongCatIsLooong commented 9 months ago

@matthew-carroll The sample code does not show how the LayoutBulider depends on the text layout of the Text widget. Are you accessing the public methods on RenderParagraph to get the current text layout and wish to monitor the positions of certain characters?

I don't think it matters whether text layout is usually considered layout. markNeedsLayout should typically only be called on a render object when inputs to its layout algorithm (performLayout / performResize) change. If you create a render object that computes its size based on its color then color is layout. Also whether a render object is a relayout boundary is determined by the framework. In other words whether another render object needs relayout is typically considered an implementation detail that you shouldn't rely on, unless you know exactly what render objects will be in the path from this render object to the target render object, and how they implemented their performLayout.

If the use case is

To repaint a caret at the appropriate location within the text.

then I think you can create a render object that does the caret repaint, and make it the direct parent of RenderParent, since RenderParagraph is not a repaint boundary.

@SelaseKay The patch looks harmless but I'm still trying to figure out what functionality we're enabling here by changing markNeedsPaint to markNeedsLayout.

matthew-carroll commented 9 months ago

If you create a render object that computes its size based on its color then color is layout.

YES! If there's a render object that changes layout when it's color changes, it should markNeedsLayout() when it's color changes!

then I think you can create a render object that does the caret repaint, and make it the direct parent of RenderParent, since RenderParagraph is not a repaint boundary.

I've dealt with this terrible recommendation a number of times now. This is an absolutely unacceptable expectation for this issue and I'm becoming frustrated with the Flutter team not listening to feedback on this. CREATING RENDER OBJECTS IS REALLY REALLY COMPLICATED!!!!!! There's absolutely no justification for requiring developers to learn how to create render objects just to paint an appropriately positioned blinking rectangle. Moreover, these overlays are often not the "direct parent" of the text layout. It would be extremely restrictive - PROHIBITIVELY restrictive to require such a thing.

And it's not just me. I've got a number of clients who feel the exact same way. The failure to empathize with this situation is now a customer service issue and it's impacting multiple areas of development within SuperEditor, SuperReader, SuperTextField, and SuperText.

@justinmc - I'm not sure if you're still overseeing things related to text layout, etc, but it sure would be nice to see an improvement in the Flutter team's understanding of goals we're working on out here. It would be nice for Flutter team devs to empathize with the customer perspective, and then based on that understanding, to help resolve customer problems, rather than tell us they aren't problems and tell us the answer is to do very complicated things. Like I said above, this is super frustrating and I'm getting close to the point of simply not filing any more of these issues because I feel like I'm punished every time that I do. The same thing goes for this important (somewhat related) issue: https://github.com/flutter/flutter/issues/123305

justinmc commented 9 months ago

@matthew-carroll when you say "developers" are you talking about yourself or people building applications using super_editor? I agree it would be prohibitive to require devs using super_editor to have to write their own RenderObjects for small tasks.

I think it's useful for Flutter to keep track of issues like this and https://github.com/flutter/flutter/issues/123305 even if no one has been able to find a solution, so thanks for filing and dealing with all of the back and forth.

matthew-carroll commented 9 months ago

@justinmc I mean developers using super_editor. Imagine any of the number of ways various companies might want to customize or add decorations to text. Just like I mentioned in the content layers linked issue, teams want to control the appearance and sometimes the relative positioning of things on top of the text and things below the text. The official stance of the Flutter team has been "well, they should write custom render objects". It's incredibly tone deaf.

flutter / flutter