[Property Editor] Add workaround for embedded TextField focus issues - #8957
Conversation
There was a problem hiding this comment.
Hello @elliette, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses an issue where TextField widgets embedded in an IFRAME retain focus even when they shouldn't. The fix involves adding a workaround that listens for the window losing focus (blur event) and then explicitly blurring the currently active TextField. This prevents the keyboard from remaining active when the user interacts with other parts of the application outside the TextField.
Highlights
- Bug Fix: Implements a workaround for a focus issue with
TextFieldwidgets in IFRAMEs (#8929). - Focus Management: Adds event listeners to detect when the window loses focus and programmatically blur the active
TextField. - Conditional Implementation: The workaround is only applied in web environments due to the use of
dart.library.js_interop.
Changelog
Click here to see the changelog
- packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart
- Imported
utils.dartto access the focus fix handlers.
- Imported
- packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_view.dart
- Converted
_PropertiesListfrom aStatelessWidgetto aStatefulWidgetto manage the lifecycle of the focus fix. - Added
initStateto callsetUpTextFieldFocusFixHandlerwhen the widget is initialized. - Added
disposeto callremoveTextFieldFocusFixHandlerwhen the widget is disposed. - Updated the
buildmethod to usewidget.editablePropertiesandwidget.editPropertyto access the properties passed to the widget.
- Converted
- packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/utils/_utils_desktop.dart
- Created a desktop implementation of the focus fix handlers that do nothing, as the issue is specific to web environments.
- packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/utils/_utils_web.dart
- Created a web implementation of the focus fix handlers that use
dart:js_interopandpackage:web/web.dartto listen for the 'blur' event on the window and blur the active element if it's anHTMLElement.
- Created a web implementation of the focus fix handlers that use
- packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/utils/utils.dart
- Added conditional import to use the web implementation of the focus fix handlers when running in a web environment.
- Added
setUpTextFieldFocusFixHandlerandremoveTextFieldFocusFixHandlerfunctions to set up and remove the focus fix.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The first version of HTML was written by Tim Berners-Lee in 1993. It only had about 20 tags!
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a workaround for a focus issue with TextField widgets embedded in IFRAMEs within the DevTools application. The approach involves adding and removing blur listeners to the window to force blur the active element, effectively preventing the TextField from holding onto focus. The implementation appears to be well-structured and addresses the reported issue. I've provided some minor suggestions for improvement.
Summary of Findings
- Conditional Import: The use of conditional imports with
if (dart.library.js_interop)is a good approach for platform-specific code. However, ensure that the desktop implementation provides a no-op to prevent errors on non-web platforms. - Lifecycle Management: The
initStateanddisposemethods in_PropertiesListStatecorrectly manage the addition and removal of the focus fix handler. This ensures that the workaround is active only when the widget is in use and prevents memory leaks. - Code Clarity: The comments explaining the workaround and referencing the related GitHub issues are helpful for understanding the purpose of the code.
Assessment
The pull request effectively addresses the focus issue with TextField widgets in IFRAMEs. The code is well-organized and includes necessary lifecycle management. The use of conditional imports allows for platform-specific behavior. I recommend addressing the minor suggestions provided in the review comments. After addressing these comments, it should be in good shape to be merged, but please have others review and approve this code before merging.
| ...widget.editableProperties.map( | ||
| (property) => _EditablePropertyItem( | ||
| property: property, | ||
| editProperty: editProperty, | ||
| editProperty: widget.editProperty, |
There was a problem hiding this comment.
Consider using the map method directly without spreading the result into a list. This can improve readability and potentially performance.
Also, consider using a forEach loop instead of map if you are not using the result of the mapping.
for (final property in widget.editableProperties) {
_EditablePropertyItem(
property: property,
editProperty: widget.editProperty,
);
}
There was a problem hiding this comment.
I'm pretty sure we were using spread operator here inside a new List so that we could specify the List as type List<Widget> so that we could call joinWith on it (below)
There was a problem hiding this comment.
Can we do this?
Column(
children: <Widget>[
for (final property in widget.editableProperties)
_EditablePropertyItem(...),
].joinWith(const PaddedDivider.noPadding())
);
There was a problem hiding this comment.
Fwiw, joinWith could be an extension on Iterable instead of List, and then using widget.editableProperties.map(...).joinWith(...) would avoid allocating the extra intermediate list.
If this is a small list it probably doesn't matter though, but it could add up over time 🤷♂️ .
There was a problem hiding this comment.
Switched to using a for loop here!
Switching to joinWith to an Iterable extension makes sense, though I think we still need to cast Iterable<widget subtype> to Iterable<Widget> in order to join with a different widget subtype (in this case _EditableProperty joined with PaddedDivider).
There was a problem hiding this comment.
You should also be able to just pass an explicit type argument to map .map<Widget>(...)
kenzieschmoll
left a comment
There was a problem hiding this comment.
LGTM after comments are addressed.
Fixes #8929
Adds workaround described in flutter/flutter#155265 (comment) to prevent
TextFieldfrom holding onto focus when embedded in an IFRAME.Demo (fixed!) 🎉