[flutter_inappwebview] Fix navigation/getUrl/scroll bugs and add integration tests - #1083
[flutter_inappwebview] Fix navigation/getUrl/scroll bugs and add integration tests#1083seungsoo47 wants to merge 8 commits into
Conversation
seungsoo47
commented
Aug 12, 2026
seungsoo47
commented
- Fix onTitleChanged to fire on later title changes (e.g. via JS document.title), not just once after load.
- Fix a getUrl() race with cancelled navigations, and skip the shouldOverrideUrlLoading round-trip for app-initiated navigations (loadUrl, goBack, reload, etc.).
- Fix scrollBy/getScrollX/getScrollY occasionally returning a stale position right after scrollTo/scrollBy (EWK applies scroll asynchronously).
- Add Tizen integration tests ported from upstream flutter_inappwebview v6.1.5.
- Bump flutter_inappwebview_tizen to 0.2.0.
…age load The Tizen implementation only reported the page title once, right after a page finished loading. It never listened for the WebView's own title-changed notifications, so title updates made afterwards (for example by JavaScript setting document.title) were never reported to onTitleChanged. Register a "title,changed" listener on the underlying webview instance, matching the pattern already used for load and navigation events, so onTitleChanged fires whenever the title actually changes.
…ound-trip on programmatic navigation OnNavigationPolicy always suspended the view and asked Dart's shouldOverrideUrlLoading whether to allow a navigation, even for navigations the app itself requested (loadUrl, goBack, reload, ...). That round-trip is meant for user/page-initiated navigation only. Also, when Dart calls stopLoading() to cancel a pending navigation, getUrl() had no way to know a cancellation happened: EWK's "url,changed" event can still fire for the cancelled URL (before or after ewk_view_stop() takes effect), so getUrl() could end up reporting a URL the app never actually finished navigating to. Fix both: - Every EWK call that starts an app-requested navigation now goes through NavigateProgrammatically(), which marks the navigation as programmatic. OnNavigationPolicy checks this flag and accepts immediately, skipping the shouldOverrideUrlLoading round-trip for it. - StopNavigation() records that the current navigation was cancelled and reverts committed_url_ to the URL snapshotted just before the navigation decision was accepted (pending_navigation_revert_url_). OnUrlChange ignores "url,changed" while a cancellation is pending, and getUrl() returns committed_url_ instead of asking EWK directly in that window.
…tale position ewk_view_scroll_pos_get() right after ewk_view_scroll_set() can return the pre-scroll position because EWK applies the scroll asynchronously, so scrollBy's delta and getScrollX/getScrollY's return value were sometimes stale by one frame. Track the last requested scroll position in target_scroll_x_/y_ and use it as the source of truth until EWK's reported position catches up with it, then fall back to querying EWK directly. Reset both to -1 on navigation start/error since a new page invalidates any pending scroll target.
Add Tizen-compatible test cases derived from the upstream
flutter_inappwebview v6.1.5 integration test suite, covering the parts
of the API that the Tizen implementation actually supports (the
InAppWebView widget/controller and CookieManager.deleteAllCookies).
Most of upstream's suite exercises features this plugin does not
implement (in-app browser, Chrome Custom Tabs, headless webview, find
interaction, service worker, proxy, tracing, process-global config,
the local asset-loader server, and most Android/iOS-only settings and
callbacks), so those tests don't apply here and were left out.
New test cases, alongside the 4 already in the file:
- getProgress reports 100 once the page finishes loading
- reload reloads the currently displayed page
- loadUrl navigates to a new URL
- postUrl and loadUrl submit an HTTP POST request body
- loadFile loads a bundled asset file
- programmatic scroll updates and reports the scroll position
- onScrollChanged fires when the scroll position changes
- onTitleChanged fires when document.title changes
- stopLoading interrupts an in-flight page load
- clearAllCache completes without throwing
- zoomBy triggers onZoomScaleChanged
- onReceivedError reports a host lookup failure / is not raised for a
successful load
- setSettings applies updated webview settings
The new tests reuse the file's existing local HTTP server fixture
instead of upstream's live external URLs, so they stay reliable on a
TV emulator or device without depending on outside network resources.
A small bundled HTML asset was added for the loadFile case.
Making the onTitleChanged test pass required fixing a gap in the
plugin itself (separate commit): it only reported the title once,
right after a page finished loading, and never listened for later
title changes such as JavaScript setting document.title.
Validated with `flutter-tizen drive` on a Raspberry Pi device (all 17
cases pass). flutter_inappwebview is currently marked disabled for
the TV emulator profile in .github/recipe.yaml because of a separate,
unrelated crash on WebView disposal there; that is out of scope for
this change.
The postUrl/loadUrl body assertions poll for the expected text via
_waitForCondition instead of reading document.querySelector('p')
immediately, since the page's DOM update after a POST/navigation
isn't synchronous with the awaited call and the immediate read was
occasionally flaky.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bb2e06498
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
goBack/goForward always reported success to NavigateProgrammatically regardless of whether ewk_view_back()/ewk_view_forward() actually had history to navigate. When called with no history, no navigation policy callback ever fires to clear is_programmatic_navigation_, so the flag leaks into the next user-initiated navigation and incorrectly skips shouldOverrideUrlLoading. Use the EWK calls' own return value instead. getScrollX/getScrollY kept substituting the requested scrollTo/scrollBy target for the actual position until they matched, to mask EWK applying scroll asynchronously. If the requested position is beyond the page's max scroll extent, EWK clamps it and the actual position never matches the target, so out-of-range coordinates were reported indefinitely. Mask only the single read immediately following a scroll instead. Found by chatgpt-codex-connector's review on PR flutter-tizen#1083.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7dcbb1442
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…getUrl after loadData The XF86Back (remote/hardware Back key) handler wrapped ewk_view_back() in NavigateProgrammatically, marking it as a programmatic navigation. OnNavigationPolicy takes the early-accept path for programmatic navigations and never calls shouldOverrideUrlLoading, so apps could not intercept or block a user-initiated Back-key navigation even with useShouldOverrideUrlLoading enabled. Call ewk_view_back() directly so it goes through the normal navigation-policy path, matching goBack() being the only case that should bypass the delegate. is_navigation_cancelled_ (set by StopNavigation() when a delegate cancels a navigation) is only cleared by OnNavigationPolicy. loadData() calls ewk_view_html_string_load(), which never triggers OnNavigationPolicy, so calling loadData() after a cancelled navigation left the flag stuck and getUrl() kept returning the pre-cancellation URL even though new content had loaded. Clear the flag before the html_string_load call. Found by chatgpt-codex-connector's review on PR flutter-tizen#1083.
| if (target_scroll_x_ >= 0) { | ||
| x = target_scroll_x_; | ||
| target_scroll_x_ = -1; | ||
| } | ||
| if (target_scroll_y_ >= 0) { | ||
| y = target_scroll_y_; | ||
| target_scroll_y_ = -1; | ||
| } |
There was a problem hiding this comment.
The getScrollX/getScrollY handler clears both axis targets on a single call. After scrollTo(30, 40), calling getScrollX() also resets target_scroll_y_, so an immediately following getScrollY() reads EWK's not-yet-applied stale position — reintroducing the exact symptom
this PR fixes.
| void WebView::OnTitleChange(void* data, Evas_Object* obj, void* event_info) { | ||
| WebView* webview = static_cast<WebView*>(data); | ||
| const char* title = static_cast<const char*>(event_info); | ||
| if (!title) { | ||
| return; | ||
| } | ||
| flutter::EncodableMap args = { | ||
| {flutter::EncodableValue("title"), flutter::EncodableValue(title)}}; | ||
| webview->webview_channel_->InvokeMethod( | ||
| "onTitleChanged", std::make_unique<flutter::EncodableValue>(args)); | ||
| } |
There was a problem hiding this comment.
Is it guaranteed that onTitleChanged is called exactly once during the page load process?
Adding the title,changed callback while keeping the existing onTitleChanged emission in OnLoadFinished makes the event fire twice with the same title on a normal page load (once via title,changed during load, once at load,finished).
| bool disposed_ = false; | ||
| Ewk_Mouse_Button_Type mouse_button_type_ = (Ewk_Mouse_Button_Type)0; | ||
| bool is_programmatic_navigation_ = false; | ||
| bool is_navigation_cancelled_ = false; |
There was a problem hiding this comment.
It looks that is_navigation_cancelled_ can be replaced by utilizing committed_url_. Currently, is_navigation_cancelled_ looks to be an unnecessary member.
| bool WebView::NavigateProgrammatically(const std::function<bool()>& ewk_call) { | ||
| is_programmatic_navigation_ = true; | ||
| const bool started = ewk_call(); | ||
| if (!started) { | ||
| is_programmatic_navigation_ = false; | ||
| } | ||
| return started; | ||
| } |
There was a problem hiding this comment.
um.. I am not sure if "NavigateProgrammatically" or "Programmatic" is an appropriate name for the current behavior.
And, there must be a case where a page load is called, and another page or back/forward request is made before the page load is complete. Please check if this case is also blocked.
There was a problem hiding this comment.
Confirmed — this can indeed be blocked incorrectly.
is_programmatic_navigation_ is a single shared flag, so it can't tell which in-flight navigation a given policy,navigation,decide / load,started / load,finished / load,error event actually belongs to. If a second programmatic call (e.g. goBack()) arrives while an earlier one (e.g. loadUrl()) is still in flight, the abandoned first load's load,error can clear the flag before the second call's own policy decision arrives, so that second, genuinely-programmatic navigation ends up routed through shouldOverrideUrlLoading as if it were user-initiated (the app could be asked to approve its own goBack()). The reverse case — a real user click landing while a programmatic flag is still pending — is possible for the same reason.
A clean fix needs a per-navigation correlation id, which the EWK callbacks don't provide, so neither a bool nor a simple counter can fully disambiguate which event belongs to which call without deeper engine-side cooperation. I'd like to land the other fixes from this review as-is and track this specific race as a known limitation / follow-up issue rather than block this PR on it — let me know if you'd rather address it here instead.
On naming: leaving NavigateProgrammatically / is_programmatic_navigation_ as-is for now since I don't have a clearly better name that captures the caveat above; happy to revisit if you have a suggestion.
| - Fix `onTitleChanged` to also fire when the page's title changes after the | ||
| initial load (e.g. when JavaScript updates `document.title`), instead of | ||
| only once when loading finishes. | ||
| - Fix a race where `getUrl()` could return the URL of a navigation that was | ||
| cancelled via `shouldOverrideUrlLoading`, and skip the | ||
| `shouldOverrideUrlLoading` round-trip for app-initiated navigations | ||
| (`loadUrl`, `goBack`, `reload`, etc.). | ||
| - Fix `scrollBy`/`getScrollX`/`getScrollY` occasionally returning a stale | ||
| scroll position right after `scrollTo`/`scrollBy`. |
There was a problem hiding this comment.
Most md files do not use this type of line break. Also, the modifications must be explained briefly. Detailed explanations should be provided in the body of the PR.
| // Snapshot the URL EWK is displaying before accepting the navigation | ||
| // below. EWK can fire "url,changed" for the new (possibly-to-be-cancelled) | ||
| // URL as soon as ewk_policy_decision_use() runs, racing with the async | ||
| // shouldOverrideUrlLoading round-trip. StopNavigation() reverts getUrl() | ||
| // using this snapshot rather than whatever "url,changed" reported last, so | ||
| // that race can't leave getUrl() stuck on a cancelled URL. |
There was a problem hiding this comment.
I think it is an unnecessary comment.
| if (webview->is_programmatic_navigation_) { | ||
| webview->is_programmatic_navigation_ = false; | ||
| ewk_policy_decision_use(policy_decision); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please explain a little more why "shouldOverrideUrlLoading" should not be called.
| if (!pending_navigation_revert_url_.empty()) { | ||
| committed_url_ = pending_navigation_revert_url_; | ||
| } | ||
| ewk_view_resume(webview_instance_); |
There was a problem hiding this comment.
Why is ewk_view_resume() necessary? ewk_view_stop() is called immediately after ewk_view_resume(). If this is due to internal behavior, you can leave a comment or note with //TODO on this seemingly abnormal code.
| return; | ||
| } | ||
|
|
||
| webview->pending_navigation_revert_url_ = url_before_navigation; |
There was a problem hiding this comment.
Why is it revert_url? This simply contains the previous URL information.
| if (is_navigation_cancelled_ && !committed_url_.empty()) { | ||
| result->Success(flutter::EncodableValue(committed_url_)); | ||
| } else { |
There was a problem hiding this comment.
I believe it should operate according to the internal logic of ewk_view_url_get().
I do not think there is a need to separately save and return the previous URL. If the page is cancelled while loading, I believe it is correct to return that there is no URL, as the display of that page is not normal.
|
I've submitted a new PR based on the main branch. |
2 participants
