Skip to content

Commit d1c0af9

Browse files
matthew-carrollmatthew-carroll
committed
[SuperEditor][Bug] - Fix inherited widget bug in MessagePageScaffold, fix toolbar bug in KeyboardPanelScaffold, fix pipeline timing issue with overlayController.show() (Resolves #2795) (#2796)
1 parent 53781d6 commit d1c0af9

File tree

10 files changed

+174
-42
lines changed

10 files changed

+174
-42
lines changed

‎super_editor/example/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
ignoresPersistentStateOnLaunch = "NO"
4949
debugDocumentVersioning = "YES"
5050
debugServiceExtension = "internal"
51+
enableGPUValidationMode = "1"
5152
allowLocationSimulation = "YES">
5253
<BuildableProductRunnable
5354
runnableDebuggingMode = "0">

‎super_editor/example_chat/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
buildConfiguration = "Debug"
2727
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
2828
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
29+
customLLDBInitFile = "$(SRCROOT)/Flutter/ephemeral/flutter_lldbinit"
2930
shouldUseLaunchSchemeArgsEnv = "YES">
3031
<MacroExpansion>
3132
<BuildableReference
@@ -54,11 +55,13 @@
5455
buildConfiguration = "Debug"
5556
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
5657
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
58+
customLLDBInitFile = "$(SRCROOT)/Flutter/ephemeral/flutter_lldbinit"
5759
launchStyle = "0"
5860
useCustomWorkingDirectory = "NO"
5961
ignoresPersistentStateOnLaunch = "NO"
6062
debugDocumentVersioning = "YES"
6163
debugServiceExtension = "internal"
64+
enableGPUValidationMode = "1"
6265
allowLocationSimulation = "YES">
6366
<BuildableProductRunnable
6467
runnableDebuggingMode = "0">

‎super_editor/example_chat/lib/message_page_scaffold_demo/demo_super_editor_message_page.dart‎

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,16 @@ class _ChatThread extends StatelessWidget {
8181
// message appears at the bottom, and you want to retain the
8282
// scroll offset near the newest messages, not the oldest.
8383
itemBuilder: (context, index) {
84+
if (index == 8) {
85+
// Arbitrarily placed text field to test moving focus between a non-editor
86+
// and the editor.
87+
return TextField(
88+
decoration: InputDecoration(
89+
hintText: "Content text field...",
90+
),
91+
);
92+
}
93+
8494
return Padding(
8595
padding: const EdgeInsets.symmetric(vertical: 4),
8696
child: Material(
@@ -121,12 +131,6 @@ class _EditorBottomSheetState extends State<_EditorBottomSheet> {
121131
void initState() {
122132
super.initState();
123133

124-
print("Editor bottom sheet scroll controller: ${_scrollController.hashCode}");
125-
_scrollController.addListener(() {
126-
print(
127-
"Scroll controller change - scroll offset: ${_scrollController.offset}, max scroll: ${_scrollController.position.maxScrollExtent}");
128-
});
129-
130134
_editor = createDefaultDocumentEditor(
131135
document: MutableDocument(
132136
nodes: [
@@ -329,17 +333,6 @@ class _ChatEditorState extends State<_ChatEditor> {
329333

330334
widget.messagePageController.addListener(_onMessagePageControllerChange);
331335

332-
_editorFocusNode.addListener(() {
333-
print(
334-
"Editor focus change. Has primary: ${_editorFocusNode.hasPrimaryFocus}. Has non-primary: ${_editorFocusNode.hasFocus}.");
335-
});
336-
337-
widget.scrollController.addListener(() {
338-
print("Scroll change to: ${widget.scrollController.offset}");
339-
// print("StackTrace:\n${StackTrace.current}");
340-
// print("\n\n");
341-
});
342-
343336
_isImeConnected.addListener(_onImeConnectionChange);
344337

345338
SuperKeyboard.instance.mobileGeometry.addListener(_onKeyboardChange);
@@ -361,8 +354,6 @@ class _ChatEditorState extends State<_ChatEditor> {
361354

362355
widget.messagePageController.removeListener(_onMessagePageControllerChange);
363356

364-
widget.scrollController.dispose();
365-
366357
_keyboardPanelController.dispose();
367358
_isImeConnected.dispose();
368359

‎super_editor/example_chat/lib/message_page_scaffold_demo/demo_textfield_message_page.dart‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,8 +333,6 @@ class _ChatEditorState extends State<_ChatEditor> implements SoftwareKeyboardCon
333333

334334
@override
335335
Widget build(BuildContext context) {
336-
print("BUILD TextField demo");
337-
print(" - toolbar visibility: ${_keyboardPanelController.toolbarVisibility}");
338336
return KeyboardPanelScaffold(
339337
controller: _keyboardPanelController,
340338
isImeConnected: _isImeConnected,
@@ -367,6 +365,7 @@ class _ChatEditorState extends State<_ChatEditor> implements SoftwareKeyboardCon
367365
child: TextField(
368366
key: _editorKey,
369367
focusNode: _textFieldFocusNode,
368+
scrollController: _scrollController,
370369
decoration: InputDecoration(
371370
hintText: "Write message...",
372371
),

‎super_editor/example_chat/pubspec.lock‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,18 @@ packages:
3434
dependency: transitive
3535
description:
3636
name: async
37-
sha256: d2872f9c19731c2e5f10444b14686eb7cc85c76274bd6c16e1816bff9a3bab63
37+
sha256: "758e6d74e971c3e5aceb4110bfd6698efc7f501675bcfe0c775459a8140750eb"
3838
url: "https://pub.dev"
3939
source: hosted
40-
version: "2.12.0"
40+
version: "2.13.0"
4141
attributed_text:
4242
dependency: transitive
4343
description:
4444
name: attributed_text
45-
sha256: "9d8bdc6e54387f31e52d2dcc3d93d656072582a44bcaa855c9ea8b3b2fa7abd3"
45+
sha256: "177ea01f58a8d8df279f4066834375a2009bdd304d559c084bb06f784b258477"
4646
url: "https://pub.dev"
4747
source: hosted
48-
version: "0.4.4"
48+
version: "0.4.5"
4949
boolean_selector:
5050
dependency: transitive
5151
description:
@@ -114,10 +114,10 @@ packages:
114114
dependency: transitive
115115
description:
116116
name: fake_async
117-
sha256: "6a95e56b2449df2273fd8c45a662d6947ce1ebb7aafe80e550a3f68297f3cacc"
117+
sha256: "5368f224a74523e8d2e7399ea1638b37aecfca824a3cc4dfdf77bf1fa905ac44"
118118
url: "https://pub.dev"
119119
source: hosted
120-
version: "1.3.2"
120+
version: "1.3.3"
121121
file:
122122
dependency: transitive
123123
description:
@@ -249,10 +249,10 @@ packages:
249249
dependency: transitive
250250
description:
251251
name: leak_tracker
252-
sha256: c35baad643ba394b40aac41080300150a4f08fd0fd6a10378f8f7c6bc161acec
252+
sha256: "6bb818ecbdffe216e81182c2f0714a2e62b593f4a4f13098713ff1685dfb6ab0"
253253
url: "https://pub.dev"
254254
source: hosted
255-
version: "10.0.8"
255+
version: "10.0.9"
256256
leak_tracker_flutter_testing:
257257
dependency: transitive
258258
description:
@@ -488,7 +488,7 @@ packages:
488488
path: ".."
489489
relative: true
490490
source: path
491-
version: "0.3.0-dev.23"
491+
version: "0.3.0-dev.33"
492492
super_keyboard:
493493
dependency: "direct main"
494494
description:
@@ -629,10 +629,10 @@ packages:
629629
dependency: transitive
630630
description:
631631
name: vm_service
632-
sha256: "0968250880a6c5fe7edc067ed0a13d4bae1577fe2771dcf3010d52c4a9d3ca14"
632+
sha256: ddfa8d30d89985b96407efce8acbdd124701f96741f2d981ca860662f1c0dc02
633633
url: "https://pub.dev"
634634
source: hosted
635-
version: "14.3.1"
635+
version: "15.0.0"
636636
watcher:
637637
dependency: transitive
638638
description:

‎super_editor/lib/src/chat/message_page_scaffold.dart‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,15 +332,23 @@ class MessagePageElement extends RenderObjectElement {
332332
@override
333333
void activate() {
334334
messagePageElementLog.info('ContentLayersElement - activating');
335+
_didActivateSinceLastBuild = false;
335336
super.activate();
336337
}
337338

339+
// Whether this `Element` has been built since the last time `activate()` was run.
340+
var _didActivateSinceLastBuild = false;
341+
338342
@override
339343
void deactivate() {
340344
messagePageElementLog.info('ContentLayersElement - deactivating');
345+
_didDeactivateSinceLastBuild = false;
341346
super.deactivate();
342347
}
343348

349+
// Whether this `Element` has been built since the last time `deactivate()` was run.
350+
bool _didDeactivateSinceLastBuild = false;
351+
344352
@override
345353
void unmount() {
346354
messagePageElementLog.info('ContentLayersElement - unmounting');
@@ -395,6 +403,15 @@ class MessagePageElement extends RenderObjectElement {
395403
);
396404
}
397405
});
406+
407+
// The activation and deactivation processes involve visiting children, which
408+
// we must honor, but the visitation happens some time after the actual call
409+
// to activate and deactivate. So we remember when activation and deactivation
410+
// happened, and now that we've built the `_content`, we clear those flags because
411+
// we assume whatever visitation those processes need to do is now done, since
412+
// we did a build. To learn more about this situation, look at `visitChildren`.
413+
_didActivateSinceLastBuild = false;
414+
_didDeactivateSinceLastBuild = false;
398415
}
399416

400417
@override
@@ -495,17 +512,60 @@ class MessagePageElement extends RenderObjectElement {
495512
visitor(_bottomSheet!);
496513
}
497514

515+
// Building the `_content` is tricky and we're still not sure how to do it
516+
// correctly. Originally, we refused to visit `_content` when `WidgetsBinding.instance.locked`
517+
// is `true`. The original warning about this was the following:
518+
//
498519
// WARNING: Do not visit content when "locked". If you do, then the pipeline
499520
// owner will collect that child for rebuild, e.g., for hot reload, and the
500521
// pipeline owner will tell it to build before the message editor is laid
501522
// out. We only want the content to build during the layout phase, after the
502523
// message editor is laid out.
524+
//
525+
// However, error stacktraces have been showing up for a while whenever the tree
526+
// structure adds/removes widgets in the tree. One way to see this was to open the
527+
// Flutter debugger and enable the widget selector. This adds the widget selector
528+
// widget to tree, and seems to trigger the bug:
529+
//
530+
// 'package:flutter/src/widgets/framework.dart': Failed assertion: line 6164 pos 14:
531+
// '_dependents.isEmpty': is not true.
532+
//
533+
// This happens because when this `Element` runs `deactivate()`, its super class visits
534+
// all the children to deactivate them, too. When that happens, we're apparently
535+
// locked, so we weren't visiting `_content`. This resulted in an error for any
536+
// `_content` subtree widget that setup an `InheritedWidget` dependency, because
537+
// that dependency didn't have a chance to release.
538+
//
539+
// To deal with deactivation, I tried adding a flag during deactivation so that
540+
// we visit `_content` during deactivation. I then discovered that the visitation
541+
// related to deactivation happens sometime after the call to `deactivate()`. So instead
542+
// of only allowing visitation during `deactivate()`, I tracked whether this `Element`
543+
// was in a deactivated state, and allowed visitation when in a deactivated state.
544+
//
545+
// I then found that there's a similar issue during `activate()`. This also needs to
546+
// recursively activate the subtree `Element`s, sometime after the call to `activate()`.
547+
// Therefore, whether activated or deactivated, we need to allow visitation, but we're
548+
// always either activated or deactivated, so this approach needed to be further adjusted.
549+
//
550+
// Presently, when `activate()` or `deactivate()` runs, a flag is set for each one.
551+
// When either of those flags are `true`, we allow visitation. We reset those flags
552+
// during the building of `_content`, as a way to recognize when the activation or
553+
// deactivation process must be finished.
554+
//
555+
// For reference, when hot restarting or hot reloading if we don't enable visitation
556+
// during activation, we get the following error:
557+
//
558+
// The following assertion was thrown during performLayout():
559+
// 'package:flutter/src/widgets/framework.dart': Failed assertion: line 4323 pos 7: '_lifecycleState ==
560+
// _ElementLifecycle.active &&
561+
// newWidget != widget &&
562+
// Widget.canUpdate(widget, newWidget)': is not true.
503563

504564
// FIXME: locked is supposed to be private. We're using it as a proxy
505565
// indication for when the build owner wants to build. Find an
506566
// appropriate way to distinguish this.
507567
// ignore: invalid_use_of_protected_member
508-
if (!WidgetsBinding.instance.locked) {
568+
if (!WidgetsBinding.instance.locked || !_didActivateSinceLastBuild || !_didDeactivateSinceLastBuild) {
509569
if (_content != null) {
510570
visitor(_content!);
511571
}
@@ -826,7 +886,6 @@ class RenderMessagePageScaffold extends RenderBox {
826886

827887
@override
828888
void detach() {
829-
// print("detach()'ing RenderChatScaffold from pipeline");
830889
// IMPORTANT: we must detach ourselves before detaching our children.
831890
// This is a Flutter framework requirement.
832891
super.detach();

‎super_editor/lib/src/default_editor/document_gestures_touch_ios.dart‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,7 +1516,12 @@ class SuperEditorIosToolbarOverlayManagerState extends State<SuperEditorIosToolb
15161516
super.didChangeDependencies();
15171517

15181518
_controlsController = SuperEditorIosControlsScope.rootOf(context);
1519-
_overlayPortalController.show();
1519+
1520+
WidgetsBinding.instance.addPostFrameCallback((_) {
1521+
// There's some stupid Flutter error that happens without this post frame callback
1522+
// where the error is "OverlayPortalController.show() should not be called during build.".
1523+
_overlayPortalController.show();
1524+
});
15201525
}
15211526

15221527
@visibleForTesting
@@ -1578,7 +1583,12 @@ class SuperEditorIosMagnifierOverlayManagerState extends State<SuperEditorIosMag
15781583
void didChangeDependencies() {
15791584
super.didChangeDependencies();
15801585
_controlsController = SuperEditorIosControlsScope.rootOf(context);
1581-
_overlayPortalController.show();
1586+
1587+
WidgetsBinding.instance.addPostFrameCallback((_) {
1588+
// There's some stupid Flutter error that happens without this post frame callback
1589+
// where the error is "OverlayPortalController.show() should not be called during build.".
1590+
_overlayPortalController.show();
1591+
});
15821592
}
15831593

15841594
@override

‎super_editor/lib/src/default_editor/layout_single_column/super_editor_dry_layout.dart‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,13 @@ class RenderViewportWithDryLayout extends RenderViewport {
138138
}
139139

140140
@override
141-
computeMaxIntrinsicHeight(double width) {
141+
double computeMaxIntrinsicHeight(double width) {
142142
final layoutBox = _findFirstRenderBoxInSliverList(child);
143143
return layoutBox.computeMaxIntrinsicHeight(width);
144144
}
145145

146146
@override
147-
computeMinIntrinsicHeight(double width) {
147+
double computeMinIntrinsicHeight(double width) {
148148
final layoutBox = _findFirstRenderBoxInSliverList(child);
149149
return layoutBox.computeMinIntrinsicHeight(width);
150150
}

‎super_editor/lib/src/infrastructure/keyboard_panel_scaffold.dart‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -643,16 +643,16 @@ Building keyboard scaffold
643643
return ValueListenableBuilder(
644644
valueListenable: _currentBottomSpacing,
645645
builder: (context, currentHeight, child) {
646-
if (!_wantsToShowToolbar && !shouldShowKeyboardPanel) {
647-
return const SizedBox.shrink();
648-
}
649-
650646
onNextFrame((_) {
651647
// Ensure that our latest keyboard height/panel height calculations are
652648
// accounted for in the ancestor safe area after this layout pass.
653649
_updateSafeArea();
654650
});
655651

652+
if (!_wantsToShowToolbar && !shouldShowKeyboardPanel) {
653+
return const SizedBox.shrink();
654+
}
655+
656656
return Positioned(
657657
bottom: 0,
658658
left: 0,
@@ -1227,6 +1227,15 @@ class _KeyboardScaffoldSafeAreaState extends State<KeyboardScaffoldSafeArea> {
12271227
return 0;
12281228
}
12291229

1230+
final screenHeight = MediaQuery.sizeOf(safeAreaContext).height;
1231+
if (myGlobalBottom > screenHeight) {
1232+
// The content is below the bottom of the screen. This can happen, for example, when a
1233+
// page animates in/out, such as a bottom sheet. While the content is at all below the
1234+
// bottom of the screen, apply the `bottomInsets` without any adjustment. When the
1235+
// content is fully onscreen, we can adjust it with `spaceBelowMe`.
1236+
return bottomInsets;
1237+
}
1238+
12301239
final spaceBelowMe = MediaQuery.sizeOf(safeAreaContext).height - myGlobalBottom;
12311240

12321241
// The bottom insets are measured from the bottom of the screen. But we might not

0 commit comments

Comments
 (0)