From 98c7cbe8e636b74adee7b434e82724cb530cd686 Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Wed, 6 May 2026 21:03:20 +0100 Subject: [PATCH 1/4] 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 --- .../components/scrollbar/component.js | 69 +++++++++---------- 1 file changed, 32 insertions(+), 37 deletions(-) diff --git a/src/framework/components/scrollbar/component.js b/src/framework/components/scrollbar/component.js index 406008f24f6..187eb4ca132 100644 --- a/src/framework/components/scrollbar/component.js +++ b/src/framework/components/scrollbar/component.js @@ -95,6 +95,12 @@ class ScrollbarComponent extends Component { */ _evtHandleEntityChanges = []; + /** + * @type {ElementDragHelper|null} + * @private + */ + _handleDragHelper = null; + /** * Sets whether the scrollbar moves horizontally or vertically. Can be: * @@ -178,12 +184,16 @@ class ScrollbarComponent extends Component { * @type {Entity|string|null} */ set handleEntity(arg) { - if (this._handleEntity === arg) { - return; + let newEntity; + if (arg instanceof GraphNode) { + newEntity = arg; + } else if (typeof arg === 'string') { + newEntity = this.system.app.getEntityFromIndex(arg) ?? null; + } else { + newEntity = null; } - const isString = typeof arg === 'string'; - if (this._handleEntity && isString && this._handleEntity.getGuid() === arg) { + if (this._handleEntity === newEntity) { return; } @@ -191,15 +201,9 @@ class ScrollbarComponent extends Component { this._handleEntityUnsubscribe(); } - if (arg instanceof GraphNode) { - this._handleEntity = arg; - } else if (isString) { - this._handleEntity = this.system.app.getEntityFromIndex(arg) || null; - } else { - this._handleEntity = null; - } + this._handleEntity = newEntity; - if (this._handleEntity) { + if (newEntity) { this._handleEntitySubscribe(); } } @@ -235,9 +239,9 @@ class ScrollbarComponent extends Component { const handles = this._evtHandleEntityChanges; handles.push(element.once('beforeremove', this._onHandleElementLose, this)); - handles.push(element.on('set:anchor', this._onSetHandleAlignment, this)); - handles.push(element.on('set:margin', this._onSetHandleAlignment, this)); - handles.push(element.on('set:pivot', this._onSetHandleAlignment, this)); + handles.push(element.on('set:anchor', this._updateHandlePositionAndSize, this)); + handles.push(element.on('set:margin', this._updateHandlePositionAndSize, this)); + handles.push(element.on('set:pivot', this._updateHandlePositionAndSize, this)); } _handleEntityElementUnsubscribe() { @@ -267,22 +271,16 @@ class ScrollbarComponent extends Component { } } - _onSetHandleAlignment() { - this._updateHandlePositionAndSize(); - } - _updateHandlePositionAndSize() { const handleEntity = this._handleEntity; - const handleElement = handleEntity?.element; + if (!handleEntity) return; - if (handleEntity) { - const position = handleEntity.getLocalPosition(); - position[this._getAxis()] = this._getHandlePosition(); - handleEntity.setLocalPosition(position); - } + const position = handleEntity.getLocalPosition(); + position[this._getAxis()] = this._getHandlePosition(); + handleEntity.setLocalPosition(position); - if (handleElement) { - handleElement[this._getDimension()] = this._getHandleLength(); + if (handleEntity.element) { + handleEntity.element[this._getDimension()] = this._getHandleLength(); } } @@ -331,23 +329,20 @@ class ScrollbarComponent extends Component { } _destroyDragHelper() { - if (this._handleDragHelper) { - this._handleDragHelper.destroy(); - } + this._handleDragHelper?.destroy(); + this._handleDragHelper = null; } - _setHandleDraggingEnabled(enabled) { + onEnable() { if (this._handleDragHelper) { - this._handleDragHelper.enabled = enabled; + this._handleDragHelper.enabled = true; } } - onEnable() { - this._setHandleDraggingEnabled(true); - } - onDisable() { - this._setHandleDraggingEnabled(false); + if (this._handleDragHelper) { + this._handleDragHelper.enabled = false; + } } onRemove() { From b4e78e545e38143157d4c4aaa937d88638406ada Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Wed, 6 May 2026 21:13:32 +0100 Subject: [PATCH 2/4] 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 --- src/framework/components/scrollbar/component.js | 3 +++ .../components/scrollbar/component.test.mjs | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/framework/components/scrollbar/component.js b/src/framework/components/scrollbar/component.js index 187eb4ca132..81fad236662 100644 --- a/src/framework/components/scrollbar/component.js +++ b/src/framework/components/scrollbar/component.js @@ -255,6 +255,9 @@ class ScrollbarComponent extends Component { this._handleEntityElementSubscribe(); this._destroyDragHelper(); this._handleDragHelper = new ElementDragHelper(this._handleEntity.element, this._getAxis()); + // ElementDragHelper defaults to enabled; mirror the component's current state so a helper + // built while the scrollbar is disabled does not start out draggable + this._handleDragHelper.enabled = this.enabled && this.entity.enabled; this._handleDragHelper.on('drag:move', this._onHandleDrag, this); this._updateHandlePositionAndSize(); diff --git a/test/framework/components/scrollbar/component.test.mjs b/test/framework/components/scrollbar/component.test.mjs index 52ca1378372..11f98cb8a03 100644 --- a/test/framework/components/scrollbar/component.test.mjs +++ b/test/framework/components/scrollbar/component.test.mjs @@ -210,6 +210,23 @@ describe('ScrollbarComponent', function () { expect(e.scrollbar.handleEntity).to.equal(null); }); + it('does not leave a newly-built drag helper enabled when the scrollbar is disabled', function () { + const handle = new Entity(); + // intentionally no element yet — adding it later triggers _onHandleElementGain and + // builds a fresh ElementDragHelper, which defaults to enabled = true + + const e = new Entity(); + e.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + e.addComponent('scrollbar', { enabled: false, handleEntity: handle }); + app.root.addChild(e); + + handle.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + + // helper must mirror the component's disabled state, not its own default + expect(e.scrollbar._handleDragHelper).to.exist; + expect(e.scrollbar._handleDragHelper.enabled).to.equal(false); + }); + it('unsubscribes from the previous handle entity when reassigned', function () { const handle1 = new Entity(); handle1.addComponent('element', { type: ELEMENTTYPE_IMAGE }); From 6016d61bf0abb87806b55643a315709d00396b2e Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Wed, 6 May 2026 21:32:03 +0100 Subject: [PATCH 3/4] 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 --- .../components/scrollbar/component.js | 11 ++++++++-- .../components/scrollbar/component.test.mjs | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/framework/components/scrollbar/component.js b/src/framework/components/scrollbar/component.js index 81fad236662..be79305998c 100644 --- a/src/framework/components/scrollbar/component.js +++ b/src/framework/components/scrollbar/component.js @@ -118,8 +118,12 @@ class ScrollbarComponent extends Component { this._orientation = arg; + // ElementDragHelper captures its axis at construction, so an existing helper + // must be rebuilt to keep dragging in sync with the new orientation if (this._handleEntity?.element) { this._handleEntity.element[this._getOppositeDimension()] = 0; + this._rebuildDragHelper(); + this._updateHandlePositionAndSize(); } } @@ -253,14 +257,17 @@ class ScrollbarComponent extends Component { _onHandleElementGain() { this._handleEntityElementSubscribe(); + this._rebuildDragHelper(); + this._updateHandlePositionAndSize(); + } + + _rebuildDragHelper() { this._destroyDragHelper(); this._handleDragHelper = new ElementDragHelper(this._handleEntity.element, this._getAxis()); // ElementDragHelper defaults to enabled; mirror the component's current state so a helper // built while the scrollbar is disabled does not start out draggable this._handleDragHelper.enabled = this.enabled && this.entity.enabled; this._handleDragHelper.on('drag:move', this._onHandleDrag, this); - - this._updateHandlePositionAndSize(); } _onHandleElementLose() { diff --git a/test/framework/components/scrollbar/component.test.mjs b/test/framework/components/scrollbar/component.test.mjs index 11f98cb8a03..a50fa5d8b06 100644 --- a/test/framework/components/scrollbar/component.test.mjs +++ b/test/framework/components/scrollbar/component.test.mjs @@ -169,6 +169,26 @@ describe('ScrollbarComponent', function () { expect(handle.element.height).to.equal(heightBefore); }); + it('rebuilds the drag helper for the new axis when orientation changes at runtime', function () { + const handle = new Entity(); + handle.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + + const e = new Entity(); + e.addChild(handle); + e.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + e.addComponent('scrollbar', { handleEntity: handle, orientation: ORIENTATION_HORIZONTAL }); + app.root.addChild(e); + + // ElementDragHelper captures its axis at construction, so the helper must be + // rebuilt for the new axis when orientation flips - otherwise drags stay on the + // old axis and value updates can stop working + expect(e.scrollbar._handleDragHelper._axis).to.equal('x'); + + e.scrollbar.orientation = ORIENTATION_VERTICAL; + + expect(e.scrollbar._handleDragHelper._axis).to.equal('y'); + }); + }); describe('#handleEntity', function () { From e581c1a9e7b08d01cc76d0d470ef4296d520eef2 Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Wed, 6 May 2026 21:40:25 +0100 Subject: [PATCH 4/4] 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 --- src/framework/components/scrollbar/component.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/framework/components/scrollbar/component.js b/src/framework/components/scrollbar/component.js index be79305998c..863d051268f 100644 --- a/src/framework/components/scrollbar/component.js +++ b/src/framework/components/scrollbar/component.js @@ -182,8 +182,8 @@ class ScrollbarComponent extends Component { } /** - * Sets the entity to be used as the scrollbar handle. This entity must have a - * {@link ScrollbarComponent}. + * Sets the entity to be used as the scrollbar handle. This entity must have an + * {@link ElementComponent} (with `useInput: true` for the handle to be draggable). * * @type {Entity|string|null} */