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

fix: apple bounding box (CGPathGetPathBoundingBox) #2177

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

omeid
Copy link
Contributor

@omeid omeid commented Nov 24, 2023

Summary

As per the spec, bbox should not include any transformations, including scaling. It should also not include any control points. This fixes getBBox to give correct results matching Google Chrome, Firefox as per the spec.

  • What issues does the pull request solve? Please tag them so that they will get automatically closed once the PR is merged

  • What is the feature? (if applicable)
    Bug fix.

  • How did you implement the solution?
    Like this, see.

  • What areas of the library does it impact?
    getBBox API.

Test Plan

I build some stuff that works with SVG, the results on web were inconsistent, and digging down I realise it was a react-native-svg.

Some fun fact, there was a surprisingly similar bug related to very confusing named (CGPathGetBoundingBox vs CGPathGetPathBoundingBox) APIs in Firefox some years ago as well.
https://bugzilla.mozilla.org/show_bug.cgi?id=1369904

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS
Android 🔘#
Web 🔘 *
  • Depends on the host platform. But works fine in major browsers.

will create a follow up PR. Currently getBBox on Android doesn't account for scaling properly.

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

@omeid
Copy link
Contributor Author

omeid commented Dec 5, 2023

cc @msand

Since you have laid out the initial work for this.

@omeid
Copy link
Contributor Author

omeid commented Sep 29, 2024

Pinging this as this is the only reason I need to maintain a fork and build expo apps.

@bohdanprog
Copy link
Member

@omeid can you rebase your branch to main?

@omeid
Copy link
Contributor Author

omeid commented Oct 8, 2024

@bohdanprog Done!

@omeid
Copy link
Contributor Author

omeid commented Oct 14, 2024

Friendly ping this so that it doesn't go out of sync again. @jakex7

@jakex7
Copy link
Member

jakex7 commented Oct 15, 2024

Hey @omeid, thanks for the PR! The code looks good to me, but I'm not entirely sure what issue it's resolving. Could you provide some examples of code that doesn't work currently and how it's improved after the fix? 😄

@omeid
Copy link
Contributor Author

omeid commented Oct 16, 2024

@jakex7 Currently calling getBBox() on any shape reference, it returns incorrect in iOS and Android.

On Android, it is wrongly scaled.
On iOS, it uses CGPathGetBoundingBox instead of CGPathGetPathBoundingBox. The former includes the control points, while bounding-box is about the visual parts of a shape.

If you want, I can create a snack later.

@jakex7
Copy link
Member

jakex7 commented Nov 5, 2024

@omeid sorry for the late response. I was able to test this PR, and CGPathGetPathBoundingBox appears to work well. However, I'm still uncertain about the issue with the Android scaling. Could you provide a reproduction for this problem?

It seems the scaling issue was introduced in this commit: f3e0b190a917c8f1ddc6a19ccd47563ad6c1ca59. Should we consider removing the scaling from other related areas as well?

Additionally, could you please split this into two separate PRs since they address different aspects?

@omeid
Copy link
Contributor Author

omeid commented Nov 7, 2024

Thanks for the review @jakex7, I will have to test the other methods as I didn't have a need for them, I will create a separate PR for that along with any other changes that might be required for the scaling on Android.

@omeid
Copy link
Contributor Author

omeid commented Nov 7, 2024

Once this is merged, I will create another PR for the Android side.

Copy link
Member

@jakex7 jakex7 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you!

@jakex7 jakex7 changed the title fix bbox as per spec fix: apple bounding box (CGPathGetPathBoundingBox) Nov 7, 2024
@jakex7 jakex7 merged commit 38186e8 into software-mansion:main Nov 7, 2024
5 of 6 checks passed
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.

3 participants