Skip to content

Conversation

@tvolkert
Copy link
Contributor

Currently, MaterialApp inserts a DefaultTextStyle in its hierarchy
with a style of _errorTextStyle. This means that any material app
with a Text widget that:
(a) doesn't have another DefaultTextStyle widget in its ancestry
between it and the MaterialApp, and
(b) specifies an explicit style with inherit: true
... is susceptible to having its styles inherit from the app-level
error styles. In this case, it's forced to insert a DefaultTextStyle
as a child of the MaterialApp, which is a little counter-intuitive.

This is exacerbated by the change in #12249, which changed the default
platform text themes in typography.dart to inherit: true. While this
change was correct, it makes apps more susceptible to the behavior above.
Consider the case of an app using a Text widget and explicitly setting
a style on that widget of a platform default style. In that case, they're
explicitly saying "I want this text to have style X" (e.g.
Typography.white.body1), and because the default platform text styles
don't specify decorations, those get inherited from the top-level error
text styles.

This change adds the ability to give MaterialApp a defaultTextStyle,
and only to use _errorTextStyles if such a default is unspecified.

Currently, MaterialApp inserts a `DefaultTextStyle` in its hierarchy
with a style of `_errorTextStyle`. This means that any material app
with a `Text` widget that:
(a) doesn't have another `DefaultTextStyle` widget in its ancestry
    between it and the `MaterialApp`, and
(b) specifies an explicit style with `inherit: true`
... is susceptible to having its styles inherit from the app-level
error styles.  In this case, it's forced to insert a `DefaultTextStyle`
as a *child* of the `MaterialApp`, which is a little counter-intuitive.

This is exacerbated by the change in #12249, which changed the default
platform text themes in `typography.dart` to `inherit: true`. While this
change was correct, it makes apps more susceptible to the behavior above.
Consider the case of an app using a `Text` widget and explicitly setting
a `style` on that widget of a platform default style. In that case, they're
*explicitly* saying "I want this text to have style X" (e.g.
`Typography.white.body1`), and because the default platform text styles
don't specify decorations, those get inherited from the top-level error
text styles.

This change adds the ability to give `MaterialApp` a `defaultTextStyle`,
and only to use `_errorTextStyles` if such a default is unspecified.
@Hixie
Copy link
Contributor

Hixie commented Oct 13, 2017

This is the wrong solution.

It's invalid to have text in a material app that isn't in a Material, and the Material introduces the right styles.

@Hixie
Copy link
Contributor

Hixie commented Oct 13, 2017

The right fix is probably to make the default text style be one that actually throws an exception when used, saying that you have to use a Material.

@tvolkert
Copy link
Contributor Author

ACK. Closing this, and I fixed the issue that was affecting the internal tests by fixing the tests to use Material correctly. thx.

@tvolkert tvolkert closed this Oct 13, 2017
@tvolkert tvolkert deleted the textstyle branch October 13, 2017 21:22
@Hixie
Copy link
Contributor

Hixie commented Oct 15, 2017

Having said that, #12548 is similar and interesting...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants