Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. #48346

GaryQian · 2020-01-07T21:18:33Z

Description

Adds support for TextHeightBehavior in Text, RichText, and DefaulttextStyle.

Depends on flutter/engine#15087.

Related Issues

Adds features to resolve #47175

Tests

Golden tests added. Additional tests in engine-side PR performs unit tests.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

No, no existing tests failed, so this is not a breaking change.

fluttergithubbot · 2020-01-07T21:18:36Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

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

GaryQian · 2020-01-23T23:30:52Z

Tests will fail until manual roll of engine with engine-side landed.

justinmc

LGTM 👍

justinmc · 2020-01-23T23:57:01Z

packages/flutter/lib/painting.dart

@@ -17,7 +17,7 @@
 ///    painting boxes.
 library painting;

-export 'dart:ui' show Shadow, PlaceholderAlignment;
+export 'dart:ui' show Shadow, PlaceholderAlignment, TextHeightBehavior;


I think you meant to remove TextHeightBehavior here.

I am trying to expose this as part of the painting library so that users do not have to import dart:ui to use this. Does that make sense?

Oh right sorry, I see. I guess I don't see why not 👍

GaryQian · 2020-01-27T22:41:13Z

In manual roll process. Engine autoroller is stopped until this lands.

dnfield · 2020-01-28T17:37:39Z

@GaryQian - CI is still failing on this PR because it's missing some changes in tests that assert on ParagraphStyle.toString

I'm going to revert the upstream change in the engine and re-enable the roller - autorollers are getting behind and it's an issue for the Skia team.

dnfield · 2020-01-28T17:40:58Z

Also, I'm pretty sure this will have to count as a breaking change... Not sure how we count things like toString changing, but there's at least one golden failure in here.

GaryQian · 2020-01-28T19:01:27Z

The golden failure is a new golden test.

The toString test failure was a typo on my part, these particular tests for web don't run on engine until we roll into framework.

GaryQian · 2020-01-29T22:39:31Z

Will let #49742 land first, which will remove need for an engine roll to be in the PR.

slightfoot · 2020-02-11T20:37:02Z

Just received a crash from then when I removed textHeightBehavior property from a Text widget and performed a hot-reload.

I/flutter (19810): ══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
I/flutter (19810): The following assertion was thrown while rebuilding dirty elements:
I/flutter (19810): 'package:flutter/src/painting/text_painter.dart': Failed assertion: line 349 pos 12: 'value !=
I/flutter (19810): null': is not true.
I/flutter (19810): 
I/flutter (19810): Either the assertion indicates an error in the framework itself, or we should provide substantially
I/flutter (19810): more information in this error message to help you determine and fix the underlying cause.
I/flutter (19810): In either case, please report this assertion by filing a bug on GitHub:
I/flutter (19810):   https://github.com/flutter/flutter/issues/new?template=BUG.md
I/flutter (19810): 
I/flutter (19810): The relevant error-causing widget was:
I/flutter (19810):   Text file:///######################## REDACTED ##################################
I/flutter (19810): 
I/flutter (19810): When the exception was thrown, this was the stack:
I/flutter (19810): #2      TextPainter.textHeightBehavior= (package:flutter/src/painting/text_painter.dart:349:12)
I/flutter (19810): #3      RenderParagraph.textHeightBehavior= (package:flutter/src/rendering/paragraph.dart:285:18)
I/flutter (19810): #4      RichText.updateRenderObject (package:flutter/src/widgets/basic.dart:5186:9)

Checked the source and you have a assertion for null in the setter in TextPaitner, even though null in is a perfectly valid value for this variable. In fact the constructors default value is to have this variable null.

GaryQian · 2020-02-11T21:41:45Z

Thanks for the catch, let me see if I can get this sorted quickly!

GaryQian · 2020-02-11T21:49:42Z

I just posted a quick PR to fix this. Thanks for bringing this to my attention! #50603

chris-rutkowski · 2020-04-07T04:01:26Z

Hey, I don't see this property in any theme. Are we expected to add it to every Text widget?

GaryQian · 2020-04-07T10:57:36Z

It should be set in DefaultTextStyle.

GaryQian added c: new feature Nothing broken; request for a new capability a: typography Text rendering, possibly libtxt customer: google Various Google teams labels Jan 7, 2020

GaryQian self-assigned this Jan 7, 2020

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 7, 2020

googlebot added the cla: yes label Jan 7, 2020

GaryQian added the work in progress; do not review label Jan 7, 2020

GaryQian mentioned this pull request Jan 7, 2020

Engine/LibTxt/dart:ui impl of TextHeightBehavior flutter/engine#15087

Merged

GaryQian changed the title ~~Expose BoundaryLineHeightBehavior in Text and RichText.~~ Expose HeightBehavior in Text, RichText, and DefaultTextStyle. Jan 9, 2020

GaryQian mentioned this pull request Jan 10, 2020

[Text] Allow for line height *between* lines on text. #47175

Closed

GaryQian removed the work in progress; do not review label Jan 15, 2020

GaryQian force-pushed the lineheight branch from 6f440b4 to 3a9e6b1 Compare January 23, 2020 01:38

fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Jan 23, 2020

GaryQian requested review from justinmc and HansMuller January 23, 2020 23:21

justinmc approved these changes Jan 24, 2020

View reviewed changes

GaryQian force-pushed the lineheight branch from 8a7d729 to c77404d Compare January 27, 2020 21:44

GaryQian changed the title ~~Expose HeightBehavior in Text, RichText, and DefaultTextStyle.~~ Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. Jan 27, 2020

fluttergithubbot added the engine flutter/engine repository. See also e: labels. label Jan 27, 2020

GaryQian force-pushed the lineheight branch 2 times, most recently from e548dfe to f735a0a Compare January 29, 2020 21:01

GaryQian added 3 commits January 29, 2020 13:02

Integration with text and RichText

0244a03

Expose dart:ui BoundaryLineHeightBehavior in painting lib

ba44556

Add boundaryLineHeightBehavior to DefaultTextStyle

8cbdcf3

GaryQian added 8 commits January 29, 2020 13:02

Rename

51e53be

Add template macros for docs

2c6dec5

TextPainter impl, tests

505129a

Rename HeightBehavior -> TextHeightBehavior

7e2c70c

Fix tests

e88a225

Roll engine

3fe8588

Fix analyzer

ef597d7

Merge

833814f

GaryQian force-pushed the lineheight branch from f735a0a to 833814f Compare January 29, 2020 21:02

Merge branch 'master' into lineheight

b542c49

GaryQian added the waiting for tree to go green label Jan 30, 2020

GaryQian merged commit 52df599 into flutter:master Jan 30, 2020

GaryQian mentioned this pull request Feb 11, 2020

Remove invalid assert for TextHeightBehavior #50603

Merged

This was referenced Feb 28, 2020

IOS 13.3 TextStyle FontWeight being completely Ignored #49492

Closed

Flutter v1.15.0 marcglasberg/assorted_layout_widgets#2

Closed

ryan-berger mentioned this pull request Jun 11, 2020

Accessibility and scaling Sub6Resources/flutter_html#308

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. #48346

Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. #48346

GaryQian commented Jan 7, 2020 •

edited

Loading

fluttergithubbot commented Jan 7, 2020

GaryQian commented Jan 23, 2020

justinmc left a comment

justinmc Jan 23, 2020

GaryQian Jan 24, 2020

justinmc Jan 24, 2020

GaryQian commented Jan 27, 2020

dnfield commented Jan 28, 2020

dnfield commented Jan 28, 2020

GaryQian commented Jan 28, 2020 •

edited

Loading

GaryQian commented Jan 29, 2020

slightfoot commented Feb 11, 2020

GaryQian commented Feb 11, 2020

GaryQian commented Feb 11, 2020 •

edited

Loading

chris-rutkowski commented Apr 7, 2020

GaryQian commented Apr 7, 2020

Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. #48346

Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. #48346

Conversation

GaryQian commented Jan 7, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

fluttergithubbot commented Jan 7, 2020

GaryQian commented Jan 23, 2020

justinmc left a comment

Choose a reason for hiding this comment

justinmc Jan 23, 2020

Choose a reason for hiding this comment

GaryQian Jan 24, 2020

Choose a reason for hiding this comment

justinmc Jan 24, 2020

Choose a reason for hiding this comment

GaryQian commented Jan 27, 2020

dnfield commented Jan 28, 2020

dnfield commented Jan 28, 2020

GaryQian commented Jan 28, 2020 • edited Loading

GaryQian commented Jan 29, 2020

slightfoot commented Feb 11, 2020

GaryQian commented Feb 11, 2020

GaryQian commented Feb 11, 2020 • edited Loading

chris-rutkowski commented Apr 7, 2020

GaryQian commented Apr 7, 2020

GaryQian commented Jan 7, 2020 •

edited

Loading

GaryQian commented Jan 28, 2020 •

edited

Loading

GaryQian commented Feb 11, 2020 •

edited

Loading