WebView URL filtering #3973
WebView URL filtering #3973
Conversation
* implented on_navigation_started for android * extended the webview example
* applied black
* added type declaration for handler * added WeakrefCallable wrapper
…_navigation_starting handlers
…he cleanup method
…yproject.toml * improve docstring for on_navigation_starting * updated app.py to include the latest changes from main
* fixed too long lines * set asynchronous handler in example app
* adjusted WebView documentation for Android's staticProxy * added Android staticProxy for testbed application * removed debugging prints
* fixed test_navigation_starting_async
* added test for set_content with navigation_starting handler * removed unnecessary try block * modified set_url on dummy platform
* fixed set_content on Android * added sync handler test for testbed app
|
@freakboy3742 I implemented all changes requested in the review. Please re-review this PR. |
|
Hello @t-arn, The docs failure is not related to your PR. We'll make sure it's resolved before the PR is landed. I want to let you know that the core team is currently on holiday leave, so response times are going to be slower for the next few weeks. You can likely expect a response within eight days, likely sooner, as we'll be checking in weekly. We appreciate your patience. Thanks! |
|
Hello @kattni
Thanks for the info. I had hoped that the merge of this PR is going to be my Christmas present :-) But then, this PR is on the way for so long now that a few more weeks won't matter. Tom |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Getting closer; I've left a few notes inline; they're mostly cosmetic, or test strategy fixes. I've cleaned most of those up myself.
At this point, the biggest problem is that it doesn't appear to do anything on Android. Clicking the buttons to navigate to a new URL aren't triggering any dialogs or other handlers. I'm not sure I understand how it can do anything - as I've flagged before, it doesn't makes sense to me that explicitly setting the URL doesn't trigger another navigation handler request - and in my testing, that seems to be because navigation request handling isn't being triggered at all.
I also haven't had a chance to test this on Windows yet.
| '.webview_static_proxy") missing in pyproject.toml section ' | ||
| '"build_gradle_extra_content". on_navigation_starting ' | ||
| "handler is therefore not available" | ||
| ) |
There was a problem hiding this comment.
This error should only be raised if the problem actually exists - if you're not attempting to use on_navigation_handler, you don't need to modify the configuration.
| if not allow: | ||
| return True |
There was a problem hiding this comment.
if not bool: return bool is code that is calling out for simplification.
core/src/toga/widgets/webview.py
Outdated
| **Note:** On Android, this handler needs `chaquopy.defaultConfig.staticProxy( | ||
| "toga_android.widgets.webview_static_proxy")` in the build_gradle_extra_content | ||
| section of pyproject.toml |
There was a problem hiding this comment.
This is the wrong place for this of note - it should either be in the platform quirks for the widget, or on an API that is specifically related to the feature.
There was a problem hiding this comment.
As a cosmetic thing, it would also be worth adding to the Briefcase Android template (commented out) so that it's there as an easy addition.
core/tests/widgets/test_webview.py
Outdated
| "set content", | ||
| root_url="", | ||
| content="<h1>Fancy page</h1>", | ||
| ) |
There was a problem hiding this comment.
This test isn't really adding anything.
core/tests/widgets/test_webview.py
Outdated
| assert widget.on_navigation_starting is None | ||
| finally: | ||
| # Clear the feature attribute. | ||
| del DummyWebView.SUPPORTS_ON_NAVIGATION_STARTING |
There was a problem hiding this comment.
This is what monkeypatch is for.
| del DummyWebView.SUPPORTS_ON_NAVIGATION_STARTING | ||
|
|
||
|
|
||
| def test_navigation_starting_no_handler(widget): |
There was a problem hiding this comment.
This should be a parameterized test.
| if url == "https://beeware.org": | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
There's no need for this complication - an async handler is an async handler.
@freakboy3742 Don't you get a QuestionDialog in the WebView example on Android when you try to navigate to the Toga tutorial page?? |
|
@t-arn I've realized that there's a slim possibility where the impl object could be destroyed but the native object is still alive and emitting final events to the handlers... I'm working on fixing the memory leaks in #4016, so keep in mind you may need to update how the weakref things are handled after #4016's design is finalized. @freakboy3742 I am able to reproduce the behavior, however, since there's updates to the app template, if you've previously ran the example, you may need to remove and recreate the Android build of the app. |
🤦 Ok - that's on me. I was pressing buttons and not seeing any dialogs appear; I now see that I needed to actually navigate on the page for the dialog to be triggered. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Once I worked out what I was actually testing (🤦), and I cleaned up the errors I introduced last week (🤦) this worked as expected.
I've left a few notes, but I've cleaned those up for you as penance for my mistakes :-) I've also added an implementation for iOS and macOS.
core/src/toga/widgets/webview.py
Outdated
|
|
||
|
|
||
| class OnNavigationStartingHandler(Protocol): | ||
| def __call__(self, widget: WebView, url: str, **kwargs: Any) -> True: |
There was a problem hiding this comment.
| def __call__(self, widget: WebView, url: str, **kwargs: Any) -> True: | |
| def __call__(self, widget: WebView, url: str, **kwargs: Any) -> bool: |
|
|
||
|
|
||
| async def test_on_navigation_starting_sync(widget, probe, on_load): | ||
| skip_on_platforms("iOS", "macOS", "linux") |
There was a problem hiding this comment.
We try to keep these kinds of decisions on the probe, so that we don't need to keep explicit support lists on each test.
There was a problem hiding this comment.
Better still - the widget itself has a property, so it doesn't need to be duplicated on the probe.
| await widget.evaluate_javascript( | ||
| 'window.location.assign("https://github.com/beeware/toga/")' | ||
| ) | ||
| await asyncio.sleep(2) |
There was a problem hiding this comment.
We use probe.redraw() here so that in slow mode, we get a log of what the test is trying to do.
| skip_on_platforms("iOS", "macOS", "linux") | ||
|
|
||
| def handler(widget, **kwargs): | ||
| url = kwargs.get("url", None) |
There was a problem hiding this comment.
We can enforce the existence of the url argument here.
| allow = True | ||
| if not url.startswith("https://beeware.org/"): | ||
| allow = False | ||
| return allow |
There was a problem hiding this comment.
Or...
| allow = True | |
| if not url.startswith("https://beeware.org/"): | |
| allow = False | |
| return allow | |
| return url.startswith("https://beeware.org/"): |
| content=ANY, | ||
| on_load=on_load, | ||
| ) | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
This one shouldn't be needed; assert_content_change() waits for the page load.
| async def simulated_question_dialog(url): | ||
| await asyncio.sleep(1) | ||
| allow = True | ||
| if not url.startswith("https://beeware.org/"): | ||
| allow = False | ||
| return allow |
There was a problem hiding this comment.
This complication isn't needed -an async handler is an async handler.
|
@freakboy3742 Thank you for finishing up this PR! I'm glad it is finally done and I can move on to other things :-) |
4 participants
This PR aims to implement URL filtering for WebView on Windows and Android.
Possible use cases are:
PR Checklist: