Move shared inline widget logic to `RenderInlineWidgetContainerDefaults` #127308

LongCatIsLooong · 2023-05-22T09:53:08Z

Added InlineWidgetContainerDefaults for deduping inline widget code
Added a helper function WidgetSpan.extractFromInlineSpan for extracting WidgetSpans and automatically applying text scaling (at widget level)
Removed TextPainter.inlinePlaceholderScales. I'm going to deprecate the scale argument in TextPainter.addPlaceholder next, as scaling is now done at the widget level.
Added runtime check and comments to make sure nobody is extending PlaceholderSpan directly (unfortunately we can't remove PlaceholderSpan without moving RenderEditable and RenderParagraph to the widgets library).

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai

Just double check, is this also solving the issue of double scaling of the text widget inside inline widget?

chunhtai · 2023-05-23T17:22:09Z

packages/flutter/lib/src/painting/placeholder_span.dart

+/// of the placeholder.
+///
+/// Flutter widgets such as [TextField], [Text] and [RichText] do not recognize
+/// [PlaceholderSpan] subclasses other than [WidgetSpan]. **Consider


If that is the case, should we deprecate PlaceholderSpan and expose widgetspan directly?

We can add a typedef PlaceholderSpan = WidgetSpan; for a soft transition

RenderEditable and RenderParagraph need to be moved to the widget library if we want to remove/deprecate PlaceholderSpan. Let me see if I can just hide PlaceholderSapn.

Oh wait with the ParentDataWidget trick maybe we don't need the PlaceholderSpan class anymore.

ah I see. I guess that is why the PlaceholderSpan was first introduced. I think we should avoid package level private.

we could use interface modifier, but people can still implements it.

Should it still be considered valid to extends PlaceHolderSpan if developer builds on top Rendering library without Widget library?

Should it still be considered valid to extends PlaceHolderSpan if developer builds on top Rendering library without Widget library?

Yes but it would be very confusing when people write extends PlaceholderSpan, use the class in Text or TextField, and see their PlaceholderSpan class isn't working, and the inline widgets are all messed up. ~~Like you said we could do a typedef and I think everything would work?~~ well I forgot we can't import widget_span.dart there.

Looks like it's possible to remove PlaceholderSpan from RenderEditable and RenderParagraph, I just have to replace TextParentData.span with TextParentData.alignment and TextParentData.baselineOffset. But it's probably for a different PR.

chunhtai · 2023-05-23T18:13:50Z

packages/flutter/lib/src/rendering/paragraph.dart

+  String toString() =>'widget: $span, ${offset == null ? "not laid out" : "offset: $offset"}';
+}
+
+/// A mixin that provides useful default behaviors for text [RenderBox]es


what are the defaults?

The default layout/paint/hitTest implementations I assume. I followed this class: https://master-api.flutter.dev/flutter/rendering/RenderBoxContainerDefaultsMixin-mixin.html

would be good to document these in the doc string

Added documentation for when to call each default method.

LongCatIsLooong · 2023-05-23T21:47:33Z

is this also solving the issue of double scaling of the text widget inside inline widget?

It doesn't but my plan is to expose a flag WidgetSpan.automaticallyApplyTextScale or some such so people can opt out of the current automatic inline widget scaling when the inline widget is something more complex than an inline icon/image.

chunhtai · 2023-05-23T22:36:16Z

is this also solving the issue of double scaling of the text widget inside inline widget?

It doesn't but my plan is to expose a flag WidgetSpan.automaticallyApplyTextScale or some such so people can opt out of the current automatic inline widget scaling when the inline widget is something more complex than an inline icon/image.

That doesn't seem like a clean API though... we can continue the discussion in dd https://docs.google.com/document/d/1-DlElw3zWRDlqoc9z4YfkU9PbBwRTnc7NhLM97ljGwk/edit?disco=AAAAxR2fXig

goderbauer · 2023-05-26T19:49:04Z

packages/flutter/lib/src/painting/placeholder_span.dart

