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

Avoid 'use' in SVG due to iOS 13.5.1 bug #8993

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Avoid 'use' in SVG due to iOS 13.5.1 bug #8993

merged 3 commits into from
Jun 30, 2020

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Jun 25, 2020

This is a potential workaround for #8984. All the SVG icons in the animation widget that were not displaying all used use (see https://developer.mozilla.org/en-US/docs/Web/SVG/Element/defs). Re-writing them to embed the SVG elements directly wherever used instead of referencing them with the use tag is one way to work around this.

This PR does this just by moving all the SVG icons from the <defs> and embeds them wherever they are used in the animation widget.

We should not merge this until we investigate the root cause. I thought it was as simple as the use tag just not working in iOS 13.5.1 but the code example from the MDN page runs fine, so perhaps something specific about the way we're using it is triggering this bug.

I don't see any relevant WebKit bugs filed yet for "ios + svg": https://bugs.webkit.org/buglist.cgi?quicksearch=ios%20svg

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ 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.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2020

This solution sounds good to me. All the rest of our widgets add the svgs as elements. This widget is was one of the first one we wrote so it's maybe a little over-engineered compared to what we would do if we re-wrote it today

@TJKoury
Copy link
Contributor

TJKoury commented Jun 26, 2020

I know this might not be the place for it, but does everyone like the widgets I created for Celestrak?

They work fine on 13.5.1.

@OmarShehata
Copy link
Contributor Author

@hpinkos that's a good point, I didn't realize the Animation widget was the very first one. I'll clean the code up today and bump for review.

@TJKoury I do remember seeing that - it's pretty cool! I do remember discussing updating these widgets in CesiumJS (for example, the timeline used in Cesium Stories works a lot better on mobile) but it looks like we don't have an issue open for that. In the meantime I'd love to see a forum thread (https://community.cesium.com/c/general/7) about your widgets work and how others could use them in their apps if they're interested!

@mramato
Copy link
Contributor

mramato commented Jun 30, 2020

I both confirmed the original issue and the fact that this fixes it. Since iOS is pretty important overall, I'm all for merging this for the release tomorrow. While obviously it's irksome to not know the root cause, this is isolated to the animation widget, so not a big deal.

@OmarShehata
Copy link
Contributor Author

I meant to get to this last night - I'm going to do another quick pass to see how the other widgets are implemented and mark this as ready later today.

@OmarShehata OmarShehata marked this pull request as ready for review June 30, 2020 17:02
@OmarShehata
Copy link
Contributor Author

Ok this is ready. @hpinkos I just simplified the code a bit by moving the svg icon definitions into a dictionary. I re-confirmed on my iPad it works with the hosted branch: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/ios-workaround/Apps/Sandcastle/index.html.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 30, 2020

Looks good, thanks @OmarShehata!

@hpinkos hpinkos merged commit ee6857f into master Jun 30, 2020
@hpinkos hpinkos deleted the ios-workaround branch June 30, 2020 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants