Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] fix: unreliable encoding for web #5737

Merged
auto-submit merged 19 commits into from
Jul 6, 2022
Merged

[webview_flutter] fix: unreliable encoding for web #5737

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f8e51ad
fix: unreliable encoding
FlafyDev May 14, 2022
e9dfa1f
chore: update version and changelog
FlafyDev May 14, 2022
e0b13de
revert changelog formatting
FlafyDev May 14, 2022
0b61f9c
update tests to use new encoding
FlafyDev May 26, 2022
99b5c76
correct new test
FlafyDev Jun 7, 2022
5781562
fix unnecessary `Encoding.getByName`
FlafyDev Jun 7, 2022
397f609
change changelog
FlafyDev Jun 7, 2022
42bf109
revert changelog autoformat
FlafyDev Jun 7, 2022
db3c42a
Merge branch 'flutter:main' into webview_flutter_web_encoding
FlafyDev Jun 8, 2022
5d9e7c1
revert deleted test
FlafyDev Jun 17, 2022
15deea5
fix loadHtmlString test
FlafyDev Jun 17, 2022
b760936
change verify to be more explicit and fit simillar ones
FlafyDev Jun 18, 2022
d2704ed
group loadHtmlString tests
FlafyDev Jun 18, 2022
7df9cfa
loadRequest escapes # correctly test
FlafyDev Jun 18, 2022
95fd76b
change loadRequest escapes # correctly test to text/html
FlafyDev Jun 18, 2022
d62c793
fix loadRequest test
FlafyDev Jun 18, 2022
c1c4884
remove unused import
FlafyDev Jun 22, 2022
61c0028
change # tests to verify with contains instead
FlafyDev Jun 22, 2022
902e8f3
convert double quotes to single quotes
FlafyDev Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/webview_flutter/webview_flutter_web/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.1.0+4

* Fixes unreliable encoding of HTML to the iframe element.

## 0.1.0+3

* Minor fixes for new analysis options.
Expand Down
14 changes: 11 additions & 3 deletions packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:html';
import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart';
Expand Down Expand Up @@ -183,7 +184,11 @@ class WebWebViewPlatformController implements WebViewPlatformController {
String? baseUrl,
}) async {
// ignore: unsafe_html
_element.src = 'data:text/html,${Uri.encodeFull(html)}';
_element.src = Uri.dataFromString(
html,
mimeType: 'text/html',
encoding: Encoding.getByName('utf-8'),
).toString();
}

@override
Expand All @@ -199,8 +204,11 @@ class WebWebViewPlatformController implements WebViewPlatformController {
final String contentType =
httpReq.getResponseHeader('content-type') ?? 'text/html';
// ignore: unsafe_html
_element.src =
'data:$contentType,${Uri.encodeFull(httpReq.responseText ?? '')}';
_element.src = Uri.dataFromString(
httpReq.responseText ?? '',
mimeType: contentType,
encoding: Encoding.getByName('utf-8'),
).toString();
}

@override
Expand Down
2 changes: 1 addition & 1 deletion packages/webview_flutter/webview_flutter_web/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: webview_flutter_web
description: A Flutter plugin that provides a WebView widget on web.
repository: https://github.com/flutter/plugins/tree/main/packages/webview_flutter/webview_flutter_web
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+webview%22
version: 0.1.0+3
version: 0.1.0+4

environment:
sdk: ">=2.14.0 <3.0.0"
Expand Down
14 changes: 11 additions & 3 deletions packages/webview_flutter/webview_flutter_web/test/webview_flutter_web_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:convert';
import 'dart:html';
import 'dart:typed_data';

Expand Down Expand Up @@ -64,7 +65,11 @@ void main() {
// Run
controller.loadHtmlString('test html');
// Verify
verify(mockElement.src = 'data:text/html,${Uri.encodeFull('test html')}');
verify(mockElement.src = Uri.dataFromString(
'test html',
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g May 31, 2022

Choose a reason for hiding this comment

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

These test cases don't actually exercise the problem described in your issue. Would either of these even fail if you reverted the non-test changes?

Copy link
Contributor Author

@FlafyDev FlafyDev Jun 6, 2022

Choose a reason for hiding this comment

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

The tests wouldn't pass before this PR's changes because instead of starting the src's value with data:text/html, it now starts with data:text/html;charset=utf-8.

It would be more accurate for the test to only check if the mockElement.src starts with data:text/html;charset=utf-8, I guess. But I don't know how to do it with mock tests or if it's even necessary.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 6, 2022

Choose a reason for hiding this comment

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

That test failure would have nothing to do with the issue you fixed. I could make these tests pass without fixing the bug by just adding ;charset=utf-8 into the interpolated string.

The issue you are fixing is about properly escaping special characters--specifically # in your examples--so that's what the test should be verifying.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 6, 2022

Choose a reason for hiding this comment

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

(In fact, the changelog should say "incorrect escaping", not "unreliable encoding", since this is about escaping rather than encoding.)

Copy link
Contributor Author

@FlafyDev FlafyDev Jun 6, 2022

Choose a reason for hiding this comment

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

You're right I'll fix the test to include the new escaping. But should it be renamed to only fix escaping even though it adds the ;charset=utf-8?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 6, 2022

Choose a reason for hiding this comment

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

If you want to also mention adding explicit encoding as a secondary change, feel free. But there have been zero reports of encoding issues so it's not particularly important to mention.

mimeType: 'text/html',
encoding: Encoding.getByName('utf-8'),
).toString());
});

group('loadRequest', () {
Expand Down Expand Up @@ -122,8 +127,11 @@ void main() {
requestHeaders: <String, String>{'Foo': 'Bar'},
sendData: Uint8List.fromList('test body'.codeUnits),
));
verify(
mockElement.src = 'data:text/plain,${Uri.encodeFull('test data')}');
verify(mockElement.src = Uri.dataFromString(
'test data',
mimeType: 'text/plain',
encoding: Encoding.getByName('utf-8'),
).toString());
});
});
});
Expand Down