-
Notifications
You must be signed in to change notification settings - Fork 484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: WebView
enhancements
#4018
Conversation
Reviewer's Guide by SourceryThis pull request introduces significant enhancements to the WebView component in the Flet framework, improving its functionality across different platforms (mobile, web, and desktop). The changes include new features, better platform support, and code refactoring for improved maintainability. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ndonkoHenri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more comprehensive error handling for WebView operations, especially for platform-specific implementations.
- It might be beneficial to add some performance benchmarks or profiling for the new WebView features, particularly for mobile platforms.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
arguments={"url": url, "method": method.value}, | ||
) | ||
|
||
def run_javascript(self, value: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 suggestion (security): Add security warning for run_javascript method
Consider adding a prominent warning in the method documentation about the potential security risks of executing arbitrary JavaScript.
def run_javascript(self, value: str):
"""
Execute JavaScript in the WebView.
WARNING: This method can pose significant security risks.
Only use with trusted input to prevent potential
cross-site scripting (XSS) attacks.
"""
@@ -337,6 +337,9 @@ def invoke_method( | |||
assert ( | |||
self.__page | |||
), f"{self.__class__.__qualname__} Control must be added to the page first" | |||
if arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider moving argument processing to a separate method
Extracting the argument processing logic to a separate method could improve readability and reusability, especially since it's duplicated in invoke_method_async.
def _process_arguments(self, arguments):
if arguments:
return {k: str(v) for k, v in arguments.items() if v is not None}
return {}
if arguments:
arguments = self._process_arguments(arguments)
Kindly merge and one more thing add permission in manifest for Android to display local html files in web view |
This comment has been minimized.
This comment has been minimized.
This comment was marked as spam.
This comment was marked as spam.
Is there any chance for supporting WebView CookieManager ? |
This looks great! Can't wait to implement this in my project once it gets merged to main!! |
@sourcery-ai review |
This comment was marked as spam.
This comment was marked as spam.
Have documentation updated in flet docs? |
Working on it. |
Good stuff @ndonkoHenri 😎! But i have some questions, are we going to get the run_javascripts() methods support on other platforms soon? I thought this PR would add WV support for the windows, |
Windows and Linux are not yet supported. Will have a look at them after we release 0.25.0. |
Would be great to have the run_javascripts() method on Web too Edit: Am not able to update the url value when running on web. Is it a bug or that’s how it’s supposed to be? |
Am able to change the url value outside of a function,… but once am inside a function/eventhandler an error is shown saying that the method is supported on Android, iOS and macOS only. |
@Benitmulindwa let's continue this in a new discussion. Can you create one and explain the issues you face? |
@ndonkoHenri all right lemme do it |
On Windows, an error is reported: Webview is not yet supported on this Platform. |
Windows and Linux support is still to be added. |
Summary by Sourcery
Enhance the WebView component with new event handling capabilities and methods, including platform-specific implementations for mobile, macOS, Windows, and Linux. Deprecate the 'javascript_enabled' property in favor of 'enable_javascript'. Refactor platform detection logic to improve code maintainability. Update dependencies to support new features and remove unused imports from test files.
New Features:
Enhancements:
Build:
Tests: