Listen to text spacing overrides on the web#178081
Conversation
This commit squashes 114 commits from the typography-overrides-squash branch. The changes include updates to StrutStyle, EditableText, MediaQuery, and other text-related widgets. It also includes changes to the platform dispatcher.
There was a problem hiding this comment.
Code Review
This pull request introduces typography overrides from the platform, allowing for better accessibility and consistency with system text settings. The changes span across the engine and framework, adding new properties to PlatformDispatcher and MediaQueryData, and applying them in Text, EditableText, and SelectableText widgets. The web engine implementation cleverly uses a ResizeObserver to detect CSS changes. My review has identified a critical issue in the web implementation that could lead to performance problems, a high-severity bug in a copyWith method, and a couple of medium-severity issues related to code correctness and duplication. Overall, this is a great feature addition, and with these fixes, it will be solid.
|
Changes since last review:
|
mdebbar
left a comment
There was a problem hiding this comment.
web side looks good to me!
|
|
||
| spellCheckResults = SpellCheckResults(text, suggestions); | ||
| renderEditable.text = buildTextSpan(); | ||
| final double? lineHeightScaleFactor = mounted |
There was a problem hiding this comment.
why's the mounted check needed?
There was a problem hiding this comment.
Because this logic is happening in an async method, the lint complained we were not checking mounted. https://dart.dev/tools/linter-rules/use_build_context_synchronously
There was a problem hiding this comment.
On line 4476 we return if suggestions == null. Could we update that to return also if !mounted, and then simplify this logic? I don't feel strongly, just a thought.
There was a problem hiding this comment.
I think we should not exit early if suggestions != null and !mounted in that case we still want the spell check to work and that should continue to work as expected as it didn't access the context before.
suggestions == null || !mounted - returns early even when spell check might be expected
There was a problem hiding this comment.
Uhm the state should stop doing spell checking and stop receiving spell checking updates as soon as it's disposed so there should be no need for the mount check? Do we have that lint enabled in the framework?
There was a problem hiding this comment.
Yeah this lint is currently enabled in the framework
Line 249 in e74c595
There was a problem hiding this comment.
On line 4476 we return if suggestions == null. Could we update that to return also if !mounted, and then simplify this logic? I don't feel strongly, just a thought.
I ended up going with this suggestion. It makes sense that we don't spell check at all while unmounted.
| // TODO(Renzo-Olivares): Remove after investigating a solution for overriding all | ||
| // styles for children in an [InlineSpan] tree, see: https://github.com/flutter/flutter/issues/177952. | ||
| class _OverridingTextStyleTextSpanUtils { | ||
| static TextSpan applyTextSpacingOverrides({ |
There was a problem hiding this comment.
nit: maybe remove the utility class an expose the functions instead?
There was a problem hiding this comment.
I think I like the class because i'm able to place the TODO on the entire class. Are there any benefits to placing them directly in Text as functions?
| /// supported. | ||
| final bool supportsShowingSystemContextMenu; | ||
|
|
||
| /// The height of the text, as a multiple of the font size. |
There was a problem hiding this comment.
nit: maybe: text override, same for the other two fields
There was a problem hiding this comment.
Updated to Overrides the height of the text...
| gestureSettings, | ||
| Object.hashAll(displayFeatures), | ||
| supportsShowingSystemContextMenu, | ||
| Object.hash( |
There was a problem hiding this comment.
why are these 4 grouped together?
There was a problem hiding this comment.
We have exceeded the 20 objects that this method takes, so I ended up grouping them. Should I do something else instead in this case?
| required double? wordSpacingOverride, | ||
| required double? paragraphSpacingOverride, | ||
| required super.child, | ||
| }) : data = MediaQuery.of(context).applyTextStyleOverrides( |
There was a problem hiding this comment.
This looks error-prone. data is evaluated on construction so if the user caches this widget this doesn't get rebuilt even if the ancestor media query changes.
There was a problem hiding this comment.
I'd just wrap this in a builder, and you don't have to take context if you do that.
There was a problem hiding this comment.
Or move this to MediaQueryData.
There was a problem hiding this comment.
Oh good catch. +1 to wrapping this in a builder and removing the context argument.
There was a problem hiding this comment.
Went with the Builder suggestion. Thank you!
| wordSpacingOverride: 9.0, | ||
| paragraphSpacingOverride: 9.0, | ||
| ), | ||
| child: Builder( |
There was a problem hiding this comment.
If you switch to Weiyu's suggestion of adding a Builder in MediaQuery.applyTextStyleOverrides, don't forget to remove the Builders from these tests :)
There was a problem hiding this comment.
Good call, I did end up going with the Builder suggestion.
Original PR/Discussion: flutter#172915 # Framework: * `EditableText`/`SelectableText`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. Note `SelectableText` is built on `EditableText` so it also applies these overrides. * `Text`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. * Exposes line height override through `MediaQueryData.lineHeightScaleFactorOverride` and `maybeLineHeightScaleFactorOverrideOf(context)`. * Exposes letter spacing override through `MediaQueryData.letterSpacingOverride` and `maybeLetterSpacingOverrideOf(context)`. * Exposes word spacing override through `MediaQueryData.wordSpacingOverride` and `maybeWordSpacingOverrideOf(context)`. * Exposes paragraph spacing override through `MediaQueryData.paragraphSpacingOverride` and `maybeParagraphSpacingOverrideOf(context)`. * `MediaQuery.applyTextStyleOverrides()` \ `MediaQueryData.applyTextStyleOverrides()` to be able to reset/override the text spacing settings on `MediaQueryData`. # Engine: * Introduces new members on `PlatformDispatcher` API that hold the text spacing properties that are overridden on the web. * We provide the `lineHeightScaleFactorOverride`, `letterSpacingOverride`, `wordSpacingOverride`, and `paragraphSpacingOverride` on the web by attaching a `ResizeObserver` to an off-screen hidden element, when its size changes we capture its text spacing CSS properties, and notify the framework through `onMetricsChanged`. Fixes flutter#142712 https://github.com/user-attachments/assets/aaaa3e74-c232-4956-acd2-ae1a4487e415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <roliv@google.com>
Original PR/Discussion: flutter#172915 # Framework: * `EditableText`/`SelectableText`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. Note `SelectableText` is built on `EditableText` so it also applies these overrides. * `Text`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. * Exposes line height override through `MediaQueryData.lineHeightScaleFactorOverride` and `maybeLineHeightScaleFactorOverrideOf(context)`. * Exposes letter spacing override through `MediaQueryData.letterSpacingOverride` and `maybeLetterSpacingOverrideOf(context)`. * Exposes word spacing override through `MediaQueryData.wordSpacingOverride` and `maybeWordSpacingOverrideOf(context)`. * Exposes paragraph spacing override through `MediaQueryData.paragraphSpacingOverride` and `maybeParagraphSpacingOverrideOf(context)`. * `MediaQuery.applyTextStyleOverrides()` \ `MediaQueryData.applyTextStyleOverrides()` to be able to reset/override the text spacing settings on `MediaQueryData`. # Engine: * Introduces new members on `PlatformDispatcher` API that hold the text spacing properties that are overridden on the web. * We provide the `lineHeightScaleFactorOverride`, `letterSpacingOverride`, `wordSpacingOverride`, and `paragraphSpacingOverride` on the web by attaching a `ResizeObserver` to an off-screen hidden element, when its size changes we capture its text spacing CSS properties, and notify the framework through `onMetricsChanged`. Fixes flutter#142712 https://github.com/user-attachments/assets/aaaa3e74-c232-4956-acd2-ae1a4487e415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <roliv@google.com>
Original PR/Discussion: flutter#172915 # Framework: * `EditableText`/`SelectableText`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. Note `SelectableText` is built on `EditableText` so it also applies these overrides. * `Text`, applies `lineHeightScaleFactorOverride`, `wordSpacingOverride`, and `letterSpacingOverride` to it's `TextStyle` similarly to how we already do for bold platform overrides. * Exposes line height override through `MediaQueryData.lineHeightScaleFactorOverride` and `maybeLineHeightScaleFactorOverrideOf(context)`. * Exposes letter spacing override through `MediaQueryData.letterSpacingOverride` and `maybeLetterSpacingOverrideOf(context)`. * Exposes word spacing override through `MediaQueryData.wordSpacingOverride` and `maybeWordSpacingOverrideOf(context)`. * Exposes paragraph spacing override through `MediaQueryData.paragraphSpacingOverride` and `maybeParagraphSpacingOverrideOf(context)`. * `MediaQuery.applyTextStyleOverrides()` \ `MediaQueryData.applyTextStyleOverrides()` to be able to reset/override the text spacing settings on `MediaQueryData`. # Engine: * Introduces new members on `PlatformDispatcher` API that hold the text spacing properties that are overridden on the web. * We provide the `lineHeightScaleFactorOverride`, `letterSpacingOverride`, `wordSpacingOverride`, and `paragraphSpacingOverride` on the web by attaching a `ResizeObserver` to an off-screen hidden element, when its size changes we capture its text spacing CSS properties, and notify the framework through `onMetricsChanged`. Fixes flutter#142712 https://github.com/user-attachments/assets/aaaa3e74-c232-4956-acd2-ae1a4487e415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <roliv@google.com>
Original PR/Discussion: #172915
Framework:
EditableText/SelectableText, applieslineHeightScaleFactorOverride,wordSpacingOverride, andletterSpacingOverrideto it'sTextStylesimilarly to how we already do for bold platform overrides. NoteSelectableTextis built onEditableTextso it also applies these overrides.Text, applieslineHeightScaleFactorOverride,wordSpacingOverride, andletterSpacingOverrideto it'sTextStylesimilarly to how we already do for bold platform overrides.MediaQueryData.lineHeightScaleFactorOverrideandmaybeLineHeightScaleFactorOverrideOf(context).MediaQueryData.letterSpacingOverrideandmaybeLetterSpacingOverrideOf(context).MediaQueryData.wordSpacingOverrideandmaybeWordSpacingOverrideOf(context).MediaQueryData.paragraphSpacingOverrideandmaybeParagraphSpacingOverrideOf(context).MediaQuery.applyTextStyleOverrides()\MediaQueryData.applyTextStyleOverrides()to be able to reset/override the text spacing settings onMediaQueryData.Engine:
PlatformDispatcherAPI that hold the text spacing properties that are overridden on the web.lineHeightScaleFactorOverride,letterSpacingOverride,wordSpacingOverride, andparagraphSpacingOverrideon the web by attaching aResizeObserverto an off-screen hidden element, when its size changes we capture its text spacing CSS properties, and notify the framework throughonMetricsChanged.Fixes #142712
Screen.Recording.2025-07-30.at.2.50.37.PM.mov
Pre-launch Checklist
///).