Skip to content

[HybridWebView] Fix some issues with the interception, typescript and other features - #29829

Merged
rmarinho merged 4 commits into
net10.0from
dev/hybridwebview-header-case
Jun 5, 2025
Merged

rmarinho merged 4 commits into
net10.0from
dev/hybridwebview-header-case

Conversation

@mattleibow

@mattleibow mattleibow commented Jun 4, 2025

Copy link
Copy Markdown
Member

This pull request introduces several improvements and fixes to the HybridWebView component, focusing on case-insensitive header handling, improving compatibility across platforms, refactoring JavaScript code, and enhancing testing. The most significant changes include updates to header dictionaries to support case-insensitivity, refactoring the JavaScript code for better readability and maintainability, and adding platform-specific compatibility checks in tests.

Case-insensitive header handling:

JavaScript code refactoring:

  • src/Core/src/Handlers/HybridWebView/HybridWebView.js: Refactored JavaScript functions such as SendRawMessage, InvokeDotNet, and __InvokeJavaScript into standalone functions, and restructured the HybridWebView object for better readability and maintainability. Removed optional chaining for compatibility. [1] [2] [3] [4]

Platform-specific compatibility in tests:

TypeScript updates:

These changes collectively enhance the HybridWebView component's functionality, improve cross-platform compatibility, and ensure robust testing practices.

Android: Observed lower case on some OS versions (ie 28)
Windows: Header lookup is case insensitive
macOS/iOS: CI will inform
Copilot AI review requested due to automatic review settings June 4, 2025 21:26
@mattleibow
mattleibow requested a review from a team as a code owner June 4, 2025 21:26
@mattleibow
mattleibow requested review from PureWeen and rmarinho June 4, 2025 21:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new test case to verify that the HybridWebView can intercept requests and read headers in a case-insensitive manner, addressing issues observed on various OS versions.

  • Added a new test method RequestsCanBeInterceptedAndCaseInsensitiveHeadersRead.
  • Incorporated conditional test data for different platforms based on header handling capabilities.
Comments suppressed due to low confidence (1)

src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs:771

  • [nitpick] Consider refactoring the repetitive try-catch blocks used for header value retrieval into a helper function or loop to reduce duplication and improve maintainability.
try { headerValues["X-Echo-Name"] = e.Headers["X-Echo-Name"]; }

@mattleibow

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow changed the title [HybridWebView] Sometimes the OS changes the header casing [HybridWebView] Fix some issues with the interception, typescript and other features Jun 5, 2025
Comment on lines -56 to -58
internal IReadOnlyDictionary<string, string>? GetRequestHeaders() => new WrappedHeadersDictionary(Request.Headers);

class WrappedHeadersDictionary : IReadOnlyDictionary<string, string>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The previous way to use wrappers was me being "so clever" and "avoiding allocations" and "reducing copies"... But yeah, that was not sensible since the amount of things that would happen was so little. In order to hit this path, the user has to actually request the headers.

For all other cases, this doesn't do anything.

Comment on lines +185 to +188
internal IReadOnlyDictionary<string, string> GetRequestHeaders() =>
_headers ??= Request.RequestHeaders is { } rh
? new Dictionary<string, string>(rh, StringComparer.OrdinalIgnoreCase)
: new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately, depending on the server and how the headers are populated, Android sometimes converts the headers to lower case. So we always will ned a case-insensitive.

Windows was already case insensitive, so this just makes it all consistent.

Comment thread src/Core/src/Core.csproj Outdated

@rmarinho rmarinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests now pass!

@rmarinho rmarinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Talked too soon , seems still have some failure on API24, but it doesn t crash anymore

Screenshot 2025-06-05 at 11 04 04

}
// Determine the mechanism to receive messages from the host application.
if (window.chrome?.webview?.addEventListener) {
if (window.chrome && window.chrome.webview && window.chrome.webview.addEventListener) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The Android Chromeis weird. It suports async functions ahead of its time, but then also does not support ?.. I am at a loss.

@rmarinho

rmarinho commented Jun 5, 2025

Copy link
Copy Markdown
Member

/backport to release/10.0.1xx-preview5

@github-actions

github-actions Bot commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0.1xx-preview5: https://github.com/dotnet/maui/actions/runs/15473805094

@rmarinho rmarinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok seems we are green now! Thanks @mattleibow

@rmarinho

rmarinho commented Jun 5, 2025

Copy link
Copy Markdown
Member

I backported but I also have this branch to just bring latest changes to the release branch .. go wild or go home

@rmarinho
rmarinho merged commit e79ada6 into net10.0 Jun 5, 2025
@rmarinho
rmarinho deleted the dev/hybridwebview-header-case branch June 5, 2025 18:47
@mattleibow

Copy link
Copy Markdown
Member Author

/backport to main

@github-actions

github-actions Bot commented Jun 6, 2025

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jun 6, 2025

Copy link
Copy Markdown
Contributor

@mattleibow backporting to "main" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Sometimes the OS changes the header casing
Using index info to reconstruct a base tree...
M	src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
CONFLICT (content): Merge conflict in src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Sometimes the OS changes the header casing
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions github-actions Bot locked and limited conversation to collaborators Jul 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants