[ResizeObserver 3/3] ResizeObserver Web API implementation - #57723
[ResizeObserver 3/3] ResizeObserver Web API implementation#57723paradowstack wants to merge 42 commits into
Conversation
a7e8c72 to
d9da0c1
Compare
|
@rubennorte has imported this pull request. If you are a Meta employee, you can view this in D114045415. |
|
Are the CI failures legit? |
795d2b1 to
027b820
Compare
There were missing targets in Package.swift file - the last commit fixed that. |
rubennorte
left a comment
There was a problem hiding this comment.
This looks great, thanks a lot!
Could you please address the comments in the PR?
Also, I think we should add more tests to see what would happen if a callback scheduled an update in a component. React DOM would probably batch those and run in a separate task, but they also provide flushSync to process them immediately. We don't provide that now in RN but I think we should at some point for use cases like this.
| ... | ||
| }; | ||
|
|
||
| declare type ResizeObserverEntry = { |
There was a problem hiding this comment.
Both this and ResizeObserverSize should be declared as as classes.
| // See | ||
| // https://w3c.github.io/csswg-drafts/resize-observer/#broadcast-resize-notifications-h | ||
| if (resizeObserverDelegate_ != nullptr) { | ||
| resizeObserverDelegate_->runResizeObservations(); |
There was a problem hiding this comment.
I think this is incomplete. This should be run in a loop to process updates coming callbacks themselves. Check resizeObserverDepth in https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model
There was a problem hiding this comment.
That is true, I misunderstood a bit flushSync issue.
I have changed it to use resize loop as in the spec.
| _blockSize: number; | ||
|
|
||
| constructor(inlineSize: number, blockSize: number) { | ||
| this._inlineSize = inlineSize; |
There was a problem hiding this comment.
Can we make this constructor and the one in ResizeObserverEntry private using the same pattern we're using in other Web classes (see <Class>_public)
There was a problem hiding this comment.
They already follow that pattern - take a look at the end of these files.
| zeroContentRect); | ||
| } | ||
|
|
||
| auto layoutMetrics = LayoutableShadowNode::computeRelativeLayoutMetrics( |
There was a problem hiding this comment.
This call is expensive and not necessary for ResizeObserver. In ResizeObserver, the position of the element in the viewport is not considered, only its size. For that, you can just look at the own layout of the element, and don't need the bounding client rect computed from that function.
There was a problem hiding this comment.
Replaced with cheaper version.
| // the padding box (i.e. the paddings only, excluding borders). | ||
| // https://w3c.github.io/csswg-drafts/resize-observer/#dom-resizeobserverentry-contentrect | ||
| auto contentRect = Rect{ | ||
| .origin = |
There was a problem hiding this comment.
This is against the spec. The origin is 0,0 for the contentRect provided.
There was a problem hiding this comment.
Only if target is an SVG element, from spec:
If target is not an SVG element or target is an SVG element with an associated CSS layout box do these steps:
Set this.contentRect.top to target.padding top.
Set this.contentRect.left to target.padding left.If target is an SVG element without an associated CSS layout box do these steps:
Set this.contentRect.top and this.contentRect.left to 0.
See 3.4.4. Create and populate a ResizeObserverEntry point 7.
|
Also, re: the changelog, this should just be [internal] until we promote this to the experimental/canary channels |
NativeResizeObserver.h ships in the prebuilt header layout but includes ResizeObserverManager.h (React-Fabric ships only observers/events) and "FBReactNativeSpecJSI.h" (resolves via pod header maps only). Same two offenders already baselined for the intersection and mutation observer native modules.
bc50446 to
6192ecd
Compare
Thanks for the review! I have addressed all the comments, now implementation includes proper resize loop (gather / broadcast / depth), more tests (Fantom + cc @javache |
6192ecd to
afea976
Compare
Summary:
Implements the
ResizeObserverWeb API for the New Architecture, behind theenableResizeObserverByDefaultflag (off by default).The motivation is Web compatibility, and it's more capable than
onLayout: callers pick which box to observe (content-box,border-box,device-pixel-content-box) and the sizes for those boxes are delivered in the notification.The design is as follows: JS
ResizeObserver/ResizeObserverEntry/ResizeObserverSizeon top of a manager singleton and theNativeResizeObserverTurboModule, using the same notify +takeRecordspull model. In C++,ResizeObserverManagercollects observed targets whose layout changed at commit time (viashadowTreeDidCommit) and then computes and delivers observations in the event loop's "update the rendering" step (RuntimeSchedulerResizeObserverDelegate::runResizeObservations), as the spec requires. Requires bridgeless + the modern event loop; the legacy scheduler gets a no-op delegate.Known deviations from the spec (each pinned by a test):
ResizeObserver loop completed with undelivered notificationserror is never emitted; a callback that triggers a resize is delivered on the next tick.observe()algorithm.observe()order rather than construction order.Changelog:
[INTERNAL] [ADDED] - Add the
ResizeObserverAPI, behind theenableResizeObserverByDefaultfeature flagTest Plan:
ResizeObserver-itest.jscovers many test scenarios.ResizeObserverexamples (box sizes, text, visibility); verified initial, resize, and animation-driven delivery there.RuntimeSchedulerTest.cppnew test scenarios for update rendering loop.