Skip to content

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

Merged
GaryQian merged 12 commits intoflutter:masterfrom
GaryQian:lineheight
Jan 30, 2020
Merged

Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle.#48346
GaryQian merged 12 commits intoflutter:masterfrom
GaryQian:lineheight

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Jan 7, 2020 •
edited
Loading

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.

@GaryQian 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 GaryQian self-assigned this Jan 7, 2020
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jan 7, 2020

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.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 7, 2020
@GaryQian GaryQian mentioned this pull request Jan 7, 2020
Merged
@GaryQian GaryQian changed the title Expose BoundaryLineHeightBehavior in Text and RichText. Expose HeightBehavior in Text, RichText, and DefaultTextStyle. Jan 9, 2020
@GaryQian GaryQian mentioned this pull request Jan 10, 2020
Closed
@fluttergithubbot fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Jan 23, 2020
@GaryQian
Copy link
Contributor Author

GaryQian commented Jan 23, 2020

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

justinmc
justinmc approved these changes Jan 24, 2020
View reviewed changes
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 👍

packages/flutter/lib/painting.dart Outdated
Copy link
Contributor

@justinmc justinmc Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to remove TextHeightBehavior here.

Copy link
Contributor Author

@GaryQian GaryQian Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@justinmc justinmc Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@GaryQian GaryQian changed the title Expose HeightBehavior in Text, RichText, and DefaultTextStyle. Expose TextHeightBehavior in Text, RichText, and DefaultTextStyle. Jan 27, 2020
@fluttergithubbot fluttergithubbot added the engine flutter/engine related. See also e: labels. label Jan 27, 2020
@GaryQian
Copy link
Contributor Author

GaryQian commented Jan 27, 2020

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

@dnfield
Copy link
Contributor

dnfield commented Jan 28, 2020

@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
Copy link
Contributor

dnfield commented Jan 28, 2020

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
Copy link
Contributor Author

GaryQian commented Jan 28, 2020 •
edited
Loading

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 GaryQian force-pushed the lineheight branch 2 times, most recently from e548dfe to f735a0a Compare January 29, 2020 21:01
@GaryQian
Copy link
Contributor Author

GaryQian commented Jan 29, 2020

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

@GaryQian GaryQian merged commit 52df599 into flutter:master Jan 30, 2020
@slightfoot
Copy link
Member

slightfoot commented Feb 11, 2020

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
Copy link
Contributor Author

GaryQian commented Feb 11, 2020

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

@GaryQian GaryQian mentioned this pull request Feb 11, 2020
Merged
@GaryQian
Copy link
Contributor Author

GaryQian commented Feb 11, 2020 •
edited
Loading

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

This was referenced Feb 28, 2020
Closed
Closed
@chris-rutkowski
Copy link

chris-rutkowski commented Apr 7, 2020

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

@GaryQian
Copy link
Contributor Author

GaryQian commented Apr 7, 2020

It should be set in DefaultTextStyle.

@ryan-berger ryan-berger mentioned this pull request Jun 11, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justinmc justinmc justinmc approved these changes

@HansMuller HansMuller Awaiting requested review from HansMuller

Assignees

@GaryQian GaryQian

Labels

a: typography Text rendering, possibly libtxt c: new feature Nothing broken; request for a new capability customer: google Various Google teams engine flutter/engine related. See also e: labels. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

7 participants

@GaryQian @fluttergithubbot @dnfield @slightfoot @chris-rutkowski @justinmc @googlebot