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.

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

Fixed displaying Cesium Ion icon in WebView #8758

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Fixed displaying Cesium Ion icon in WebView #8758

merged 2 commits into from
Apr 30, 2020

Conversation

vividos
Copy link
Contributor

@vividos vividos commented Apr 15, 2020

When running in a WebView on Android, iOS or UWP WebView, the base URL either starts with file:/// or ms-appx-web:///. When the Credits object for Cesium Ion is created, the <img> tag contains a src attribute with this base URL. The integrated DOMpurify strips all src tags which doesn't contain http or https URLs. This fix changes the src attribute to contain an absolute Url in that case.

This PR doesn't need my other try at fixing the Ion icon issue, PR #8669, as it would also work without it.

Fixes #8665.

@cesium-concierge
Copy link

Thanks for the pull request @vividos!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@vividos
Copy link
Contributor Author

vividos commented Apr 21, 2020

@OmarShehata Could you please take a look at this PR and test it with your Cordova test app? Also there is an error from the Prettier tool that I tried to fix, but unfortunately the tool doesn't exactly say what the issue is. Thanks!

@vividos
Copy link
Contributor Author

vividos commented Apr 24, 2020

@OmarShehata I finally got it working with Prettier not reporting any issues now.

@vividos
Copy link
Contributor Author

vividos commented Apr 28, 2020

@mramato @lilleyse Maybe you could take a look at this PR and consider it for the next version? I'll rebase it on the latest master. Thanks!

@OmarShehata
Copy link
Contributor

@vividos thanks for continuing to investigate this. I can confirm this PR fixes the issue on Android with Cordova and makes the Cesium ion logo correctly appear.

@mramato this is the correct approach as far as I'm aware but would love to get a second opinion when you have a chance to take a look. This requires a change in ThirdParty code that we're using to parse URLs which is producing incorrect behavior as I mentioned here: #8669 (comment)

For what to do with this PR, @mramato the behavior for parsing URIs with file:/// (three slashes, no authority or hostname) is incorrect in master, and is fixed in this PR. Sandcastle with test case here #8665 (comment).

I'm not aware of a way to fix this without modifying the third party code, since ThirdParty/Uri.js incorrectly skips the // when the authority and hostname are blank. I would merge this after unit tests are added for it but wanted to get a second opinion here.

@vividos
Copy link
Contributor Author

vividos commented Apr 30, 2020

@OmarShehata A short comment from me, this PR only changes CreditDisplay.js, where the <img src is specified. Using an absolute path, without file:// prevents the DOMPurify to remove the src attribute altogether.

The other PR from me, #8669, is not necessary to fix the Ion icon, and that PR modifies the third-party code.

@OmarShehata OmarShehata merged commit 757ee68 into CesiumGS:master Apr 30, 2020
@OmarShehata
Copy link
Contributor

Oh great! My bad for missing that @vividos . This change looks good to me then, and perhaps we can revisit the ThirdParty/Url.js fix when we have more time.

Congrats on your first merged contribution to CesiumJS Michael!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cesium ion icon missing, or handling file:// base URL
4 participants