Defer systemFontsDidChange to the transientCallbacks phase (#117123)

* Always defer systemFontsDidChange to transientCallbacks phase * unnecessary import
engine-flutter-autoroll · Dec 16, 2022 · 3d0607b · 3d0607b
1 parent 9102f2f
commit 3d0607b
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 100 deletions.
diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart
@@ -10,6 +10,7 @@ import 'package:flutter/animation.dart';
 import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart';
 import 'package:flutter/painting.dart';
+import 'package:flutter/scheduler.dart';
 import 'package:flutter/semantics.dart';
 
 import 'debug.dart';
@@ -4011,13 +4012,18 @@ mixin ContainerRenderObjectMixin<ChildType extends RenderObject, ParentDataType
 /// Mixin for [RenderObject] that will call [systemFontsDidChange] whenever the
 /// system fonts change.
 ///
-/// System fonts can change when the OS install or remove a font. Use this mixin if
-/// the [RenderObject] uses [TextPainter] or [Paragraph] to correctly update the
-/// text when it happens.
+/// System fonts can change when the OS installs or removes a font. Use this
+/// mixin if the [RenderObject] uses [TextPainter] or [Paragraph] to correctly
+/// update the text when it happens.
 mixin RelayoutWhenSystemFontsChangeMixin on RenderObject {
 
   /// A callback that is called when system fonts have changed.
   ///
+  /// The framework defers the invocation of the callback to the
+  /// [SchedulerPhase.transientCallbacks] phase to ensure that the
+  /// [RenderObject]'s text layout is still valid when user interactions are in
+  /// progress (which usually take place during the [SchedulerPhase.idle] phase).
+  ///
   /// By default, [markNeedsLayout] is called on the [RenderObject]
   /// implementing this mixin.
   ///
@@ -4029,15 +4035,44 @@ mixin RelayoutWhenSystemFontsChangeMixin on RenderObject {
     markNeedsLayout();
   }
 
+  bool _hasPendingSystemFontsDidChangeCallBack = false;
+  void _scheduleSystemFontsUpdate() {
+    assert(
+      SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idle,
+      '${objectRuntimeType(this, "RelayoutWhenSystemFontsChangeMixin")}._scheduleSystemFontsUpdate() '
+      'called during ${SchedulerBinding.instance.schedulerPhase}.',
+    );
+    if (_hasPendingSystemFontsDidChangeCallBack) {
+      return;
+    }
+    _hasPendingSystemFontsDidChangeCallBack = true;
+    SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {
+      assert(_hasPendingSystemFontsDidChangeCallBack);
+      _hasPendingSystemFontsDidChangeCallBack = false;
+      assert(
+        attached || (debugDisposed ?? true),
+        '$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.',
+      );
+      if (attached) {
+        systemFontsDidChange();
+      }
+    });
+  }
+
   @override
-  void attach(covariant PipelineOwner owner) {
+  void attach(PipelineOwner owner) {
     super.attach(owner);
-    PaintingBinding.instance.systemFonts.addListener(systemFontsDidChange);
+    // If there's a pending callback that would imply this node was detached
+    // between the idle phase and the next transientCallbacks phase. The tree
+    // can not be mutated between those two phases so that should never happen.
+    assert(!_hasPendingSystemFontsDidChangeCallBack);
+    PaintingBinding.instance.systemFonts.addListener(_scheduleSystemFontsUpdate);
   }
 
   @override
   void detach() {
-    PaintingBinding.instance.systemFonts.removeListener(systemFontsDidChange);
+    assert(!_hasPendingSystemFontsDidChangeCallBack);
+    PaintingBinding.instance.systemFonts.removeListener(_scheduleSystemFontsUpdate);
     super.detach();
   }
 }

diff --git a/packages/flutter/lib/src/rendering/paragraph.dart b/packages/flutter/lib/src/rendering/paragraph.dart
@@ -8,7 +8,6 @@ import 'dart:ui' as ui show BoxHeightStyle, BoxWidthStyle, Gradient, LineMetrics
 
 import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart';
-import 'package:flutter/scheduler.dart';
 import 'package:flutter/semantics.dart';
 import 'package:flutter/services.dart';
 
@@ -654,37 +653,10 @@ class RenderParagraph extends RenderBox
     );
   }
 