+/// [PlaceholderSpan] and may be used instead to specify a widget as the contents
+/// of the placeholder.
+///
+/// Flutter widgets such as [TextField], [Text] and [RichText] do not recognize


Is this a new limitation introduced by this PR? Or was this a problem before?

Ah, is problem here that these classes all do type checks ala span is WidgetSpan? That's kinda unfortunate...

Yeah PlaceholderSpan doesn't know how to generate renderable content (while WidgetSpan can), but in RenderEditable and RenderParagraph we can't do span is WidgetSpan unfortunately. But I think we'll be able to avoid referencing PlaceholderSpan in this mixin, since it's using ParentData instead of extracting the PlaceholderSpans from the InlineSpan tree.

goderbauer · 2023-05-26T19:55:12Z

packages/flutter/test/widgets/text_test.dart

    expect(paragraph.getMinIntrinsicWidth(0.0), 200);
  });

+  testWidgets('can compute IntrinsicHeight for widget span with text scaling', (WidgetTester tester) async {


nit: looks like this test is now testing IntrinsicHeight and Width? Maybe update the title?

goderbauer · 2023-05-26T19:57:51Z

packages/flutter/lib/src/rendering/editable.dart

  @override
  @protected
  bool hitTestChildren(BoxHitTestResult result, { required Offset position }) {
    // Hit test text spans.


nit: this comment now seems kinda unnecessary?

goderbauer · 2023-05-26T20:05:35Z

packages/flutter/lib/src/rendering/paragraph.dart

+///
+///  * [WidgetSpan.extractFromInlineSpan], a helper function for extracting
+///    [WidgetSpan]s from an [InlineSpan] tree.
+mixin RenderInlineWidgetContainerDefaults on RenderBox, ContainerRenderObjectMixin<RenderBox, TextParentData> {


The naming here is a bit strange, "Widget" is a concept of the layer above rendering (where this is located in), so it technically doesn't have any knowledge of what a widget is.

Same for the method on this class that contain the name widget.

Same for the docs on this class. It should probably be rephrased to not be tied to the concept of widget (of course it can call out the WidgetSpan as an example...)

goderbauer · 2023-05-26T20:09:48Z

packages/flutter/lib/src/rendering/paragraph.dart

+        );
+  }
+
+  /// Computes the layout for every inline widget children using the given


it seems to operate on RenderBox children, though, not widgets?

goderbauer · 2023-05-26T20:09:55Z

packages/flutter/lib/src/rendering/paragraph.dart

+    ];
+  }
+
+  /// Positions each inline widget according to the coordinates provided in the


goderbauer · 2023-05-26T20:16:49Z

packages/flutter/lib/src/rendering/paragraph.dart

+  Offset? get offset => _offset;
+  Offset? _offset;
+
+  /// The [WidgetSpan] associated with this inline widget.


Its type is PlaceholderSpan, though?

goderbauer · 2023-05-26T20:20:00Z

packages/flutter/lib/src/widgets/widget_span.dart

+  /// Helper function for extracting [WidgetSpan]s in preorder, from the given
+  /// [InlineSpan] as a list of widgets.
+  ///
+  /// The extracted widgets will be annotated with necessary semantics


What's "necessary"? Necessary for what? :D

goderbauer · 2023-05-26T20:25:41Z

packages/flutter/lib/src/widgets/widget_span.dart

+  ///
+  /// The extracted widgets will be annotated with necessary semantics
+  /// information. This function is used by [EditableText] and [RichText] so
+  /// calling it directly is rarely necessary.


This should document the textScaleFactor param.

Also, for my own understanding: I thought we are getting rid of textScaleFactor?

That's the next step. If it's ok I think I'll just replace textScaleFactor with textScaler without deprecating in the migration PR given nobody but the framework widgets are calling this method.

goderbauer · 2023-05-26T20:32:14Z

packages/flutter/lib/src/widgets/widget_span.dart

+    if (child == null) {
+      return;
+    }
+    context.pushTransform(


Store the layerHandler this returns and pass it back in as oldLayer to give the engine more information for performance optimizations?

Also, should we skip the transform if the scale is 1.0?

goderbauer

LGTM

…erDefaults` (flutter/flutter#127308)

flutter/flutter@ee162e4...3db9504 2023-05-28 [email protected] Roll Flutter Engine from 340936f7c03c to a4d4ad9dae1a (1 revision) (flutter/flutter#127772) 2023-05-28 [email protected] Roll Flutter Engine from 57e491a46e05 to 340936f7c03c (1 revision) (flutter/flutter#127767) 2023-05-28 [email protected] Roll Flutter Engine from a37b1a38d97a to 57e491a46e05 (1 revision) (flutter/flutter#127765) 2023-05-28 [email protected] Roll Flutter Engine from b1698020bd57 to a37b1a38d97a (1 revision) (flutter/flutter#127761) 2023-05-27 [email protected] Roll Flutter Engine from 9e97c7379c7f to b1698020bd57 (1 revision) (flutter/flutter#127758) 2023-05-27 [email protected] Roll Flutter Engine from 0f38a0f0a18a to 9e97c7379c7f (1 revision) (flutter/flutter#127755) 2023-05-27 [email protected] Roll Flutter Engine from 1187168472a7 to 0f38a0f0a18a (1 revision) (flutter/flutter#127752) 2023-05-27 [email protected] Roll Flutter Engine from 3426cb827efb to 1187168472a7 (1 revision) (flutter/flutter#127745) 2023-05-27 [email protected] Roll Flutter Engine from 795db50e7453 to 3426cb827efb (9 revisions) (flutter/flutter#127738) 2023-05-27 [email protected] Updated the `ToggleButtons` API doc to link to `SegmentedButton` (flutter/flutter#127021) 2023-05-26 [email protected] Remove more `test_api/src` imports (flutter/flutter#127716) 2023-05-26 [email protected] Move shared inline widget logic to `RenderInlineWidgetContainerDefaults` (flutter/flutter#127308) 2023-05-26 [email protected] Roll Flutter Engine from 3b5b5fc96b8f to 795db50e7453 (1 revision) (flutter/flutter#127720) 2023-05-26 [email protected] Update collection-fors to prefer final (as per updated `prefer_final_in_for_each`) (flutter/flutter#127511) 2023-05-26 [email protected] Roll Flutter Engine from eed12f36f595 to 3b5b5fc96b8f (1 revision) (flutter/flutter#127713) 2023-05-26 [email protected] Remove references to deprecated `ThemeData.primaryColorBrightness` (flutter/flutter#127238) 2023-05-26 [email protected] TwoDimensional scrolling foundation (flutter/flutter#125437) 2023-05-26 [email protected] Roll Flutter Engine from 858d9753453c to eed12f36f595 (2 revisions) (flutter/flutter#127702) 2023-05-26 [email protected] Remove button announcement for `MenuItemButton` and `SubmenuButton` (flutter/flutter#127620) 2023-05-26 [email protected] Reland "Update labeler action wildcards #127524" (flutter/flutter#127690) 2023-05-26 [email protected] Revert "Log all lines from ios-deploy (#127502)" (flutter/flutter#127684) 2023-05-26 [email protected] Roll Flutter Engine from ec7b0ae3599f to 858d9753453c (2 revisions) (flutter/flutter#127700) 2023-05-26 [email protected] Add M3 date range picker tests and fix header background theme color (flutter/flutter#127662) 2023-05-26 [email protected] Support minifcation for apps that depend on AGP 8 and integration_test (flutter/flutter#127628) 2023-05-26 [email protected] Bump Snippet version (flutter/flutter#127688) 2023-05-26 [email protected] [Linux] Use up_client_get_devices2 when possible (flutter/flutter#127699) 2023-05-26 [email protected] Roll Flutter Engine from 84f2fc16e55d to ec7b0ae3599f (3 revisions) (flutter/flutter#127698) 2023-05-26 [email protected] Fix `TextField` error in production environment because it wrongly uses ancestor render boxes (flutter/flutter#126324) 2023-05-26 [email protected] Interactive viewer doesn't appear to respect the trackpadScrollCausesScale parameter (flutter/flutter#127114) 2023-05-26 [email protected] Remove release timeline (flutter/flutter#127685) 2023-05-26 [email protected] Manual roll Flutter Engine from 8573f3b63a1f to 84f2fc16e55d (7 revisions) (flutter/flutter#127676) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…erDefaults` (flutter/flutter#127308)

LongCatIsLooong added 4 commits May 19, 2023 18:12

WidgetSpan scaling

10c03c9

wip

349fd97

comments

ae1c9f3

rename

9f60fc1

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels May 22, 2023

fix doc references

255888c

LongCatIsLooong requested review from chunhtai and goderbauer May 22, 2023 17:20

LongCatIsLooong marked this pull request as ready for review May 22, 2023 17:35

chunhtai reviewed May 23, 2023

View reviewed changes

comments

52cc21b

goderbauer reviewed May 26, 2023

View reviewed changes

LongCatIsLooong added 2 commits May 26, 2023 15:02

Merge remote-tracking branch 'upstream/master' into inline-span-scaling

996b490

review

2e02160

github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 26, 2023

goderbauer approved these changes May 26, 2023

View reviewed changes

goderbauer mentioned this pull request May 26, 2023

Bot is not adding labels to PRs #127722

Closed

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label May 26, 2023

auto-submit bot merged commit b5df180 into flutter:master May 26, 2023

LongCatIsLooong deleted the inline-span-scaling branch May 26, 2023 23:48

engine-flutter-autoroll mentioned this pull request May 27, 2023

Roll Flutter from ee162e4b7f4f to fe22aaca5d6c (24 revisions) flutter/packages#4094

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 27, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

5742048

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll mentioned this pull request May 27, 2023

Roll Flutter from ee162e4b7f4f to fe22aaca5d6c (24 revisions) flutter/packages#4095

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 27, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

2844e37

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll mentioned this pull request May 27, 2023

Roll Flutter from ee162e4b7f4f to c23aadc77cc1 (25 revisions) flutter/packages#4096

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 27, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

d45d112

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll mentioned this pull request May 28, 2023

Roll Flutter from ee162e4b7f4f to 08b06137349e (30 revisions) flutter/packages#4097

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 28, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

2e12c64

…erDefaults` (flutter/flutter#127308)

daohoangson mentioned this pull request May 28, 2023

Verify inline widget scaling is working as expected daohoangson/flutter_widget_from_html#924

Open

engine-flutter-autoroll mentioned this pull request May 28, 2023

Roll Flutter from ee162e4b7f4f to 3db9504dcda2 (31 revisions) flutter/packages#4098

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 28, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

a87d772

…erDefaults` (flutter/flutter#127308)

moffatman mentioned this pull request Jul 14, 2023

_RenderScaledInlineWidget can throw assertion due to floating point rounding problems #130588

Closed

2 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

f62824e

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

8b5d277

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

6011453

…erDefaults` (flutter/flutter#127308)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b5df180 Move shared inline widget logic to `RenderInlineWidgetContain…

9d5595c

…erDefaults` (flutter/flutter#127308)

Move shared inline widget logic to RenderInlineWidgetContainerDefaults #127308

Move shared inline widget logic to RenderInlineWidgetContainerDefaults #127308

Uh oh!

Conversation

LongCatIsLooong commented May 22, 2023

Pre-launch Checklist

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented May 23, 2023

Uh oh!

chunhtai commented May 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Move shared inline widget logic to `RenderInlineWidgetContainerDefaults` #127308

Move shared inline widget logic to `RenderInlineWidgetContainerDefaults` #127308

LongCatIsLooong May 23, 2023 •

edited

Loading