-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Fix embed webview endcoding #50555
Conversation
@jhnstn It's not clear to me the error case that this PR is fixing, could you add testing instructions to the PR in order to help us review the PR? Thanks 🙇 ! |
@@ -80,7 +77,7 @@ public void onProgressChanged(WebView view, int progress) { | |||
|
|||
protected void load() { | |||
String content = getIntent().getExtras().getString(ARG_CONTENT); | |||
mWebView.loadData(content, "text/html", "UTF-8"); | |||
mWebView.loadDataWithBaseURL(null, content, "text/html", "UTF-8", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation of loadData
mentions that loadDataWithBaseURL
should be used when you need to identify the main frame's origin in a trustworthy way. I haven't found references to loadDataWithBaseURL
relate to the encoding, so I'm wondering @jhnstn if you could share more info about the encoding and this function to have a better context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing @fluiddot
Real quick, the root issue is the encoding. Specifically I found that #
symbols properly encoded as %23
is the content
resulted in #
in the WebView
.
( There is a note about #
characters in the loadData
docs but that was not the issue I was seeing. )
I found quite a few references to folks having to use loadDataWithBaseUrl
to fix encoding issues. I've tried many of the options I could find to get loadData
to encode properly but nothing worked. This seemed to follow most of the observations from this SO post. There are a few workarounds but seems like loadDataWithBaseUrl
is the most straight forward and stable solution, here is a good example.
Then there is this issue on Google which has the status won't fix . There too folks recommend using loadDataWithBaseUrl
as the best option. Someone also mentions that the documentation of loadData
is misleading.
So I can't explain why loadData
is not working based on the documentation and I haven't looked into the source code to see what's the difference between the two functions regarding encoding. But from what I've read from others with the same issue and by what I've tried with loadData
I'm convinced that this is an appropriate fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jhnstn for elaborating on the solution, I really appreciate it 🙇. It's interesting how loadData
and loadDataWithBaseUrl
functions behave so differently in terms of encoding, probably as you pointed out, there's something in the source code that processes the encoding in a different way (probably better 😅).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debugged the WebView using each function and confirmed the issue:
iframe
source URL withloadData
❌ :
https://video.wordpress.com/embed/<VIDEOPRESS_GUID>?cover=1&loop=1&preloadContent=metadata&sbc=
Note how the URL stops at the seek bar color query param (sbc
).
iframe
source URL withloadDataWithBaseUrl
✅ :
https://video.wordpress.com/embed/<VIDEOPRESS_GUID>?cover=1&loop=1&preloadContent=metadata&sbc=%23f78da7&hd=1&metadata_token=<TOKEN>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎊 ! Tested using the instructions from wordpress-mobile/WordPress-Android#18411.
What?
Fixes issues with encoded characters with the native Android WebView
Why?
Decoding encoded characters can lead to unexpected behavior
How?
Switching from
WebView.loadData
toWebView.loadDataWithBaseUrl
fixes the encodingTesting Instructions
The core embed is not currently being used on any core embed blocks so to test:
Testing Instructions for Keyboard
Screenshots or screencast