-  bool _systemFontsChangeScheduled = false;
   @override
   void systemFontsDidChange() {
-    final SchedulerPhase phase = SchedulerBinding.instance.schedulerPhase;
-    switch (phase) {
-      case SchedulerPhase.idle:
-      case SchedulerPhase.postFrameCallbacks:
-        if (_systemFontsChangeScheduled) {
-          return;
-        }
-        _systemFontsChangeScheduled = true;
-        SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {
-          assert(_systemFontsChangeScheduled);
-          _systemFontsChangeScheduled = false;
-          assert(
-            attached || (debugDisposed ?? true),
-            '$this is detached during $phase but not disposed.',
-          );
-          if (attached) {
-            super.systemFontsDidChange();
-            _textPainter.markNeedsLayout();
-          }
-        });
-        break;
-      case SchedulerPhase.transientCallbacks:
-      case SchedulerPhase.midFrameMicrotasks:
-      case SchedulerPhase.persistentCallbacks:
-        super.systemFontsDidChange();
-        _textPainter.markNeedsLayout();
-        break;
-    }
+    super.systemFontsDidChange();
+    _textPainter.markNeedsLayout();
   }
 
   // Placeholder dimensions representing the sizes of child inline widgets.

diff --git a/packages/flutter/test/painting/system_fonts_test.dart b/packages/flutter/test/painting/system_fonts_test.dart
@@ -11,74 +11,72 @@ import 'package:flutter/rendering.dart';
 import 'package:flutter/services.dart';
 import 'package:flutter_test/flutter_test.dart';
 
-void main() {
-  testWidgets('RenderParagraph relayout upon system fonts changes', (WidgetTester tester) async {
-    await tester.pumpWidget(
-      const MaterialApp(
-        home: Text('text widget'),
-      ),
-    );
-    final RenderObject renderObject = tester.renderObject(find.text('text widget'));
+Future<void> verifyMarkedNeedsLayoutDuringTransientCallbacksPhase(WidgetTester tester, RenderObject renderObject) async {
+  assert(!renderObject.debugNeedsLayout);
 
-    const Map<String, dynamic> data = <String, dynamic>{
-      'type': 'fontsChange',
-    };
-    await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
-      'flutter/system',
-      SystemChannels.system.codec.encodeMessage(data),
-      (ByteData? data) { },
-    );
+  const Map<String, dynamic> data = <String, dynamic>{
+    'type': 'fontsChange',
+  };
+  await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
+    'flutter/system',
+    SystemChannels.system.codec.encodeMessage(data),
+    (ByteData? data) { },
+  );
 
-    final Completer<bool> animation = Completer<bool>();
-    tester.binding.scheduleFrameCallback((Duration timeStamp) {
-      animation.complete(renderObject.debugNeedsLayout);
-    });
-    expect(renderObject.debugNeedsLayout, isFalse);
-    await tester.pump();
-    expect(await animation.future, isTrue);
+  final Completer<bool> animation = Completer<bool>();
+  tester.binding.scheduleFrameCallback((Duration timeStamp) {
+    animation.complete(renderObject.debugNeedsLayout);
   });
 
-  testWidgets('Safe to query RenderParagraph for text layout after system fonts changes', (WidgetTester tester) async {
+  // The fonts change does not mark the render object as needing layout
+  // immediately.
+  expect(renderObject.debugNeedsLayout, isFalse);
+  await tester.pump();
+  expect(await animation.future, isTrue);
+}
+
+void main() {
+  testWidgets('RenderParagraph relayout upon system fonts changes', (WidgetTester tester) async {
     await tester.pumpWidget(
       const MaterialApp(
         home: Text('text widget'),
       ),
     );
-    const Map<String, dynamic> data = <String, dynamic>{
-      'type': 'fontsChange',
-    };
-    await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
-      'flutter/system',
-      SystemChannels.system.codec.encodeMessage(data),
-      (ByteData? data) { },
-    );
-    final RenderParagraph paragraph = tester.renderObject<RenderParagraph>(find.text('text widget'));
-    Object? exception;
-    try {
-      paragraph.getPositionForOffset(Offset.zero);
-      paragraph.hitTest(BoxHitTestResult(), position: Offset.zero);
-    } catch (e) {
-      exception = e;
-    }
-    expect(exception, isNull);
+    final RenderObject renderObject = tester.renderObject(find.text('text widget'));
+    await verifyMarkedNeedsLayoutDuringTransientCallbacksPhase(tester, renderObject);
   });
 
+  testWidgets(
+    'Safe to query a RelayoutWhenSystemFontsChangeMixin for text layout after system fonts changes',
+    (WidgetTester tester) async {
+      final _RenderCustomRelayoutWhenSystemFontsChange child = _RenderCustomRelayoutWhenSystemFontsChange();
+      await tester.pumpWidget(
+        Directionality(
+          textDirection: TextDirection.ltr,
+          child: WidgetToRenderBoxAdapter(renderBox: child),
+        ),
+      );
+      const Map<String, dynamic> data = <String, dynamic>{
+        'type': 'fontsChange',
+      };
+      await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
+        'flutter/system',
+        SystemChannels.system.codec.encodeMessage(data),
+        (ByteData? data) { },
+      );
+      expect(child.hasValidTextLayout, isTrue);
+    },
+  );
+
   testWidgets('RenderEditable relayout upon system fonts changes', (WidgetTester tester) async {
     await tester.pumpWidget(
       const MaterialApp(
         home: SelectableText('text widget'),
       ),
     );
-    const Map<String, dynamic> data = <String, dynamic>{
-      'type': 'fontsChange',
-    };
-    await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
-      'flutter/system',
-      SystemChannels.system.codec.encodeMessage(data),
-        (ByteData? data) { },
-    );
+
     final EditableTextState state = tester.state(find.byType(EditableText));
-    expect(state.renderEditable.debugNeedsLayout, isTrue);
+    await verifyMarkedNeedsLayoutDuringTransientCallbacksPhase(tester, state.renderEditable);
   });
 
   testWidgets('Banner repaint upon system fonts changes', (WidgetTester tester) async {
@@ -210,11 +208,8 @@ void main() {
       SystemChannels.system.codec.encodeMessage(data),
         (ByteData? data) { },
     );
-    final RenderObject renderObject = tester.renderObject(find.byType(RangeSlider));
-
-    late bool sliderBoxNeedsLayout;
-    renderObject.visitChildren((RenderObject child) {sliderBoxNeedsLayout = child.debugNeedsLayout;});
-    expect(sliderBoxNeedsLayout, isTrue);
+    final RenderObject renderObject = tester.renderObject(find.byWidgetPredicate((Widget widget) => widget.runtimeType.toString() == '_RangeSliderRenderObjectWidget'));
+    await verifyMarkedNeedsLayoutDuringTransientCallbacksPhase(tester, renderObject);
   });
 
   testWidgets('Slider relayout upon system fonts changes', (WidgetTester tester) async {
@@ -228,17 +223,9 @@ void main() {
         ),
       ),
     );
-    const Map<String, dynamic> data = <String, dynamic>{
-      'type': 'fontsChange',
-    };
-    await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage(
-      'flutter/system',
-      SystemChannels.system.codec.encodeMessage(data),
-        (ByteData? data) { },
-    );
     // _RenderSlider is the last render object in the tree.
     final RenderObject renderObject = tester.allRenderObjects.last;
-    expect(renderObject.debugNeedsLayout, isTrue);
+    await verifyMarkedNeedsLayoutDuringTransientCallbacksPhase(tester, renderObject);
   });
 
   testWidgets('TimePicker relayout upon system fonts changes', (WidgetTester tester) async {
@@ -289,3 +276,19 @@ void main() {
     expect(renderObject.debugNeedsPaint, isTrue);
   });
 }
+
+class _RenderCustomRelayoutWhenSystemFontsChange extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
+  bool hasValidTextLayout = false;
+
+  @override
+  void systemFontsDidChange() {
+    super.systemFontsDidChange();
+    hasValidTextLayout = false;
+  }
+
+  @override
+  void performLayout() {
+    size = constraints.biggest;
+    hasValidTextLayout = true;
+  }
+}