[FEAT] Image component updates#6
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| child: SelectableBox( | ||
| selectionColor: selectionColor, | ||
| selection: selection, | ||
| enableIgnorePointer: selection != null, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR adds support for alternative text (altText) to image components and makes several structural changes to the ImageComponent widget tree. The changes include adding an altText property throughout the image component hierarchy, refactoring the imageBuilder function signature to use named parameters and include altText, reorganizing imports to use absolute package imports, and restructuring the ImageComponent widget tree.
Changes:
- Added altText property to ImageNode, ImageComponentViewModel, ImageComponentBuilder, and ImageComponent
- Changed imageBuilder function signature from positional to named parameters and added altText support
- Reorganized imports to use absolute
package:super_editor/src/...imports instead of relative imports - Restructured ImageComponent widget tree, removing outer MouseRegion and IgnorePointer, and reordering BoxComponent and SelectableBox
- Added enableIgnorePointer parameter to SelectableBox to control pointer event handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| super_editor/lib/src/default_editor/image.dart | Added altText property throughout image component hierarchy, changed imageBuilder signature to named parameters, restructured widget tree, and reorganized imports |
| super_editor/lib/src/default_editor/box_component.dart | Added enableIgnorePointer parameter to SelectableBox for conditional pointer event handling and reorganized imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.imageBuilder, | ||
| }); | ||
|
|
||
| final Widget Function(BuildContext context, {required String imageUrl, String altText})? imageBuilder; |
There was a problem hiding this comment.
The imageBuilder signature has been changed from (BuildContext context, String imageUrl) to (BuildContext context, {required String imageUrl, String altText}). This is a breaking change that breaks existing test code:
super_editor/test/super_editor/supereditor_components_test.dart:198super_editor/test/super_editor/supereditor_test_tools.dart:1035
These tests need to be updated to use named parameters. Additionally, consider making the altText parameter required (required String altText) since it's always passed when calling the builder at line 251.
| final Widget Function(BuildContext context, {required String imageUrl, String altText})? imageBuilder; | |
| final Widget Function(BuildContext context, {required String imageUrl, required String altText})? imageBuilder; |
| child: SelectableBox( | ||
| selectionColor: selectionColor, | ||
| selection: selection, | ||
| enableIgnorePointer: selection != null, |
There was a problem hiding this comment.
The logic for enableIgnorePointer appears to be inverted. The parameter is set to selection != null, which means IgnorePointer will be enabled (ignoring = true) when there IS a selection. This seems counterintuitive - typically you would want to ignore pointer events when there is NO selection, not when there is one. Consider whether this should be selection == null instead, or if the parameter name should be changed to better reflect the intended behavior.
| enableIgnorePointer: selection != null, | |
| enableIgnorePointer: selection == null, |
| child: BoxComponent( | ||
| key: componentKey, | ||
| opacity: opacity, | ||
| child: SelectableBox( | ||
| selectionColor: selectionColor, | ||
| selection: selection, | ||
| enableIgnorePointer: selection != null, |
There was a problem hiding this comment.
The widget tree structure has been changed, placing BoxComponent (which applies opacity) above SelectableBox. This means that the selection decoration will now be affected by the opacity setting of the image component, whereas in the previous implementation, the selection decoration was not affected by opacity. If this is intentional, it's fine, but if selection decorations should always be visible at full opacity regardless of component opacity, this change will cause a regression.
| child: BoxComponent( | |
| key: componentKey, | |
| opacity: opacity, | |
| child: SelectableBox( | |
| selectionColor: selectionColor, | |
| selection: selection, | |
| enableIgnorePointer: selection != null, | |
| child: SelectableBox( | |
| selectionColor: selectionColor, | |
| selection: selection, | |
| enableIgnorePointer: selection != null, | |
| child: BoxComponent( | |
| key: componentKey, | |
| opacity: opacity, |
Co-authored-by: Copilot <[email protected]>
| /// | ||
| /// If [imageBuilder] is `null` an [Image] is used. | ||
| final Widget Function(BuildContext context, String imageUrl)? imageBuilder; | ||
| final Widget Function(BuildContext context, {required String imageUrl, String altText})? imageBuilder; |
There was a problem hiding this comment.
Bug: The imageBuilder signature change to named parameters is a breaking change, as test utilities still use the old positional signature, causing runtime errors in tests.
Severity: HIGH
Suggested Fix
Update the imageBuilder definitions in all affected test files (supereditor_test_tools.dart, supereditor_components_test.dart, test_tools.dart) to use the new signature with named parameters. For example, change (context, imageUrl) => ... to (context, {required String imageUrl, String? altText}) => ....
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: super_editor/lib/src/default_editor/image.dart#L238
Potential issue: The `imageBuilder` function signature was changed from using positional
parameters to named parameters (`Widget Function(BuildContext context, {required String
imageUrl, String altText})`). However, several test utility files, including
`super_editor/test/super_editor/supereditor_test_tools.dart`, still define
`imageBuilder` implementations using the old positional signature, like `(context,
imageUrl) => ...`. This creates a type incompatibility. When the code attempts to invoke
the builder with named parameters, it will cause a runtime error because the provided
function expects positional parameters. This will lead to failures in numerous tests
that rely on these utilities.
…lutter 3.35-3.40 compat) Upstream commit b422c32 (PR Flutter-Bounty-Hunters#2950, Feb 2026) added: @OverRide void updateStyle(TextInputStyle style) => client?.updateStyle(style); to TextInputConnectionDecorator in anticipation of Flutter's "Deprecate TextInputConnection.setStyle" breaking change (flutter/flutter#180436), which adds TextInputConnection.updateStyle( TextInputStyle) and deprecates setStyle(...). Per docs.flutter.dev/release/breaking-changes/deprecate-text-input- connection-set-style (last updated 2026-03-03), the change is "Landed in version: TBD / In stable release: Not yet." Our pinned Flutter is 3.35.7 (`flutter --version`): - flutter-sdk-3.35.5/packages/flutter/lib/src/services/text_input.dart declares only `void setStyle({...})`. There is no `class TextInputStyle` and no `void updateStyle(...)` on TextInputConnection. - TextInputConnectionDecorator implements TextInputConnection, so the override fails to compile: "TextInputStyle isn't a type" + "method updateStyle isn't defined for the type TextInputConnection". There is no Dart conditional-imports trick that can selectively include an interface override based on host SDK version (conditional imports switch libraries, not class members). Fix: drop the override and replace it with a comment block that: - Explains the upstream origin (PR Flutter-Bounty-Hunters#2950, commits 4703643 / b422c32). - Cites the Flutter breaking-change doc. - Documents restore criteria (when Flutter ships TextInputStyle in stable AND we upgrade past that release). This is a no-op functionally on stable Flutter: setStyle(...) above already covers the entire surviving public API; the framework can't call a method that doesn't exist on the parent class. Tracked as fork divergence #6 in FORK_CHANGES.md, marked as TEMPORARY. Will be re-asserted on each upstream sync until either upstream gates this on a Flutter version constraint or we upgrade past the TextInputStyle-shipping stable release. Refs: #13 (parent merge PR)
[FEAT] Image component updates