Skip to content

Commit b9f38a6

Browse files
authored
Refactors FItemData to fix inherit issues with FPopover (#623)
* Refactors FItemData to fix inherit issues with FPopover Changes `FItemData` to a data class and introduces `FInheritedItemData` to manage inheritance, preventing styling issues when using `FPopover` inside `FItemGroup` or `FTileGroup`. This change addresses a problem where styling from parent groups would unintentionally leak into popovers due to shared `BuildContext`, leading to rendering problems. * Prepare Forui for review * Update macos-latest goldens * Update windows-latest goldens * Fix select item dividers * Update windows-latest goldens * Fix dividers yet again * Update windows-latest goldens --------- Co-authored-by: Pante <[email protected]>
1 parent d7097c6 commit b9f38a6

File tree

19 files changed

+428
-133
lines changed

19 files changed

+428
-133
lines changed

forui/CHANGELOG.md

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
## 0.14.0 (Next)
1+
## 0.14.0
22

33
### `FButton`
44
* Add `FButton.onSecondaryPress`.
@@ -12,6 +12,12 @@
1212
* Add `FHeaderAction.shortcuts`.
1313

1414

15+
### `FItemData`
16+
17+
* **Breaking** Change `FItemData` to store fields in separate data class instead of directly in an inherited widget.
18+
* **Breaking** Rename `FItemData` to `FInheritedItemData`.
19+
20+
1521
### `FPopover`
1622
* **Breaking** Change `FPopoverController.shown` to `FPopoverController.status`.
1723

@@ -28,12 +34,17 @@
2834
* Fix `FSelect` layout shifting when scrolling through a long list of items.
2935

3036

37+
### `FSelectMenuTile`
38+
* Add `FSelectMenuTile.fromMap(...)`.
39+
40+
* Fix `FSelectMenuTile` throwing an error when wrapped in a `FTileGroup`.
41+
42+
3143
### `FSidebar`
3244
* Add `FSidebar.focusNode`.
3345
* Add `FSidebar.traversalEdgeBehavior`.
3446

3547
* Change `FSidebarItemStyle.focusedOutlineStyle.spacing` from 3 to 0.
36-
* Convert `FSidebar` from a stateless to a stateful widget.
3748

3849
* Fix `FSidebar`'s focus traversal behavior.
3950

@@ -49,14 +60,14 @@
4960

5061

5162
### Others
52-
* Add `FSelectMenuTile.fromMap(...)`.
5363
* Add `FToasterStyle.toastAlignment`.
5464

5565
* Change default `hideOnTapOutside` in `FDateField.calendar(...)` from `FHidePopoverRegion.anywhere` to `FHidePopoverRegion.excludeTarget`.
5666
* Change default `hideOnTapOutside` in `FTimeField.picker(...)` from `FHidePopoverRegion.anywhere` to `FHidePopoverRegion.excludeTarget`.
5767
* **Breaking** Change `FPersistentSheetController.shown` to `FPersistentSheetController.status`.
5868
* **Breaking** Change `FTooltipController.shown` to `FTooltipController.status`.
5969

70+
* Fix `FPopoverMenu` throwing an error when wrapped in a `FTileGroup`.
6071
* Fix `FTextField` applying different padding on mobile & desktop.
6172

6273

forui/lib/src/widgets/item/item.dart

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,40 @@ mixin FItemMixin on Widget {}
1414

1515
/// An item that is typically used to group related information together.
1616
///
17+
/// ## Using [FItem] in a [FPopover] when wrapped in a [FItemGroup]
18+
/// When a [FPopover] is used inside an [FItemGroup], items inside the popover will inherit styling from the parent group.
19+
/// This happens because [FPopover]'s content shares the same `BuildContext` as its child, causing data inheritance
20+
/// that may lead to unexpected rendering issues.
21+
///
22+
/// To prevent this styling inheritance, wrap the popover in a [FInheritedItemData] with null data to reset the
23+
/// inherited data:
24+
/// ```dart
25+
/// FItemGroup(
26+
/// children: [
27+
/// FItem(title: Text('Item with popover')),
28+
/// FPopoverWrapperItem(
29+
/// popoverBuilder: (_, _) => FInheritedItemData(
30+
/// child: FItemGroup(
31+
/// children: [
32+
/// FItem(title: Text('Popover Item 1')),
33+
/// FItem(title: Text('Popover Item 2')),
34+
/// ],
35+
/// ),
36+
/// ),
37+
/// child: FButton(child: Text('Open Popover')),
38+
/// ),
39+
/// ],
40+
/// );
41+
/// ```
42+
///
1743
/// See:
1844
/// * https://forui.dev/docs/data/item for working examples.
1945
/// * [FTile] for a specialized item for touch devices.
2046
/// * [FItemStyle] for customizing an item's appearance.
2147
class FItem extends StatelessWidget with FItemMixin {
2248
/// The item's style. Defaults to [FItemData.style] if present.
2349
///
24-
/// Provide a style to prevent inheritance from [FItemData].
50+
/// Provide a style to prevent inheritance from [FInheritedItemData].
2551
///
2652
/// ## CLI
2753
/// To generate and customize this style:
@@ -212,7 +238,7 @@ class FItem extends StatelessWidget with FItemMixin {
212238

213239
@override
214240
Widget build(BuildContext context) {
215-
final data = FItemData.of(context);
241+
final data = FInheritedItemData.maybeOf(context) ?? const FItemData();
216242
final inheritedStyle = data.style ?? context.theme.itemStyle;
217243

218244
final style = this.style?.call(inheritedStyle) ?? inheritedStyle;

forui/lib/src/widgets/item/item_data.dart

Lines changed: 95 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,75 @@ enum FItemDivider {
3838
none,
3939
}
4040

41-
/// An [FItemData] is used to provide data about the item's position in the current nesting level, i.e. [FTileGroup].
41+
/// An [FInheritedItemData] is used to provide data about the item's position in the current nesting level, i.e. [FTileGroup].
4242
///
4343
/// Users that wish to create their own custom group should pass additional data to the children using a separate
4444
/// inherited widget.
45-
final class FItemData extends InheritedWidget {
45+
final class FInheritedItemData extends InheritedWidget {
46+
/// Returns the [FItemData] in the given [context].
47+
static FItemData? maybeOf(BuildContext context) =>
48+
context.dependOnInheritedWidgetOfExactType<FInheritedItemData>()?.data;
49+
50+
/// The item's properties.
51+
final FItemData? data;
52+
53+
/// Creates a [FInheritedItemData].
54+
const FInheritedItemData({required super.child, this.data, super.key});
55+
56+
/// Creates a [FInheritedItemData] that merges the given fields with the current [FInheritedItemData].
57+
static Widget merge({
58+
required bool last,
59+
required Widget child,
60+
FItemStyle? style,
61+
double? spacing,
62+
FItemDivider? divider,
63+
FWidgetStateMap<Color>? dividerColor,
64+
double? dividerWidth,
65+
bool? enabled,
66+
int? index,
67+
}) => Builder(
68+
builder: (context) {
69+
final parent = maybeOf(context);
70+
final globalLast = last && (parent?.globalLast ?? true);
71+
72+
return FInheritedItemData(
73+
data: FItemData(
74+
style: style ?? parent?.style,
75+
spacing: max(spacing ?? 0, parent?.spacing ?? 0),
76+
dividerColor: dividerColor ?? parent?.dividerColor ?? FWidgetStateMap.all(Colors.transparent),
77+
dividerWidth: dividerWidth ?? parent?.dividerWidth ?? 0,
78+
divider: switch ((last, globalLast)) {
79+
// The first/middle items of a group.
80+
(false, false) => divider ?? FItemDivider.none,
81+
// Last of a group which itself isn't the last.
82+
// propagatedLast can only be false if parent?.last is false since last must always be true.
83+
// Hence, parent!.divider can never be null.
84+
(true, false) => parent!.divider,
85+
// The last item in the last group.
86+
(_, true) => FItemDivider.none,
87+
},
88+
enabled: enabled ?? parent?.enabled ?? true,
89+
index: index ?? parent?.index ?? 0,
90+
last: last,
91+
globalLast: globalLast,
92+
),
93+
child: child,
94+
);
95+
},
96+
);
97+
98+
@override
99+
bool updateShouldNotify(FInheritedItemData old) => data != old.data;
100+
101+
@override
102+
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
103+
super.debugFillProperties(properties);
104+
properties.add(DiagnosticsProperty('data', data));
105+
}
106+
}
107+
108+
/// The item's data.
109+
final class FItemData with Diagnosticable {
46110
/// The item's style.
47111
final FItemStyle? style;
48112

@@ -70,91 +134,19 @@ final class FItemData extends InheritedWidget {
70134
/// True if the item is the last item across all levels.
71135
final bool globalLast;
72136

73-
/// Creates a [FItemData].
137+
/// Creates a new [FItemData].
74138
const FItemData({
75-
required this.style,
76-
required this.spacing,
77-
required this.dividerColor,
78-
required this.dividerWidth,
79-
required this.divider,
80-
required this.enabled,
81-
required this.index,
82-
required this.last,
83-
required this.globalLast,
84-
required super.child,
85-
super.key,
139+
this.style,
140+
this.spacing = 0,
141+
this.dividerColor = const FWidgetStateMap({WidgetState.any: Colors.transparent}),
142+
this.dividerWidth = 0,
143+
this.divider = FItemDivider.none,
144+
this.enabled = true,
145+
this.index = 0,
146+
this.last = true,
147+
this.globalLast = true,
86148
});
87149

88-
/// Creates a [FItemData] that merges the given fields with the current [FItemData].
89-
static Widget merge({
90-
required bool last,
91-
required Widget child,
92-
FItemStyle? style,
93-
double? spacing,
94-
FItemDivider? divider,
95-
FWidgetStateMap<Color>? dividerColor,
96-
double? dividerWidth,
97-
bool? enabled,
98-
int? index,
99-
}) => Builder(
100-
builder: (context) {
101-
final parent = context.dependOnInheritedWidgetOfExactType<FItemData>();
102-
final globalLast = last && (parent?.globalLast ?? true);
103-
104-
return FItemData(
105-
style: style ?? parent?.style,
106-
spacing: max(spacing ?? 0, parent?.spacing ?? 0),
107-
dividerColor: dividerColor ?? parent?.dividerColor ?? FWidgetStateMap.all(Colors.transparent),
108-
dividerWidth: dividerWidth ?? parent?.dividerWidth ?? 0,
109-
divider: switch ((last, globalLast)) {
110-
// The first/middle items of a group.
111-
(false, false) => divider ?? FItemDivider.none,
112-
// Last of a group which itself isn't the last.
113-
// propagatedLast can only be false if parent?.last is false since last must always be true.
114-
// Hence, parent!.divider can never be null.
115-
(true, false) => parent!.divider,
116-
// The last item in the last group.
117-
(_, true) => FItemDivider.none,
118-
},
119-
enabled: enabled ?? parent?.enabled ?? true,
120-
index: index ?? parent?.index ?? 0,
121-
last: last,
122-
globalLast: globalLast,
123-
child: child,
124-
);
125-
},
126-
);
127-
128-
/// Returns the [FItemData] in the given [context].
129-
static FItemData? maybeOf(BuildContext context) => context.dependOnInheritedWidgetOfExactType<FItemData>();
130-
131-
/// Returns the [FItemData] in the given [context], or a default [FItemData] if none is found.
132-
factory FItemData.of(BuildContext context) =>
133-
maybeOf(context) ??
134-
FItemData(
135-
style: null,
136-
spacing: 0,
137-
dividerColor: FWidgetStateMap.all(Colors.transparent),
138-
dividerWidth: 0,
139-
divider: FItemDivider.none,
140-
enabled: true,
141-
index: 0,
142-
last: true,
143-
globalLast: true,
144-
child: const SizedBox(),
145-
);
146-
147-
@override
148-
bool updateShouldNotify(FItemData old) =>
149-
style != old.style ||
150-
dividerColor != old.dividerColor ||
151-
dividerWidth != old.dividerWidth ||
152-
divider != old.divider ||
153-
enabled != old.enabled ||
154-
index != old.index ||
155-
last != old.last ||
156-
globalLast != old.globalLast;
157-
158150
@override
159151
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
160152
super.debugFillProperties(properties);
@@ -169,4 +161,23 @@ final class FItemData extends InheritedWidget {
169161
..add(FlagProperty('last', value: last, ifTrue: 'last'))
170162
..add(FlagProperty('globalLast', value: globalLast, ifTrue: 'globalLast'));
171163
}
164+
165+
@override
166+
bool operator ==(Object other) =>
167+
identical(this, other) ||
168+
other is FItemData &&
169+
runtimeType == other.runtimeType &&
170+
style == other.style &&
171+
spacing == other.spacing &&
172+
dividerColor == other.dividerColor &&
173+
dividerWidth == other.dividerWidth &&
174+
divider == other.divider &&
175+
enabled == other.enabled &&
176+
index == other.index &&
177+
last == other.last &&
178+
globalLast == other.globalLast;
179+
180+
@override
181+
int get hashCode =>
182+
Object.hash(style, spacing, dividerColor, dividerWidth, divider, enabled, index, last, globalLast);
172183
}

forui/lib/src/widgets/item/item_group.dart

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,33 @@ mixin FItemGroupMixin on Widget {}
1515
///
1616
/// Items grouped together will be separated by a divider, specified by [divider].
1717
///
18+
/// ## Using [FItemGroup] in a [FPopover] when wrapped in a [FItemGroup]
19+
/// When a [FPopover] is used inside an [FItemGroup], items & groups inside the popover will inherit styling from the
20+
/// parent group. This happens because [FPopover]'s content shares the same `BuildContext` as its child, causing data
21+
/// inheritance that may lead to unexpected rendering issues.
22+
///
23+
/// To prevent this styling inheritance, wrap the popover in a [FInheritedItemData] with null data to reset the
24+
/// inherited data:
25+
/// ```dart
26+
/// FItemGroup(
27+
/// children: [
28+
/// FItem(title: Text('Item with popover')),
29+
/// FPopoverWrapperItem(
30+
/// popoverBuilder: (_, _) => FInheritedItemData(
31+
/// child: FItemGroup(
32+
/// children: [
33+
/// FItem(title: Text('Popover Item 1')),
34+
/// FItem(title: Text('Popover Item 2')),
35+
/// ],
36+
/// ),
37+
/// ),
38+
/// child: FButton(child: Text('Open Popover')),
39+
/// ),
40+
/// ],
41+
/// );
42+
/// ```
43+
///
44+
///
1845
/// See:
1946
/// * https://forui.dev/docs/data/item-group for working examples.
2047
/// * [FItemGroupStyle] for customizing a item group's appearance.
@@ -106,7 +133,7 @@ class FItemGroup extends StatelessWidget with FItemGroupMixin {
106133
_builder = ((style, enabled) => SliverList.list(
107134
children: [
108135
for (final (index, child) in children.indexed)
109-
FItemData.merge(
136+
FInheritedItemData.merge(
110137
style: style.itemStyle,
111138
spacing: style.spacing,
112139
enabled: enabled,
@@ -123,7 +150,7 @@ class FItemGroup extends StatelessWidget with FItemGroupMixin {
123150
/// Creates a [FItemGroup] that lazily builds its children.
124151
///
125152
/// {@template forui.widgets.FItemGroup.builder}
126-
/// The [itemBuilder] is called for each item that should be built. The current level's [FItemData] is **not**
153+
/// The [itemBuilder] is called for each item that should be built. The current level's [FInheritedItemData] is **not**
127154
/// visible to `itemBuilder`.
128155
/// * It may return null to signify the end of the group.
129156
/// * It may be called more than once for the same index.
@@ -156,7 +183,7 @@ class FItemGroup extends StatelessWidget with FItemGroupMixin {
156183
itemCount: count,
157184
itemBuilder: (context, index) {
158185
if (itemBuilder(context, index) case final item?) {
159-
return FItemData.merge(
186+
return FInheritedItemData.merge(
160187
style: style.itemStyle,
161188
spacing: style.spacing,
162189
enabled: enabled,
@@ -192,7 +219,7 @@ class FItemGroup extends StatelessWidget with FItemGroupMixin {
192219
_builder = ((style, enabled) => SliverMainAxisGroup(
193220
slivers: [
194221
for (final (index, child) in children.indexed)
195-
FItemData.merge(
222+
FInheritedItemData.merge(
196223
style: style.itemStyle,
197224
spacing: style.spacing,
198225
enabled: enabled,
@@ -208,7 +235,7 @@ class FItemGroup extends StatelessWidget with FItemGroupMixin {
208235

209236
@override
210237
Widget build(BuildContext context) {
211-
final data = FItemData.maybeOf(context);
238+
final data = FInheritedItemData.maybeOf(context);
212239
final inheritedStyle = FItemGroupStyleData.of(context);
213240
final style = this.style?.call(inheritedStyle) ?? inheritedStyle;
214241
final enabled = this.enabled ?? data?.enabled ?? true;

forui/lib/src/widgets/popover_menu.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ class FPopoverMenu extends StatelessWidget {
256256
traversalEdgeBehavior: traversalEdgeBehavior,
257257
barrierSemanticsLabel: barrierSemanticsLabel,
258258
barrierSemanticsDismissible: barrierSemanticsDismissible,
259-
popoverBuilder: (context, controller) => _menuBuilder(context, controller, style),
259+
popoverBuilder: (context, controller) => FInheritedItemData(child: _menuBuilder(context, controller, style)),
260260
builder: builder,
261261
child: child,
262262
);

0 commit comments

Comments
 (0)