refactor(scrollbar): align ScrollbarComponent with CameraComponent architecture#8693
Merged
willeastcott merged 2 commits intomainfrom May 6, 2026
Merged
Conversation
…chitecture ScrollbarComponent now owns its `orientation`, `value`, `handleSize`, and `handleEntity` state in private fields, mirroring the CameraComponent / LightComponent pattern. The bespoke `_setValue` / `data[name]` indirection and the internal `set_*` event chain are removed, ScrollbarComponentData is reduced to `enabled`, and the ScrollbarComponentSystem is now pure boilerplate driven by `_schema = ['enabled']`, an explicit `_properties` list, and an explicit `cloneComponent`. Adds a unit-test suite to guard the behaviour through the migration. Co-authored-by: Cursor <cursoragent@cursor.com>
willeastcott
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors ScrollbarComponent to follow the newer component architecture used by CameraComponent / LightComponent, reducing reliance on *ComponentData property bags while preserving the public API and adding targeted unit tests.
Changes:
- Move
orientation,value,handleSize, andhandleEntitystorage intoScrollbarComponentinstance fields; reduceScrollbarComponentDatatoenabled. - Update
ScrollbarComponentSystemto use_schema = ['enabled'], explicit initialization property ordering, and an explicitcloneComponent. - Add a comprehensive
ScrollbarComponentunit test suite covering defaults, clamping/epsilon behavior, entity reference handling, cloning, and duplicated-entity remapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/framework/components/scrollbar/component.test.mjs | Adds new unit coverage for the refactored ScrollbarComponent behaviors. |
| src/framework/components/scrollbar/system.js | Aligns system initialization/cloning/accessors with schema-only enabled plus explicit properties list. |
| src/framework/components/scrollbar/data.js | Reduces component data to enabled only. |
| src/framework/components/scrollbar/component.js | Moves scrollbar state off the data bag into component-owned fields and simplifies setter/event plumbing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
108
to
+117
| set orientation(arg) { | ||
| this._setValue('orientation', arg); | ||
| if (this._orientation === arg) { | ||
| return; | ||
| } | ||
|
|
||
| this._orientation = arg; | ||
|
|
||
| if (this._handleEntity?.element) { | ||
| this._handleEntity.element[this._getOppositeDimension()] = 0; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Confirmed - this is a real (pre-existing) bug. ElementDragHelper captures its axis at construction, so flipping orientation at runtime leaves drags constrained to the old axis and can stop value updates from happening. Fixed in 6016d61bf as part of #8694: the orientation setter now rebuilds the drag helper and calls _updateHandlePositionAndSize() when a handle element is already attached. Regression test included.
3 tasks
willeastcott
added a commit
that referenced
this pull request
May 6, 2026
ElementDragHelper captures its constraint axis at construction time, so flipping the scrollbar orientation while a handle element exists left the helper still constraining drags to the old axis. _onHandleDrag would then read the wrong component of the position vector and value updates could stop working until the helper was destroyed and rebuilt through some other path (e.g. handleEntity reassignment). Extract the helper-rebuild logic from _onHandleElementGain into a shared _rebuildDragHelper method, and call it (plus _updateHandlePositionAndSize) from the orientation setter when there is an active handle element. Adds a regression test that flips the orientation post-attach and verifies the drag helper is rebuilt for the new axis. Reported by Copilot in #8693. Co-authored-by: Cursor <cursoragent@cursor.com>
willeastcott
added a commit
that referenced
this pull request
May 6, 2026
* refactor(scrollbar): tighten ScrollbarComponent internals Small follow-up cleanup to #8693: - Declare _handleDragHelper as a class field (was implicit-via-assignment). - Tighten the handleEntity setter: resolve the candidate entity first, then a single early-return covers both "same Entity by ref" and "same GUID by string" cases. Switch || null to ?? null. - Drop the _onSetHandleAlignment passthrough; bind _updateHandlePositionAndSize directly to set:anchor / set:margin / set:pivot events on the handle element. - Use an early return in _updateHandlePositionAndSize. - _destroyDragHelper now uses optional chaining and nulls the field after destroy, so a post-element-loss enabled toggle no longer writes to a torn-down helper. - Inline the _setHandleDraggingEnabled helper into onEnable / onDisable. No public API changes. The 16 existing scrollbar tests still pass. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scrollbar): respect component enabled state when building a fresh drag helper ElementDragHelper defaults to enabled = true in its constructor, so a helper created in _onHandleElementGain (e.g. when an element is added to the handle entity after the scrollbar is already disabled) would start out draggable even though onDisable had already run. Mirror the component's current enabled state on the new helper to keep the lifecycle consistent. Adds a regression test that exercises the late-arriving-element path on a disabled scrollbar. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scrollbar): rebuild drag helper when orientation changes at runtime ElementDragHelper captures its constraint axis at construction time, so flipping the scrollbar orientation while a handle element exists left the helper still constraining drags to the old axis. _onHandleDrag would then read the wrong component of the position vector and value updates could stop working until the helper was destroyed and rebuilt through some other path (e.g. handleEntity reassignment). Extract the helper-rebuild logic from _onHandleElementGain into a shared _rebuildDragHelper method, and call it (plus _updateHandlePositionAndSize) from the orientation setter when there is an active handle element. Adds a regression test that flips the orientation post-attach and verifies the drag helper is rebuilt for the new axis. Reported by Copilot in #8693. Co-authored-by: Cursor <cursoragent@cursor.com> * docs(scrollbar): correct handleEntity JSDoc to reference ElementComponent The handleEntity setter's JSDoc claimed the handle "must have a ScrollbarComponent", but the code relies on the handle's element (ElementDragHelper is constructed from handleEntity.element). Update the doc to require an ElementComponent, with useInput: true for the handle to be draggable. Reported by Copilot in #8694. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactor
ScrollbarComponentto mirrorCameraComponent/LightComponent's architecture, continuing the move away from*ComponentDataproperty bags.ScrollbarComponentnow owns_orientation,_value,_handleSize, and_handleEntitydirectly. Properties no longer round-trip throughdata[name]; the_setValuehelper, the internalset_*event chain, and the_onSet*/_toggleLifecycleListenersplumbing are all gone. The clamp-to-[0, 1]and 1e-5 epsilon semantics forvalueandhandleSizeare inlined into the setters.ScrollbarComponentDatais reduced to a singleenabledfield.ScrollbarComponentSystemmatchesCameraComponentSystem:_schema = ['enabled'], an explicit property list drivesinitializeComponentData, an explicitcloneComponentis added (the default base-class clone would otherwise lose every non-enabledfield), andComponent._buildAccessors(ScrollbarComponent.prototype, _schema)handles theenabledaccessor.Public API
No additions, removals, or behaviour changes. The full property surface of
ScrollbarComponentis preserved (verified against the regenerated.d.ts):enabled,orientation,value,handleSize,handleEntity, and the staticEVENT_SETVALUE/set:valueevent still consumed byScrollViewComponent.Tests
Adds
test/framework/components/scrollbar/component.test.mjs, modelled on theLightComponenttest suite, covering#addComponent(defaults + round-trip),#value(clamp +set:valuefiring + epsilon no-op),#handleSize(clamp + epsilon no-op),#orientation(opposite-dimension clear on change),#handleEntity(Entity/ GUID-string /null/ unsubscribe-on-reassign),#cloneComponent(scalar round-trip + handle remap), andresolveDuplicatedEntityReferenceProperties. The suite was confirmed green againstmainbefore the refactor and remains green after it.Test plan
npm run lintpassesnpm run build:typesregenerates.d.tscleanly;npm run test:typespassesnpm test— 1688 passing, 0 failing