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

work-around dimensionless SVG in Firefox #9191

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

nmschulte
Copy link
Contributor

Attempts to resolve #9188.

@cesium-concierge
Copy link

Thanks for the pull request @nmschulte!

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

@nmschulte nmschulte force-pushed the fix-firefox-dimensionless-svg-image branch 2 times, most recently from 185320b to a8bdf0f Compare October 7, 2020 17:13
@nmschulte
Copy link
Contributor Author

Added unit tests. Chromium was using a default dimension of 300x150 if there is no viewBox, so mirrored those defaults. These default values affect the rasterizing/aliasing in the image, which is a potential issue with this work-around. However, I think a blocky image is better than no image in these instances.

@nmschulte nmschulte force-pushed the fix-firefox-dimensionless-svg-image branch from a8bdf0f to 5ddca05 Compare October 15, 2020 16:32
@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

4 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

3 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@ggetz
Copy link
Contributor

ggetz commented Mar 10, 2022

Hi @nmschulte, sorry for the delay in getting this reviewed. Was the work around in #9188 (comment) enough for your use case? If not and you have the time to update this PR, let us know.

@nmschulte
Copy link
Contributor Author

@ggetz I had developed a work-around for my use-case before ever distilling this information for the team and community to digest here.

There are still open questions in my response to the supposed work-around you reference; I don't think it is a general solution in line with the API CesiumJS is putting forward, no. Combining my last remarks with my first, there is a solution to be had, the team just needs to review the situation and make a decision. "Not a bug" would surely be a disappointing resolution in my opinion.

@ggetz
Copy link
Contributor

ggetz commented Mar 10, 2022

I do see this is related to an open issue in Firefox-- https://bugzilla.mozilla.org/show_bug.cgi?id=700533-- And they seem hesitant to call this a straightforward defect, although they do admit that the behavior is different from Chrome and Edge.

From a consistency standpoint at least, I think these changes here makes sense. If you don't mind updating your branch with the latest main and linking to the Firefox bug directly in your comment, then we can move forward with this. Thanks for your patience.

@nmschulte nmschulte force-pushed the fix-firefox-dimensionless-svg-image branch 4 times, most recently from c635ed2 to bd314ee Compare March 11, 2022 03:28
@nmschulte nmschulte force-pushed the fix-firefox-dimensionless-svg-image branch from bd314ee to c03ea38 Compare March 11, 2022 03:31
@ggetz
Copy link
Contributor

ggetz commented Mar 15, 2022

Thanks @nmschulte !

@ggetz ggetz merged commit 22c1a97 into CesiumGS:main Mar 15, 2022
@nmschulte nmschulte deleted the fix-firefox-dimensionless-svg-image branch March 16, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

Dimensionless SVG does not render in Firefox
3 participants