RenderEditable WidgetSpan intrinsics #136979

LongCatIsLooong · 2023-10-20T19:54:31Z

Update the RenderEditable implementation to match RenderParagraph. Fixes #136596

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 · 2023-10-20T20:10:14Z

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

-  double computeMinIntrinsicHeight(double width) {
-    return _preferredHeight(width);
-  }
+  double computeMinIntrinsicHeight(double width) => computeMaxIntrinsicHeight(width);


hmmm not sure if this really applies in this context, computeMaxIntrinsicHeight doc says to call getMaxIntrinsicHeight instead.

This mirrors the RenderParagraph implementation. In this specific case compute{Min/Max}InsintricHeight have the same implementation so I'm calling computeMaxIntrinsicHeight here. It does seem weird since I'd imagine maxIntrinsicHeight should >= minIntrinsicHeight when the width is given, but it's what RenderParagraph currently does.

calling the same method is expected because height will be fixed once you given a width.

getMaxIntrinsicHeight:

Returns the smallest height beyond which increasing the height never decreases the preferred width. The preferred width is the value that would be returned by getMinIntrinsicWidth for that height.

getMinIntrinsicHeight:

Returns the minimum height that this box could be without failing to correctly paint its contents within itself, without clipping.

I think these 2 are supposed to be using different line breaking algorithms?

By definition I think the former should be the height of the paragraph when all soft break opportunities are turned into actual line breaks (so the input width doesn't matter):

hello world hello world

and the latter should be the height of the paragraph when the max width is set to the input width, using the current greedy line breaking algorithm.

hello world hello world

Your suggestion is changing the line break strategy (which skia txt only expose one).
In text widget, you wouldn't expect the breaking strategy to change if you give the same width but different height constraint though

IIRC, currently minIntrinsicWidth already does that (it pretends that we take every soft break opportunity)

minIntrinsicWidth is the same line break strategy in a shorter width.

Maybe we are talking about the same line-breaking strategy? What I meant was with minIntrinsicWidth the paragraph will be broken into as many lines as possible (https://github.com/google/skia/blob/85971b25cf4b94a4843f6a4e9faa6412b3d56d92/modules/skparagraph/src/TextWrapper.cpp#L434-L461).

I guess then the question is what would user expect when the use intrinsicheight with Text.

chunhtai · 2023-10-20T20:12:04Z

packages/flutter/test/rendering/editable_test.dart

        maxLines: 2,
        minLines: 2,
        textScaler: const TextScaler.linear(2.0),
        children: renderBoxes,


Can you write a test to verify the fix? I imagine editable text with widgetspan will be affect by this change

I added line 1636. It does fail without this patch. I'll add a few more for intrinsic heights as well.

chunhtai

LGTM

chunhtai · 2023-10-20T20:55:30Z

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

-  double computeMinIntrinsicHeight(double width) {
-    return _preferredHeight(width);
-  }
+  double computeMinIntrinsicHeight(double width) => computeMaxIntrinsicHeight(width);


calling the same method is expected because height will be fixed once you given a width.

flutter-dashboard · 2023-10-20T20:57:13Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #136979 at sha 532ed74

LongCatIsLooong · 2023-10-20T21:18:56Z

I'm not seeing the golden change locally. ~~Investigating.~~

Looks like the old golden image was correct on web. Accepted the golden change.

flutter/flutter@0883cb2...6f4850d 2023-10-21 [email protected] Roll Flutter Engine from a12116df3859 to 11f87dce1017 (1 revision) (flutter/flutter#137017) 2023-10-21 [email protected] Roll Flutter Engine from 755c477c96e7 to a12116df3859 (1 revision) (flutter/flutter#137013) 2023-10-21 [email protected] Roll Flutter Engine from 4f9f4013c3b3 to 755c477c96e7 (1 revision) (flutter/flutter#137008) 2023-10-21 [email protected] Test cover painting for memory leaks (flutter/flutter#136696) 2023-10-21 [email protected] Roll Flutter Engine from 2e5fb647a76f to 4f9f4013c3b3 (2 revisions) (flutter/flutter#136999) 2023-10-21 [email protected] Roll Flutter Engine from e4fc0c4e2b97 to 2e5fb647a76f (3 revisions) (flutter/flutter#136995) 2023-10-21 [email protected] Roll Flutter Engine from 9796817fb6e2 to e4fc0c4e2b97 (1 revision) (flutter/flutter#136993) 2023-10-20 [email protected] Roll Flutter Engine from 4a65910f0ab2 to 9796817fb6e2 (2 revisions) (flutter/flutter#136990) 2023-10-20 [email protected] Adds more a11y use cases (flutter/flutter#136761) 2023-10-20 [email protected] Reland "Use Layer.toImage for golden tests on CanvasKit" (flutter/flutter#136918) 2023-10-20 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.3 to 2.22.4 (flutter/flutter#136985) 2023-10-20 [email protected] Add missing call to parent method of GApplication::shutdown, implement GApplication::startup (flutter/flutter#136836) 2023-10-20 [email protected] Roll Flutter Engine from b27e1b38375b to 4a65910f0ab2 (5 revisions) (flutter/flutter#136987) 2023-10-20 [email protected] Bump dartdoc to 7.0.1 and clean-up (flutter/flutter#136975) 2023-10-20 [email protected] RenderEditable WidgetSpan intrinsics (flutter/flutter#136979) 2023-10-20 [email protected] Roll Flutter Engine from d46933e6772e to b27e1b38375b (9 revisions) (flutter/flutter#136982) 2023-10-20 [email protected] Roll pub packages (flutter/flutter#136924) 2023-10-20 [email protected] [docs] Expand categories for automated release notes (flutter/flutter#136967) 2023-10-20 [email protected] Roll Flutter Engine from 21b5c3771ee1 to d46933e6772e (3 revisions) (flutter/flutter#136973) 2023-10-20 [email protected] [Impeller] add OpenGL GPU tracing to devicelab tests. (flutter/flutter#136974) 2023-10-20 [email protected] Roll Flutter Engine from f9d1dbc39f4c to 21b5c3771ee1 (1 revision) (flutter/flutter#136969) 2023-10-20 [email protected] Default `NativeAssets` Darwin and IOS target archs if missing (flutter/flutter#136948) 2023-10-20 [email protected] Roll Flutter Engine from 6d1e9eec0841 to f9d1dbc39f4c (1 revision) (flutter/flutter#136966) 2023-10-20 [email protected] Roll Flutter Engine from 9b88ff83fd82 to 6d1e9eec0841 (2 revisions) (flutter/flutter#136965) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

WidgetSpan intrinsics

1134ef1

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 20, 2023

LongCatIsLooong requested review from chunhtai and goderbauer and removed request for goderbauer October 20, 2023 19:54

chunhtai reviewed Oct 20, 2023

View reviewed changes

more tests

532ed74

chunhtai approved these changes Oct 20, 2023

View reviewed changes

flutter-dashboard bot added the will affect goldens Changes to golden files label Oct 20, 2023

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

auto-submit bot merged commit 5f47764 into flutter:master Oct 20, 2023

LongCatIsLooong deleted the widgetspan-intrinsics branch October 20, 2023 21:34

engine-flutter-autoroll mentioned this pull request Oct 21, 2023

Roll Flutter from 0883cb2e962d to 025550f26ac9 (23 revisions) flutter/packages#5195

Closed

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

5f47764 RenderEditable WidgetSpan intrinsics (flutter/flutter#136979)

23095fc

engine-flutter-autoroll mentioned this pull request Oct 21, 2023

Roll Flutter from 0883cb2e962d to 6f4850d74d1a (24 revisions) flutter/packages#5196

Merged

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

5f47764 RenderEditable WidgetSpan intrinsics (flutter/flutter#136979)

dc789a8

HansMuller mentioned this pull request Nov 1, 2023

Flutter Framework Team: Notable Commits #121415

Closed

RenderEditable WidgetSpan intrinsics #136979

RenderEditable WidgetSpan intrinsics #136979

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

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 Oct 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Oct 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LongCatIsLooong commented Oct 20, 2023 •

edited

Loading

LongCatIsLooong Oct 20, 2023 •

edited

Loading

LongCatIsLooong commented Oct 20, 2023 •

edited

Loading