Skip to content
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: Implement navigation event and cancellation, closes #456 #519

Merged
merged 20 commits into from Mar 18, 2022

Conversation

atlanticaccent
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

  • There are likely code style issues with this
  • This is only fully implemented for Windows

Notable: the design approach taken is to:

  • Make WebContexts generic over user event types so that they can (potentially) store an EventLoopProxy
    • This is non-breaking, as a type alias of WebContextGeneric<()> is provided, named WebContext which effectively hides any generic functionality from consumers that do not need it.
  • WebContextBuilders are necessarily also made generic over the user event type, however this should have minimal impact
  • The EventLoopProxy is utilised to send a user defined event whenever a navigation occurs.
  • An "interface" is effectively defined by WebContextGeneric::with_navigation_event which requires as argument a closure that can construct the user event that is emitted whenever a navigation occurs, thus ensuring that any user defined events also include an event for navigation
  • Navigation events cannot be cancelled in "real time", ie: within the event loop. A lambda must instead be provided that is executed within the navigation event handler itself - what difference this makes is unclear, however it does not appear to be a major drawback from what I have seen
  • A basic example is included - it demonstrates some notable drawbacks, including: not preventing javascript that effectively rewrites a page (most noticeable when navigating to Twitter and then opening any other user's feeds) and it also does not prevent popups (a future PR or enhancement to this one could prevent this, on Windows at least)

@wusyong
Copy link
Member

wusyong commented Mar 7, 2022

I think this is a really great feature to be added!
Both linux and mac have similar event callback iirc.
I can help adding them later on.

Could this just be the config of Webview? Webcontext usually describes shared behaviour (like webdriver setup).
And I think it should let users handle event loop proxy themselves. I believe the proxy can be moved into navigation call backs by users.
This should give more freedom for them.

@atlanticaccent
Copy link
Contributor Author

I could move this code into the webview fairly easily I think, including using the same type aliasing trick to reduce breakage. The reason I picked the WebContext is because it was already being used for things like the webview data directory override, which has a similar execution.

I do think that the EventLoopProxy needs to be explicitly passed in - otherwise the feature is somewhat pointless as a user could write the callback and no event will be emitted. An alternative approach could be to eschew emitting events entirely, and simply take a callback that will be executed when the platform specific event occurs.

No longer make WebContext generic over user event type.
Only take single closure, which defines behaviour to execute when
navigation occurs, and also whether to cancel navigation.
If user wishes to submit an event, they should simply move an event
proxy into the event closure and submit an event through it - see
updated example.
WebViewBuilder will now raise return an error if there is no WebContext
to attach callback to.
@atlanticaccent
Copy link
Contributor Author

I've done some thinking about your suggestions and reworked the feature's implementation.
There WebContext is no longer generic over users events. As you suggested, users should now just move an event proxy into the callback closure.
WebView and WebViewBuilder can both now add the navigation callback.
There is only one callback, which is where users can define behaviour to execute when the event happens, and also whether to cancel the navigation or not - there didn't seem to much point in having the two closures as separate.
I'm still using WebContext to hold the callback, as holding it as a field on the WebView would still mean adding an additional parameter to all platform webview implementations, when webcontexts are already available to the WebView and are passed into all platform implementations.

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far except the handler shouldn't be tied to WebContext at all. Instead, it should be on Webview same as the ipc handler.

@atlanticaccent
Copy link
Contributor Author

I've removed all modifications to WebContext and instead stored the navigation handler in WebViewAttributes

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webview2 implementation Looking good so far except two small comments below.

src/webview/web_context.rs Outdated Show resolved Hide resolved
src/webview/webview2/mod.rs Outdated Show resolved Hide resolved
@atlanticaccent
Copy link
Contributor Author

atlanticaccent commented Mar 9, 2022

0500e74
Implements WebkitGtk navigation callback handling. Tested under WSL so should presumably work everywhere else

@wusyong
Copy link
Member

wusyong commented Mar 12, 2022

Here's the one for macos but I suppose it won't be easy to implement if you are not familiar with c/objc ffi.
https://developer.apple.com/documentation/webkit/wknavigationdelegate/1455641-webview?language=objc
I could do this after I'm done with ios/android support.

@atlanticaccent
Copy link
Contributor Author

I don't really have any experience with objc ffi so if you could lend a hand after the Android port effort that would be great. I'll still try and give it a shot in my own time but I don't expect much

The function is still not called yet. But this is a prove it can work.
@atlanticaccent atlanticaccent requested a review from a team as a code owner March 15, 2022 06:42
@wusyong
Copy link
Member

wusyong commented Mar 15, 2022

Just add the macos and it's working!
Now there are two remaining changes I would like to request:
One is switch the condition on returning bool. I think it's more intuitive for true to allow the url and false for not.
The other is make the callback be Box instead of Rc. Is there a place we still need multiple ownership?

I just need the agreement and I'll help pushing the commits. I know some might be busy on work days.

@amrbashir
Copy link
Member

One is switch the condition on returning bool. I think it's more intuitive for true to allow the url and false for not.

Seems okay but I could see it the other way around too so we gonna have to document this. and If we want more clarity, we can use an enum

enum Navigation {
  Allow,
  Deny
}

The other is make the callback be Box instead of Rc. Is there a place we still need multiple ownership?

Box is fine.

@wusyong wusyong requested a review from amrbashir March 16, 2022 06:21
amrbashir
amrbashir previously approved these changes Mar 16, 2022
@wusyong wusyong merged commit aa8af02 into tauri-apps:dev Mar 18, 2022
@github-actions github-actions bot mentioned this pull request Mar 18, 2022
@wusyong
Copy link
Member

wusyong commented Mar 18, 2022

I'm very happy with the result. Thank you @atlanticaccent for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants