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.

Sign up for GitHub

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

Jump to bottom

[webview_flutter] Adds onHttpError callback to NavigationDelegate to catch HTTP error status codes #3278

Closed
wants to merge 21 commits into from
Closed

[webview_flutter] Adds onHttpError callback to NavigationDelegate to catch HTTP error status codes #3278

wants to merge 21 commits into from

Conversation

HugoOlthof
Copy link
Contributor

@HugoOlthof HugoOlthof commented Feb 23, 2023 •
edited
Loading

This PR adds the onHttpError callback to NavigationDelegate to catch HTTP error status codes. In our app we need to catch the status codes to show native error pages/alerts or redirect users to the native login screen if the user is unauthenticated. The existing onWebResourceError is not sufficient.

The Android implementation uses the onReceiveHttpError callback from WebViewClient and the iOS implementation uses the decidePolicyForNavigationResponse delegate from WKWebView.

Fixes flutter/flutter#39502.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@ghost ghost force-pushed the onpageerror-v4 branch 2 times, most recently from cc7b6ad to 50559ae Compare February 25, 2023 19:00
bparrishMines
bparrishMines requested changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I made a first pass through.

packages/webview_flutter/webview_flutter/example/test/main_test.dart Outdated Show resolved Hide resolved
...webview_flutter/webview_flutter_platform_interface/lib/src/platform_navigation_delegate.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter/lib/src/navigation_delegate.dart Outdated Show resolved Hide resolved
...ndroid/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewClientHostApiImpl.java Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFDataConverters.m Show resolved Hide resolved
...webview_flutter/webview_flutter_platform_interface/lib/src/platform_navigation_delegate.dart Outdated Show resolved Hide resolved
...webview_flutter/webview_flutter_platform_interface/lib/src/platform_navigation_delegate.dart Outdated Show resolved Hide resolved
@ditman ditman removed their request for review March 2, 2023 04:14
@ghost ghost force-pushed the onpageerror-v4 branch 2 times, most recently from db91fe9 to 3021c65 Compare March 8, 2023 16:24
@HugoOlthof HugoOlthof changed the title [webview_flutter] Adds onPageError callback to NavigationDelegate to catch HTTP error status codes [webview_flutter] Adds onHttpError callback to NavigationDelegate to catch HTTP error status codes Mar 8, 2023
@HugoOlthof HugoOlthof requested review from bparrishMines and removed request for cyanglaz March 8, 2023 17:19
bparrishMines
bparrishMines reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this! This looks mostly ready. I just had a few more comments about documentation.

This is also missing integration tests for Android, iOS, and the app-facing api. They're located at:
https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart

https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter_wkwebview/example/integration_test/webview_flutter_test.dart

https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter/example/integration_test/webview_flutter_test.dart

The test called onWebResourceError should be a good example for each file. And you should be able to use '$prefixUrl/favicon.ico' as the test url that returns a 404 status code.

I think this is ready for a secondary review since only documentation and integration tests are left.
@cyanglaz can you look over the webview_flutter_wkwebview code as a secondary reviewer?
@stuartmorgan can you look over the platform interface/app-facing code as secondary reviewer?

...oid/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewClientFlutterApiImpl.java Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview_controller.dart Outdated Show resolved Hide resolved
...es/webview_flutter/webview_flutter_platform_interface/lib/src/types/http_response_error.dart Outdated Show resolved Hide resolved
...es/webview_flutter/webview_flutter_platform_interface/lib/src/types/http_response_error.dart Outdated Show resolved Hide resolved
...bview_flutter/webview_flutter_platform_interface/test/platform_navigation_delegate_test.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/webkit_webview_controller.dart Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_android/CHANGELOG.md Outdated Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

@HugoOlthof Are you planning on updating this based on the review feedback above?

@HugoOlthof
Copy link
Contributor Author

@stuartmorgan Yes, I will update the PR as soon as possible!

@ghost ghost force-pushed the onpageerror-v4 branch 2 times, most recently from c1153fb to 390aee0 Compare March 30, 2023 12:28
bparrishMines
bparrishMines approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM! with a handful of nits

After fixing the comments, you can go ahead and create the PR with just the code in webview_flutter_platform_interface.

...es/webview_flutter/webview_flutter_platform_interface/lib/src/types/http_response_error.dart Outdated Show resolved Hide resolved
...es/webview_flutter/webview_flutter_platform_interface/lib/src/types/http_response_error.dart Outdated Show resolved Hide resolved
...es/webview_flutter/webview_flutter_platform_interface/lib/src/types/http_response_error.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/foundation/foundation.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/legacy/web_kit_webview_widget.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/test/webkit_navigation_delegate_test.dart Outdated Show resolved Hide resolved
@HugoOlthof HugoOlthof mentioned this pull request Apr 5, 2023
Merged
11 tasks
@HugoOlthof
Copy link
Contributor Author

This is the PR for webview_flutter_platform_interface: #3645

@HugoOlthof
OnPageError V4
63498b0 
Signed-off-by: Hugo Olthof <hugo@moneybird.com>
@HugoOlthof
Format all changes
e1dfa71
@HugoOlthof
Adds changelogs
3e284c3
@HugoOlthof HugoOlthof mentioned this pull request Apr 12, 2023
Closed
11 tasks
@HugoOlthof
Copy link
Contributor Author

The PR for the platform implementations: #3695

@stuartmorgan stuartmorgan self-requested a review May 8, 2023 19:34
@stuartmorgan
Copy link
Contributor

Update from triage: this is waiting for the sub-PR to complete the review process.

@stuartmorgan
Copy link
Contributor

Update from triage: sub-PR is still in progress.

@stuartmorgan
Copy link
Contributor

Update from triage: Still waiting on #3695

@bparrishMines bparrishMines added the federated: all_changes PR that contains changes for all packages for a federated plugin change label Jul 17, 2023
@Hixie
Copy link
Contributor

Hixie commented Oct 24, 2023

This is blocked on #3695 which is blocked on #4025 which is currently in review.

@stuartmorgan
Copy link
Contributor

Update from triage: #5790 has been created as a copy of the blocking PR, to try to start unwinding this stack.

@stuartmorgan
Copy link
Contributor

Update from triage: the last PR in the stack is resolved, and this is now blocked on getting the next PR over the line.

@bparrishMines bparrishMines mentioned this pull request Feb 21, 2024
Merged
11 tasks
@stuartmorgan
Copy link
Contributor

Update from triage: implementation PR (referenced just above) is still in progress.

auto-submit bot pushed a commit that referenced this pull request Mar 14, 2024
@bparrishMines
[webview_flutter_android][webview_flutter_wkwebview] Adds platform im…
92a8b7a 
…plementations for onHttpError (#6149)

Copy of #3695 since it doesn't contain permission to edit from contributors.

Part of flutter/flutter#39502

Full PR #3278
@bparrishMines bparrishMines mentioned this pull request Mar 23, 2024
Merged
11 tasks
@bparrishMines
Copy link
Contributor

closing in favor of #6378

auto-submit bot pushed a commit that referenced this pull request May 24, 2024
@bparrishMines
[webview_flutter] Adds onHttpError callback to NavigationDelegate to …
40b66fc 
…catch HTTP error status codes (#6378)

This is a copy of #3278 since it doesn't allow for contributor access.

Fixes flutter/flutter#39502
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@bparrishMines @arc-yong
[webview_flutter_android][webview_flutter_wkwebview] Adds platform im…
1322892 
…plementations for onHttpError (flutter#6149)

Copy of flutter#3695 since it doesn't contain permission to edit from contributors.

Part of flutter/flutter#39502

Full PR flutter#3278
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@bparrishMines @arc-yong
[webview_flutter] Adds onHttpError callback to NavigationDelegate to …
3df34cb 
…catch HTTP error status codes (flutter#6378)

This is a copy of flutter#3278 since it doesn't allow for contributor access.

Fixes flutter/flutter#39502
bill-skdnd pushed a commit to skdnd-metaspace/webview_flutter_wkwebview that referenced this pull request Aug 2, 2024
@bparrishMines
[webview_flutter_android][webview_flutter_wkwebview] Adds platform im…
901a08a 
…plementations for onHttpError (#6149)

Copy of flutter/packages#3695 since it doesn't contain permission to edit from contributors.

Part of flutter/flutter#39502

Full PR flutter/packages#3278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparrishMines bparrishMines bparrishMines approved these changes

@stuartmorgan stuartmorgan Awaiting requested review from stuartmorgan

@hellohuanlin hellohuanlin Awaiting requested review from hellohuanlin

Assignees
No one assigned
Labels
federated: all_changes PR that contains changes for all packages for a federated plugin change p: webview_flutter platform-android platform-ios
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[webview_flutter] Catch and handle all HTTP errors codes (eg 404) to redirect to a custom error page
4 participants
@HugoOlthof @stuartmorgan @Hixie @bparrishMines