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

Prevent intersectionRatio from being higher than 1 in IntersectionObserverEntry #41448

Closed
wants to merge 6 commits into from

Conversation

rubennorte
Copy link
Contributor

Summary:
This was possible before due to precision problems with double (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Differential Revision: D51230183

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Nov 13, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 13, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

@analysis-bot
Copy link

analysis-bot commented Nov 13, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,650,025 +696
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,033,616 +216
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 686f83e
Branch: main

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 14, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 14, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 14, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 14, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte added a commit to rubennorte/react-native that referenced this pull request Nov 14, 2023
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

rubennorte and others added 6 commits November 14, 2023 05:26
… Fabric (facebook#27687) (facebook#41452)

Summary:

## Summary

This fixes an error in `getPublicInstanceFromInstanceHandle` where we
throw an error when trying to access the public instance from the fiber
of an unmounted component. This shouldn't throw but return `null`
instead.

## How did you test this change?

Updated unit tests.
Before:
<img width="969" alt="Screenshot 2023-11-10 at 15 26 14"
src="https://github.com/facebook/react/assets/117921/ea161616-2775-4fab-8d74-da4bef48d09a">

After:
<img width="1148" alt="Screenshot 2023-11-10 at 15 28 37"
src="https://github.com/facebook/react/assets/117921/db18b918-b6b6-4925-9cfc-3b4b2f3ab92d">

DiffTrain build for commit facebook/react@6b3834a.

Test Plan: Sandcastle tests

Reviewed By: tyao1

Differential Revision: D51203276

Pulled By: tyao1
…k#41451)

Summary:

After [this change in React](facebook/react#27687), `ReactFabric.getPublicInstanceFromInternalInstanceHandle` can return `null` if the instance handle is a fiber that was unmounted (before that PR, it would throw an error).

This modifies the DOM traversal API to gracefully handle that case.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51210455
…ver (facebook#41450)

Summary:

`IntersectionObserver` shouldn't report entries for targets that are no longer being observed by the observer. This wasn't the case before because it was possible to create an intersection observer entry, then unobserve the target and then dispatch the pending entries (including the unobserved target). This fixes that issue to align with Web browsers.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51256827
…ntries (facebook#41449)

Summary:

`IntersectionObserver` was incorrectly throwing errors when reporting entries for detached targets. The problem was that we were deriving the target instance from the instance handle that we keep in native, but React removes the connection between them when the instance handle is unmounted.

This fixes the problem by keeping an internal mapping between instance handle and target internally in the intersection observer manager.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51210456
…server (facebook#41454)

Summary:

Yet another problem caused by React being too aggressive cleaning up detached fibers. This fixes a problem in `IntersectionObserver` when trying to `unobserve` detached targets. In that case we didn't have access to its shadow node anymore, so we didn't have a way to unobserve in native. This keeps an additional mapping in JS to do the conversion even after detached.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51257960
…erverEntry (facebook#41448)

Summary:

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51230183

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 14, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 5948ab7.

Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…erverEntry (facebook#41448)

Summary:
Pull Request resolved: facebook#41448

This was possible before due to precision problems with `double` (we were seeing values like 1.000000002). This is an easy way to prevent that problem.

Changelog: [internal]

Reviewed By: rshest

Differential Revision: D51230183

fbshipit-source-id: 757ef181fe369d525831faf8a6d907467efc544c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants