From a2e0789e92ad00c81c0e8d5befce0a06512c7b9f Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Tue, 26 May 2026 00:14:02 +0100 Subject: [PATCH 1/3] refactor(scrollview): align ScrollViewComponent with CameraComponent architecture Apply the same refactor pattern recently adopted by ScrollbarComponent (#8693): the component now owns its state directly via private fields, setters do their work inline, the data-bag round-trip and set_ event chain are gone, and the system declares `_schema = ['enabled']` with an explicit `_properties` list and `cloneComponent`. `mouseWheelSensitivity` previously relied on the schema's `type: 'vec2'` auto-conversion (clones Vec2 inputs, builds a Vec2 from `[x, y]` arrays); that work is moved into the setter itself so JSON-loaded scenes shipping `mouseWheelSensitivity: [1, 1]` and callers passing shared Vec2 instances continue to work. Add `test/framework/components/scroll-view/component.test.mjs` covering defaults, the round-trip via `addComponent`, the Vec2/array normalization on `mouseWheelSensitivity`, side effects on `horizontal`/`vertical`, `set:scroll`, all four entity setters (Entity / GUID / null / unsubscribe-on-reassign), `cloneComponent`, and `resolveDuplicatedEntityReferenceProperties`. No public API change: properties, behavior, and the `set:scroll` event are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/scroll-view/component.js | 189 +++--- src/framework/components/scroll-view/data.js | 47 -- .../components/scroll-view/system.js | 76 ++- .../components/scroll-view/component.test.mjs | 563 ++++++++++++++++++ 4 files changed, 710 insertions(+), 165 deletions(-) create mode 100644 test/framework/components/scroll-view/component.test.mjs diff --git a/src/framework/components/scroll-view/component.js b/src/framework/components/scroll-view/component.js index 981fa1d71ac..182284820e8 100644 --- a/src/framework/components/scroll-view/component.js +++ b/src/framework/components/scroll-view/component.js @@ -70,6 +70,51 @@ class ScrollViewComponent extends Component { */ static EVENT_SETSCROLL = 'set:scroll'; + /** + * @type {boolean|undefined} + * @private + */ + _horizontal; + + /** + * @type {boolean|undefined} + * @private + */ + _vertical; + + /** + * @type {number|undefined} + * @private + */ + _scrollMode; + + /** + * @type {number|undefined} + * @private + */ + _bounceAmount; + + /** + * @type {number|undefined} + * @private + */ + _friction; + + /** @private */ + _dragThreshold = 10; + + /** @private */ + _useMouseWheel = true; + + /** @private */ + _mouseWheelSensitivity = new Vec2(1, 1); + + /** @private */ + _horizontalScrollbarVisibility = 0; + + /** @private */ + _verticalScrollbarVisibility = 0; + /** * @type {Entity|null} * @private @@ -94,12 +139,24 @@ class ScrollViewComponent extends Component { */ _verticalScrollbarEntity = null; + /** + * @type {EventHandle|null} + * @private + */ + _evtElementAdd = null; + /** * @type {EventHandle|null} * @private */ _evtElementRemove = null; + /** + * @type {EventHandle|null} + * @private + */ + _evtViewportEntityElementAdd = null; + /** * @type {EventHandle|null} * @private @@ -190,35 +247,22 @@ class ScrollViewComponent extends Component { this._disabledContentInput = false; this._disabledContentInputEntities = []; - this._toggleLifecycleListeners('on'); + this._evtElementAdd = this.entity.on('element:add', this._onElementComponentAdd, this); this._toggleElementListeners('on'); } - /** - * Sets the enabled state of the component. - * - * @type {boolean} - */ - set enabled(arg) { - this._setValue('enabled', arg); - } - - /** - * Gets the enabled state of the component. - * - * @type {boolean} - */ - get enabled() { - return this.data.enabled; - } - /** * Sets whether horizontal scrolling is enabled. * * @type {boolean} */ set horizontal(arg) { - this._setValue('horizontal', arg); + if (this._horizontal === arg) { + return; + } + + this._horizontal = arg; + this._syncScrollbarEnabledState(ORIENTATION_HORIZONTAL); } /** @@ -227,7 +271,7 @@ class ScrollViewComponent extends Component { * @type {boolean} */ get horizontal() { - return this.data.horizontal; + return this._horizontal; } /** @@ -236,7 +280,12 @@ class ScrollViewComponent extends Component { * @type {boolean} */ set vertical(arg) { - this._setValue('vertical', arg); + if (this._vertical === arg) { + return; + } + + this._vertical = arg; + this._syncScrollbarEnabledState(ORIENTATION_VERTICAL); } /** @@ -245,7 +294,7 @@ class ScrollViewComponent extends Component { * @type {boolean} */ get vertical() { - return this.data.vertical; + return this._vertical; } /** @@ -259,7 +308,7 @@ class ScrollViewComponent extends Component { * @type {number} */ set scrollMode(arg) { - this._setValue('scrollMode', arg); + this._scrollMode = arg; } /** @@ -268,7 +317,7 @@ class ScrollViewComponent extends Component { * @type {number} */ get scrollMode() { - return this.data.scrollMode; + return this._scrollMode; } /** @@ -277,7 +326,7 @@ class ScrollViewComponent extends Component { * @type {number} */ set bounceAmount(arg) { - this._setValue('bounceAmount', arg); + this._bounceAmount = arg; } /** @@ -286,7 +335,7 @@ class ScrollViewComponent extends Component { * @type {number} */ get bounceAmount() { - return this.data.bounceAmount; + return this._bounceAmount; } /** @@ -298,7 +347,7 @@ class ScrollViewComponent extends Component { * @type {number} */ set friction(arg) { - this._setValue('friction', arg); + this._friction = arg; } /** @@ -307,15 +356,15 @@ class ScrollViewComponent extends Component { * @type {number} */ get friction() { - return this.data.friction; + return this._friction; } set dragThreshold(arg) { - this._setValue('dragThreshold', arg); + this._dragThreshold = arg; } get dragThreshold() { - return this.data.dragThreshold; + return this._dragThreshold; } /** @@ -324,7 +373,7 @@ class ScrollViewComponent extends Component { * @type {boolean} */ set useMouseWheel(arg) { - this._setValue('useMouseWheel', arg); + this._useMouseWheel = arg; } /** @@ -333,7 +382,7 @@ class ScrollViewComponent extends Component { * @type {boolean} */ get useMouseWheel() { - return this.data.useMouseWheel; + return this._useMouseWheel; } /** @@ -345,7 +394,15 @@ class ScrollViewComponent extends Component { * @type {Vec2} */ set mouseWheelSensitivity(arg) { - this._setValue('mouseWheelSensitivity', arg); + if (arg instanceof Vec2) { + // Clone so callers can pass shared singletons (e.g. Vec2.ONE) and so subsequent + // mutations to their Vec2 don't leak into the component's state. + this._mouseWheelSensitivity = arg.clone(); + } else if (Array.isArray(arg)) { + this._mouseWheelSensitivity = new Vec2(arg[0], arg[1]); + } else { + this._mouseWheelSensitivity = arg; + } } /** @@ -354,7 +411,7 @@ class ScrollViewComponent extends Component { * @type {Vec2} */ get mouseWheelSensitivity() { - return this.data.mouseWheelSensitivity; + return this._mouseWheelSensitivity; } /** @@ -364,7 +421,7 @@ class ScrollViewComponent extends Component { * @type {number} */ set horizontalScrollbarVisibility(arg) { - this._setValue('horizontalScrollbarVisibility', arg); + this._horizontalScrollbarVisibility = arg; } /** @@ -374,7 +431,7 @@ class ScrollViewComponent extends Component { * @type {number} */ get horizontalScrollbarVisibility() { - return this.data.horizontalScrollbarVisibility; + return this._horizontalScrollbarVisibility; } /** @@ -384,7 +441,7 @@ class ScrollViewComponent extends Component { * @type {number} */ set verticalScrollbarVisibility(arg) { - this._setValue('verticalScrollbarVisibility', arg); + this._verticalScrollbarVisibility = arg; } /** @@ -394,7 +451,7 @@ class ScrollViewComponent extends Component { * @type {number} */ get verticalScrollbarVisibility() { - return this.data.verticalScrollbarVisibility; + return this._verticalScrollbarVisibility; } /** @@ -428,12 +485,6 @@ class ScrollViewComponent extends Component { if (this._viewportEntity) { this._viewportEntitySubscribe(); } - - if (this._viewportEntity) { - this.data.viewportEntity = this._viewportEntity.getGuid(); - } else if (isString && arg) { - this.data.viewportEntity = arg; - } } /** @@ -476,12 +527,6 @@ class ScrollViewComponent extends Component { if (this._contentEntity) { this._contentEntitySubscribe(); } - - if (this._contentEntity) { - this.data.contentEntity = this._contentEntity.getGuid(); - } else if (isString && arg) { - this.data.contentEntity = arg; - } } /** @@ -526,12 +571,6 @@ class ScrollViewComponent extends Component { if (this._horizontalScrollbarEntity) { this._horizontalScrollbarEntitySubscribe(); } - - if (this._horizontalScrollbarEntity) { - this.data.horizontalScrollbarEntity = this._horizontalScrollbarEntity.getGuid(); - } else if (isString && arg) { - this.data.horizontalScrollbarEntity = arg; - } } /** @@ -576,12 +615,6 @@ class ScrollViewComponent extends Component { if (this._verticalScrollbarEntity) { this._verticalScrollbarEntitySubscribe(); } - - if (this._verticalScrollbarEntity) { - this.data.verticalScrollbarEntity = this._verticalScrollbarEntity.getGuid(); - } else if (isString && arg) { - this.data.verticalScrollbarEntity = arg; - } } /** @@ -611,25 +644,6 @@ class ScrollViewComponent extends Component { return this._scroll; } - /** @ignore */ - _setValue(name, value) { - const data = this.data; - const oldValue = data[name]; - data[name] = value; - this.fire('set', name, oldValue, value); - } - - /** - * @param {string} onOrOff - 'on' or 'off'. - * @private - */ - _toggleLifecycleListeners(onOrOff) { - this[onOrOff]('set_horizontal', this._onSetHorizontalScrollingEnabled, this); - this[onOrOff]('set_vertical', this._onSetVerticalScrollingEnabled, this); - - this.entity[onOrOff]('element:add', this._onElementComponentAdd, this); - } - /** * @param {string} onOrOff - 'on' or 'off'. * @private @@ -860,14 +874,6 @@ class ScrollViewComponent extends Component { this._evtVerticalScrollbarValue = null; } - _onSetHorizontalScrollingEnabled() { - this._syncScrollbarEnabledState(ORIENTATION_HORIZONTAL); - } - - _onSetVerticalScrollingEnabled() { - this._syncScrollbarEnabledState(ORIENTATION_VERTICAL); - } - _onSetScroll(x, y, resetVelocity) { if (resetVelocity !== false) { this._velocity.set(0, 0, 0); @@ -1316,7 +1322,8 @@ class ScrollViewComponent extends Component { } onRemove() { - this._toggleLifecycleListeners('off'); + this._evtElementAdd?.off(); + this._evtElementAdd = null; this._toggleElementListeners('off'); this._destroyDragHelper(); } diff --git a/src/framework/components/scroll-view/data.js b/src/framework/components/scroll-view/data.js index 408a7a18043..163a5761d89 100644 --- a/src/framework/components/scroll-view/data.js +++ b/src/framework/components/scroll-view/data.js @@ -1,52 +1,5 @@ -import { Vec2 } from '../../../core/math/vec2.js'; - -/** - * @import { Entity } from '../../../framework/entity.js' - */ - -const DEFAULT_DRAG_THRESHOLD = 10; - class ScrollViewComponentData { enabled = true; - - /** @type {boolean} */ - horizontal; - - /** @type {boolean} */ - vertical; - - /** @type {number} */ - scrollMode; - - /** @type {number} */ - bounceAmount; - - /** @type {number} */ - friction; - - dragThreshold = DEFAULT_DRAG_THRESHOLD; - - useMouseWheel = true; - - mouseWheelSensitivity = new Vec2(1, 1); - - /** @type {number} */ - horizontalScrollbarVisibility = 0; - - /** @type {number} */ - verticalScrollbarVisibility = 0; - - /** @type {Entity|null} */ - viewportEntity = null; - - /** @type {Entity|null} */ - contentEntity = null; - - /** @type {Entity|null} */ - horizontalScrollbarEntity = null; - - /** @type {Entity|null} */ - verticalScrollbarEntity = null; } export { ScrollViewComponentData }; diff --git a/src/framework/components/scroll-view/system.js b/src/framework/components/scroll-view/system.js index df3dd7f4d47..b6bce3d839e 100644 --- a/src/framework/components/scroll-view/system.js +++ b/src/framework/components/scroll-view/system.js @@ -1,4 +1,4 @@ -import { Vec2 } from '../../../core/math/vec2.js'; +import { Component } from '../component.js'; import { ComponentSystem } from '../system.js'; import { ScrollViewComponent } from './component.js'; import { ScrollViewComponentData } from './data.js'; @@ -7,22 +7,29 @@ import { ScrollViewComponentData } from './data.js'; * @import { AppBase } from '../../app-base.js' */ -const _schema = [ - { name: 'enabled', type: 'boolean' }, - { name: 'horizontal', type: 'boolean' }, - { name: 'vertical', type: 'boolean' }, - { name: 'scrollMode', type: 'number' }, - { name: 'bounceAmount', type: 'number' }, - { name: 'friction', type: 'number' }, - { name: 'dragThreshold', type: 'number' }, - { name: 'useMouseWheel', type: 'boolean' }, - { name: 'mouseWheelSensitivity', type: 'vec2' }, - { name: 'horizontalScrollbarVisibility', type: 'number' }, - { name: 'verticalScrollbarVisibility', type: 'number' } +const _schema = ['enabled']; + +// Order matters: scalars/booleans/visibility flags must precede the four entity refs. +// Assigning a scrollbar entity triggers _onHorizontalScrollbarGain / _onVerticalScrollbarGain, +// which call _syncScrollbarEnabledState — that reads `horizontal`/`vertical` and the visibility +// fields. If those are still undefined/0 at that moment, the wrong branch fires. +const _properties = [ + 'horizontal', + 'vertical', + 'scrollMode', + 'bounceAmount', + 'friction', + 'dragThreshold', + 'useMouseWheel', + 'mouseWheelSensitivity', + 'horizontalScrollbarVisibility', + 'verticalScrollbarVisibility', + 'viewportEntity', + 'contentEntity', + 'horizontalScrollbarEntity', + 'verticalScrollbarEntity' ]; -const DEFAULT_DRAG_THRESHOLD = 10; - /** * Manages creation of {@link ScrollViewComponent}s. * @@ -51,22 +58,35 @@ class ScrollViewComponentSystem extends ComponentSystem { } initializeComponentData(component, data, properties) { - if (data.dragThreshold === undefined) { - data.dragThreshold = DEFAULT_DRAG_THRESHOLD; - } - if (data.useMouseWheel === undefined) { - data.useMouseWheel = true; - } - if (data.mouseWheelSensitivity === undefined) { - data.mouseWheelSensitivity = new Vec2(1, 1); + for (let i = 0; i < _properties.length; i++) { + const property = _properties[i]; + if (data.hasOwnProperty(property)) { + component[property] = data[property]; + } } super.initializeComponentData(component, data, _schema); + } - component.viewportEntity = data.viewportEntity; - component.contentEntity = data.contentEntity; - component.horizontalScrollbarEntity = data.horizontalScrollbarEntity; - component.verticalScrollbarEntity = data.verticalScrollbarEntity; + cloneComponent(entity, clone) { + const c = entity.scrollview; + return this.addComponent(clone, { + enabled: c.enabled, + horizontal: c.horizontal, + vertical: c.vertical, + scrollMode: c.scrollMode, + bounceAmount: c.bounceAmount, + friction: c.friction, + dragThreshold: c.dragThreshold, + useMouseWheel: c.useMouseWheel, + mouseWheelSensitivity: c.mouseWheelSensitivity, + horizontalScrollbarVisibility: c.horizontalScrollbarVisibility, + verticalScrollbarVisibility: c.verticalScrollbarVisibility, + viewportEntity: c.viewportEntity, + contentEntity: c.contentEntity, + horizontalScrollbarEntity: c.horizontalScrollbarEntity, + verticalScrollbarEntity: c.verticalScrollbarEntity + }); } onUpdate(dt) { @@ -93,4 +113,6 @@ class ScrollViewComponentSystem extends ComponentSystem { } } +Component._buildAccessors(ScrollViewComponent.prototype, _schema); + export { ScrollViewComponentSystem }; diff --git a/test/framework/components/scroll-view/component.test.mjs b/test/framework/components/scroll-view/component.test.mjs new file mode 100644 index 00000000000..9c6611a2132 --- /dev/null +++ b/test/framework/components/scroll-view/component.test.mjs @@ -0,0 +1,563 @@ +import { expect } from 'chai'; + +import { Vec2 } from '../../../../src/core/math/vec2.js'; +import { ELEMENTTYPE_GROUP, ELEMENTTYPE_IMAGE } from '../../../../src/framework/components/element/constants.js'; +import { + SCROLL_MODE_CLAMP, + SCROLL_MODE_INFINITE, + SCROLLBAR_VISIBILITY_SHOW_ALWAYS +} from '../../../../src/framework/components/scroll-view/constants.js'; +import { Entity } from '../../../../src/framework/entity.js'; +import { ORIENTATION_HORIZONTAL, ORIENTATION_VERTICAL } from '../../../../src/scene/constants.js'; +import { createApp } from '../../../app.mjs'; +import { jsdomSetup, jsdomTeardown } from '../../../jsdom.mjs'; + +function buildScrollViewEntity() { + const viewport = new Entity('viewport'); + viewport.addComponent('element', { type: ELEMENTTYPE_GROUP }); + + const content = new Entity('content'); + content.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + + const hScrollbarHandle = new Entity('hScrollbarHandle'); + hScrollbarHandle.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + const hScrollbar = new Entity('hScrollbar'); + hScrollbar.addChild(hScrollbarHandle); + hScrollbar.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + hScrollbar.addComponent('scrollbar', { + orientation: ORIENTATION_HORIZONTAL, + handleEntity: hScrollbarHandle + }); + + const vScrollbarHandle = new Entity('vScrollbarHandle'); + vScrollbarHandle.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + const vScrollbar = new Entity('vScrollbar'); + vScrollbar.addChild(vScrollbarHandle); + vScrollbar.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + vScrollbar.addComponent('scrollbar', { + orientation: ORIENTATION_VERTICAL, + handleEntity: vScrollbarHandle + }); + + const e = new Entity('scrollview'); + e.addChild(viewport); + e.addChild(content); + e.addChild(hScrollbar); + e.addChild(vScrollbar); + e.addComponent('element', { type: ELEMENTTYPE_GROUP }); + + return { e, viewport, content, hScrollbar, vScrollbar }; +} + +describe('ScrollViewComponent', function () { + let app; + + beforeEach(function () { + jsdomSetup(); + app = createApp(); + }); + + afterEach(function () { + app?.destroy(); + app = null; + jsdomTeardown(); + }); + + describe('#addComponent', function () { + + it('creates a component with sensible defaults', function () { + const e = new Entity(); + e.addComponent('scrollview'); + + expect(e.scrollview).to.exist; + expect(e.scrollview.enabled).to.equal(true); + expect(e.scrollview.horizontal).to.equal(undefined); + expect(e.scrollview.vertical).to.equal(undefined); + expect(e.scrollview.scrollMode).to.equal(undefined); + expect(e.scrollview.bounceAmount).to.equal(undefined); + expect(e.scrollview.friction).to.equal(undefined); + expect(e.scrollview.dragThreshold).to.equal(10); + expect(e.scrollview.useMouseWheel).to.equal(true); + expect(e.scrollview.mouseWheelSensitivity).to.be.an.instanceof(Vec2); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(1); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(1); + expect(e.scrollview.horizontalScrollbarVisibility).to.equal(SCROLLBAR_VISIBILITY_SHOW_ALWAYS); + expect(e.scrollview.verticalScrollbarVisibility).to.equal(SCROLLBAR_VISIBILITY_SHOW_ALWAYS); + expect(e.scrollview.viewportEntity).to.equal(null); + expect(e.scrollview.contentEntity).to.equal(null); + expect(e.scrollview.horizontalScrollbarEntity).to.equal(null); + expect(e.scrollview.verticalScrollbarEntity).to.equal(null); + }); + + it('round-trips every property passed via the data argument', function () { + const { e, viewport, content, hScrollbar, vScrollbar } = buildScrollViewEntity(); + const sensitivity = new Vec2(2, 3); + + e.addComponent('scrollview', { + enabled: false, + horizontal: true, + vertical: false, + scrollMode: SCROLL_MODE_INFINITE, + bounceAmount: 0.25, + friction: 0.1, + dragThreshold: 20, + useMouseWheel: false, + mouseWheelSensitivity: sensitivity, + horizontalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + verticalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + viewportEntity: viewport, + contentEntity: content, + horizontalScrollbarEntity: hScrollbar, + verticalScrollbarEntity: vScrollbar + }); + + expect(e.scrollview.enabled).to.equal(false); + expect(e.scrollview.horizontal).to.equal(true); + expect(e.scrollview.vertical).to.equal(false); + expect(e.scrollview.scrollMode).to.equal(SCROLL_MODE_INFINITE); + expect(e.scrollview.bounceAmount).to.be.closeTo(0.25, 1e-6); + expect(e.scrollview.friction).to.be.closeTo(0.1, 1e-6); + expect(e.scrollview.dragThreshold).to.equal(20); + expect(e.scrollview.useMouseWheel).to.equal(false); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(2); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(3); + expect(e.scrollview.horizontalScrollbarVisibility).to.equal(SCROLLBAR_VISIBILITY_SHOW_ALWAYS); + expect(e.scrollview.verticalScrollbarVisibility).to.equal(SCROLLBAR_VISIBILITY_SHOW_ALWAYS); + expect(e.scrollview.viewportEntity).to.equal(viewport); + expect(e.scrollview.contentEntity).to.equal(content); + expect(e.scrollview.horizontalScrollbarEntity).to.equal(hScrollbar); + expect(e.scrollview.verticalScrollbarEntity).to.equal(vScrollbar); + }); + + }); + + describe('#mouseWheelSensitivity', function () { + + it('accepts an [x, y] array and stores a Vec2', function () { + const e = new Entity(); + e.addComponent('scrollview', { mouseWheelSensitivity: [2, 3] }); + + expect(e.scrollview.mouseWheelSensitivity).to.be.an.instanceof(Vec2); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(2); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(3); + }); + + it('clones a Vec2 input so caller mutations do not leak into component state', function () { + const sensitivity = new Vec2(2, 3); + const e = new Entity(); + e.addComponent('scrollview', { mouseWheelSensitivity: sensitivity }); + + // mutate the caller's Vec2; the component's stored Vec2 must be unaffected + sensitivity.x = 99; + sensitivity.y = 99; + + expect(e.scrollview.mouseWheelSensitivity).to.not.equal(sensitivity); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(2); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(3); + }); + + }); + + describe('#horizontal', function () { + + it('syncs the horizontal scrollbar entity enabled state when toggled', function () { + const { e, hScrollbar } = buildScrollViewEntity(); + e.addComponent('scrollview', { + horizontal: true, + vertical: true, + scrollMode: SCROLL_MODE_CLAMP, + horizontalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + horizontalScrollbarEntity: hScrollbar + }); + app.root.addChild(e); + + expect(hScrollbar.enabled).to.equal(true); + + e.scrollview.horizontal = false; + expect(hScrollbar.enabled).to.equal(false); + + e.scrollview.horizontal = true; + expect(hScrollbar.enabled).to.equal(true); + }); + + it('is a no-op when the value does not change', function () { + const { e, hScrollbar } = buildScrollViewEntity(); + e.addComponent('scrollview', { + horizontal: true, + horizontalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + horizontalScrollbarEntity: hScrollbar + }); + + // manually flip the scrollbar's enabled state and verify a same-value write + // does not stomp it (proves the early-return path runs) + hScrollbar.enabled = false; + e.scrollview.horizontal = true; + expect(hScrollbar.enabled).to.equal(false); + }); + + }); + + describe('#vertical', function () { + + it('syncs the vertical scrollbar entity enabled state when toggled', function () { + const { e, vScrollbar } = buildScrollViewEntity(); + e.addComponent('scrollview', { + horizontal: true, + vertical: true, + scrollMode: SCROLL_MODE_CLAMP, + verticalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + verticalScrollbarEntity: vScrollbar + }); + app.root.addChild(e); + + expect(vScrollbar.enabled).to.equal(true); + + e.scrollview.vertical = false; + expect(vScrollbar.enabled).to.equal(false); + }); + + }); + + describe('#scroll', function () { + + it('fires set:scroll when the value changes', function () { + const { e, viewport, content } = buildScrollViewEntity(); + e.addComponent('scrollview', { + horizontal: true, + vertical: true, + // INFINITE so jsdom's zero-sized viewport/content don't clamp the value to 0 + scrollMode: SCROLL_MODE_INFINITE, + viewportEntity: viewport, + contentEntity: content + }); + app.root.addChild(e); + + const captured = []; + e.scrollview.on('set:scroll', (scroll) => { + captured.push({ x: scroll.x, y: scroll.y }); + }); + + e.scrollview.scroll = new Vec2(0.5, 0.25); + + expect(captured.length).to.equal(1); + expect(captured[0].x).to.be.closeTo(0.5, 1e-5); + expect(captured[0].y).to.be.closeTo(0.25, 1e-5); + }); + + }); + + describe('#viewportEntity', function () { + + it('accepts an Entity reference', function () { + const viewport = new Entity(); + viewport.addComponent('element', { type: ELEMENTTYPE_GROUP }); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.viewportEntity = viewport; + + expect(e.scrollview.viewportEntity).to.equal(viewport); + }); + + it('accepts a GUID string and resolves via app.getEntityFromIndex', function () { + const viewport = new Entity(); + viewport.addComponent('element', { type: ELEMENTTYPE_GROUP }); + const guid = viewport.getGuid(); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.viewportEntity = guid; + + expect(e.scrollview.viewportEntity).to.equal(viewport); + }); + + it('accepts null', function () { + const viewport = new Entity(); + viewport.addComponent('element', { type: ELEMENTTYPE_GROUP }); + + const e = new Entity(); + e.addComponent('scrollview', { viewportEntity: viewport }); + + e.scrollview.viewportEntity = null; + + expect(e.scrollview.viewportEntity).to.equal(null); + }); + + it('unsubscribes from the previous viewport entity when reassigned', function () { + const viewport1 = new Entity(); + const viewport2 = new Entity(); + + const e = new Entity(); + e.addComponent('scrollview', { viewportEntity: viewport1 }); + + expect(viewport1.hasEvent('element:add')).to.equal(true); + expect(viewport2.hasEvent('element:add')).to.equal(false); + + e.scrollview.viewportEntity = viewport2; + + expect(viewport1.hasEvent('element:add')).to.equal(false); + expect(viewport2.hasEvent('element:add')).to.equal(true); + }); + + }); + + describe('#contentEntity', function () { + + it('accepts an Entity reference', function () { + const content = new Entity(); + content.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.contentEntity = content; + + expect(e.scrollview.contentEntity).to.equal(content); + }); + + it('accepts a GUID string and resolves via app.getEntityFromIndex', function () { + const content = new Entity(); + content.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + const guid = content.getGuid(); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.contentEntity = guid; + + expect(e.scrollview.contentEntity).to.equal(content); + }); + + it('accepts null', function () { + const content = new Entity(); + content.addComponent('element', { type: ELEMENTTYPE_IMAGE }); + + const e = new Entity(); + e.addComponent('scrollview', { contentEntity: content }); + + e.scrollview.contentEntity = null; + + expect(e.scrollview.contentEntity).to.equal(null); + }); + + it('unsubscribes from the previous content entity when reassigned', function () { + const content1 = new Entity(); + const content2 = new Entity(); + + const e = new Entity(); + e.addComponent('scrollview', { contentEntity: content1 }); + + expect(content1.hasEvent('element:add')).to.equal(true); + expect(content2.hasEvent('element:add')).to.equal(false); + + e.scrollview.contentEntity = content2; + + expect(content1.hasEvent('element:add')).to.equal(false); + expect(content2.hasEvent('element:add')).to.equal(true); + }); + + }); + + describe('#horizontalScrollbarEntity', function () { + + it('accepts an Entity reference', function () { + const { hScrollbar } = buildScrollViewEntity(); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.horizontalScrollbarEntity = hScrollbar; + + expect(e.scrollview.horizontalScrollbarEntity).to.equal(hScrollbar); + }); + + it('accepts a GUID string and resolves via app.getEntityFromIndex', function () { + const { hScrollbar } = buildScrollViewEntity(); + const guid = hScrollbar.getGuid(); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.horizontalScrollbarEntity = guid; + + expect(e.scrollview.horizontalScrollbarEntity).to.equal(hScrollbar); + }); + + it('accepts null', function () { + const { hScrollbar } = buildScrollViewEntity(); + + const e = new Entity(); + e.addComponent('scrollview', { horizontalScrollbarEntity: hScrollbar }); + + e.scrollview.horizontalScrollbarEntity = null; + + expect(e.scrollview.horizontalScrollbarEntity).to.equal(null); + }); + + it('unsubscribes from the previous scrollbar entity when reassigned', function () { + const scrollbar1 = new Entity(); + const scrollbar2 = new Entity(); + + const e = new Entity(); + e.addComponent('scrollview', { horizontalScrollbarEntity: scrollbar1 }); + + expect(scrollbar1.hasEvent('scrollbar:add')).to.equal(true); + expect(scrollbar2.hasEvent('scrollbar:add')).to.equal(false); + + e.scrollview.horizontalScrollbarEntity = scrollbar2; + + expect(scrollbar1.hasEvent('scrollbar:add')).to.equal(false); + expect(scrollbar2.hasEvent('scrollbar:add')).to.equal(true); + }); + + }); + + describe('#verticalScrollbarEntity', function () { + + it('accepts an Entity reference', function () { + const { vScrollbar } = buildScrollViewEntity(); + + const e = new Entity(); + e.addComponent('scrollview'); + + e.scrollview.verticalScrollbarEntity = vScrollbar; + + expect(e.scrollview.verticalScrollbarEntity).to.equal(vScrollbar); + }); + + it('accepts null', function () { + const { vScrollbar } = buildScrollViewEntity(); + + const e = new Entity(); + e.addComponent('scrollview', { verticalScrollbarEntity: vScrollbar }); + + e.scrollview.verticalScrollbarEntity = null; + + expect(e.scrollview.verticalScrollbarEntity).to.equal(null); + }); + + it('unsubscribes from the previous scrollbar entity when reassigned', function () { + const scrollbar1 = new Entity(); + const scrollbar2 = new Entity(); + + const e = new Entity(); + e.addComponent('scrollview', { verticalScrollbarEntity: scrollbar1 }); + + expect(scrollbar1.hasEvent('scrollbar:add')).to.equal(true); + + e.scrollview.verticalScrollbarEntity = scrollbar2; + + expect(scrollbar1.hasEvent('scrollbar:add')).to.equal(false); + expect(scrollbar2.hasEvent('scrollbar:add')).to.equal(true); + }); + + }); + + describe('#cloneComponent', function () { + + it('clones every scalar property', function () { + const e = new Entity(); + e.addComponent('scrollview', { + enabled: false, + horizontal: true, + vertical: false, + scrollMode: SCROLL_MODE_INFINITE, + bounceAmount: 0.25, + friction: 0.1, + dragThreshold: 20, + useMouseWheel: false, + mouseWheelSensitivity: new Vec2(2, 3), + horizontalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS, + verticalScrollbarVisibility: SCROLLBAR_VISIBILITY_SHOW_ALWAYS + }); + + const clone = e.clone(); + const c = clone.scrollview; + + expect(c).to.exist; + expect(c.enabled).to.equal(false); + expect(c.horizontal).to.equal(true); + expect(c.vertical).to.equal(false); + expect(c.scrollMode).to.equal(SCROLL_MODE_INFINITE); + expect(c.bounceAmount).to.be.closeTo(0.25, 1e-6); + expect(c.friction).to.be.closeTo(0.1, 1e-6); + expect(c.dragThreshold).to.equal(20); + expect(c.useMouseWheel).to.equal(false); + expect(c.mouseWheelSensitivity.x).to.equal(2); + expect(c.mouseWheelSensitivity.y).to.equal(3); + }); + + it('remaps every entity ref to the cloned subtree via the duplicated ids map', function () { + const { e, viewport, content, hScrollbar, vScrollbar } = buildScrollViewEntity(); + e.addComponent('scrollview', { + viewportEntity: viewport, + contentEntity: content, + horizontalScrollbarEntity: hScrollbar, + verticalScrollbarEntity: vScrollbar + }); + + const clone = e.clone(); + + const cloneViewport = clone.findByName('viewport'); + const cloneContent = clone.findByName('content'); + const cloneHScrollbar = clone.findByName('hScrollbar'); + const cloneVScrollbar = clone.findByName('vScrollbar'); + + expect(cloneViewport).to.exist; + expect(cloneViewport).to.not.equal(viewport); + expect(cloneContent).to.not.equal(content); + expect(cloneHScrollbar).to.not.equal(hScrollbar); + expect(cloneVScrollbar).to.not.equal(vScrollbar); + + expect(clone.scrollview.viewportEntity).to.equal(cloneViewport); + expect(clone.scrollview.contentEntity).to.equal(cloneContent); + expect(clone.scrollview.horizontalScrollbarEntity).to.equal(cloneHScrollbar); + expect(clone.scrollview.verticalScrollbarEntity).to.equal(cloneVScrollbar); + }); + + }); + + describe('resolveDuplicatedEntityReferenceProperties', function () { + + it('remaps every entity ref through duplicatedIdsMap', function () { + const viewport = new Entity(); + const content = new Entity(); + const hScrollbar = new Entity(); + const vScrollbar = new Entity(); + + const newViewport = new Entity(); + const newContent = new Entity(); + const newHScrollbar = new Entity(); + const newVScrollbar = new Entity(); + + const source = new Entity(); + source.addComponent('scrollview', { + viewportEntity: viewport, + contentEntity: content, + horizontalScrollbarEntity: hScrollbar, + verticalScrollbarEntity: vScrollbar + }); + + const target = new Entity(); + target.addComponent('scrollview'); + + const map = { + [viewport.getGuid()]: newViewport, + [content.getGuid()]: newContent, + [hScrollbar.getGuid()]: newHScrollbar, + [vScrollbar.getGuid()]: newVScrollbar + }; + + target.scrollview.resolveDuplicatedEntityReferenceProperties(source.scrollview, map); + + expect(target.scrollview.viewportEntity).to.equal(newViewport); + expect(target.scrollview.contentEntity).to.equal(newContent); + expect(target.scrollview.horizontalScrollbarEntity).to.equal(newHScrollbar); + expect(target.scrollview.verticalScrollbarEntity).to.equal(newVScrollbar); + }); + + }); + +}); From ffbe80da3f801d0957209ccc7ab823f799ffe34a Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Tue, 26 May 2026 10:35:56 +0100 Subject: [PATCH 2/3] fix(scrollview): broaden mouseWheelSensitivity input + tear down refs on remove Two follow-ups from Copilot's review on #8777: - `mouseWheelSensitivity` setter was narrower than the old schema-driven `convertValue('vec2')`: it only normalized `Array.isArray(arg)`, so a Float32Array (or any other indexable input) would slip through to the passthrough branch and break later code that reads `.x`/`.y`. Match the old contract: nullish passes through, Vec2 is cloned, anything else is treated as indexable. - `onRemove` only detached the entity's own `element:add` and element listeners. Listeners registered on referenced viewport / content / scrollbar entities, plus the pending `_evtElementRemove` once-handle on the element, stayed live - keeping the removed component reachable from those entities. Set every entity ref to null (which routes through the existing setter -> unsubscribe path) and `.off()` the pending once-handle. This is also a pre-existing leak, not a regression from the refactor. Adds tests for both: a Float32Array round-trip on `mouseWheelSensitivity`, and a `removeComponent` test asserting that referenced entities no longer have any of the component's listeners. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/scroll-view/component.js | 32 +++++++++++--- .../components/scroll-view/component.test.mjs | 43 +++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/src/framework/components/scroll-view/component.js b/src/framework/components/scroll-view/component.js index 182284820e8..1b6add1b35e 100644 --- a/src/framework/components/scroll-view/component.js +++ b/src/framework/components/scroll-view/component.js @@ -394,14 +394,18 @@ class ScrollViewComponent extends Component { * @type {Vec2} */ set mouseWheelSensitivity(arg) { - if (arg instanceof Vec2) { - // Clone so callers can pass shared singletons (e.g. Vec2.ONE) and so subsequent - // mutations to their Vec2 don't leak into the component's state. + // Mirror the old schema-driven `type: 'vec2'` conversion in + // ComponentSystem.convertValue: pass null/undefined/0/'' through untouched, + // clone a Vec2 input (so callers can pass shared singletons like Vec2.ONE + // without mutations leaking into component state), and treat anything else + // as indexable to support `[x, y]` arrays and typed arrays from JSON-loaded + // scenes. + if (!arg) { + this._mouseWheelSensitivity = arg; + } else if (arg instanceof Vec2) { this._mouseWheelSensitivity = arg.clone(); - } else if (Array.isArray(arg)) { - this._mouseWheelSensitivity = new Vec2(arg[0], arg[1]); } else { - this._mouseWheelSensitivity = arg; + this._mouseWheelSensitivity = new Vec2(arg[0], arg[1]); } } @@ -1324,6 +1328,22 @@ class ScrollViewComponent extends Component { onRemove() { this._evtElementAdd?.off(); this._evtElementAdd = null; + + // `_evtElementRemove` is a `once('beforeremove', ...)` handle registered on + // `this.entity.element`. If the scrollview is removed while the element survives, + // the once handler would dangle on the element and keep this component referenced. + this._evtElementRemove?.off(); + this._evtElementRemove = null; + + // Setting each entity ref to null routes through the existing setter and triggers + // the matching `_*EntityUnsubscribe`, so any listeners we registered on those + // referenced entities (viewport/content/scrollbar `element:add`, `scrollbar:add`, + // element resize, scrollbar `set:value`, etc.) are torn down here. + this.viewportEntity = null; + this.contentEntity = null; + this.horizontalScrollbarEntity = null; + this.verticalScrollbarEntity = null; + this._toggleElementListeners('off'); this._destroyDragHelper(); } diff --git a/test/framework/components/scroll-view/component.test.mjs b/test/framework/components/scroll-view/component.test.mjs index 9c6611a2132..8578a8faa48 100644 --- a/test/framework/components/scroll-view/component.test.mjs +++ b/test/framework/components/scroll-view/component.test.mjs @@ -156,6 +156,19 @@ describe('ScrollViewComponent', function () { expect(e.scrollview.mouseWheelSensitivity.y).to.equal(3); }); + it('accepts a typed array (any indexable value) the same as a plain array', function () { + // The old schema-driven `type: 'vec2'` conversion in ComponentSystem.convertValue + // accepted any indexable input - including typed arrays. The setter must preserve + // that contract; otherwise scenes/templates that ship a Float32Array would store + // the raw buffer and later code that reads `.x`/`.y` would NaN out. + const e = new Entity(); + e.addComponent('scrollview', { mouseWheelSensitivity: new Float32Array([2, 3]) }); + + expect(e.scrollview.mouseWheelSensitivity).to.be.an.instanceof(Vec2); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(2); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(3); + }); + }); describe('#horizontal', function () { @@ -455,6 +468,36 @@ describe('ScrollViewComponent', function () { }); + describe('removeComponent', function () { + + it('tears down every listener it registered on referenced entities', function () { + const { e, viewport, content, hScrollbar, vScrollbar } = buildScrollViewEntity(); + e.addComponent('scrollview', { + viewportEntity: viewport, + contentEntity: content, + horizontalScrollbarEntity: hScrollbar, + verticalScrollbarEntity: vScrollbar + }); + app.root.addChild(e); + + // sanity: the component has wired itself into each referenced entity + expect(viewport.hasEvent('element:add')).to.equal(true); + expect(content.hasEvent('element:add')).to.equal(true); + expect(hScrollbar.hasEvent('scrollbar:add')).to.equal(true); + expect(vScrollbar.hasEvent('scrollbar:add')).to.equal(true); + + e.removeComponent('scrollview'); + + // every listener registered on a referenced entity must be gone, otherwise + // the entities would keep callbacks pointing at a removed component + expect(viewport.hasEvent('element:add')).to.equal(false); + expect(content.hasEvent('element:add')).to.equal(false); + expect(hScrollbar.hasEvent('scrollbar:add')).to.equal(false); + expect(vScrollbar.hasEvent('scrollbar:add')).to.equal(false); + }); + + }); + describe('#cloneComponent', function () { it('clones every scalar property', function () { From f369e68d4d9f0e278ead7155cd37bec6df23ddc5 Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Tue, 26 May 2026 10:49:53 +0100 Subject: [PATCH 3/3] fix(scrollview): skip explicit undefined in initializeComponentData Copilot review on #8777 caught a behavior regression: the old initializeComponentData normalized dragThreshold / useMouseWheel / mouseWheelSensitivity when they were `=== undefined`, so callers shipping `{ dragThreshold: undefined }` still ended up with the default 10. The refactored loop guarded with `hasOwnProperty`, which let explicit undefined values clobber the class-field defaults - turning `useMouseWheel` falsy (disables wheel scrolling) and `mouseWheelSensitivity` into undefined (`_onMouseWheel` then throws on `.x`/`.y`). Tighten the guard to `data[property] !== undefined` so the class-field defaults survive explicit-undefined input, exactly matching the legacy behavior. Adds a test that addComponent with `{ dragThreshold: undefined, useMouseWheel: undefined, mouseWheelSensitivity: undefined }` lands on the documented defaults. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/scroll-view/system.js | 7 ++++++- .../components/scroll-view/component.test.mjs | 20 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/framework/components/scroll-view/system.js b/src/framework/components/scroll-view/system.js index b6bce3d839e..8dcdfdd66ff 100644 --- a/src/framework/components/scroll-view/system.js +++ b/src/framework/components/scroll-view/system.js @@ -60,7 +60,12 @@ class ScrollViewComponentSystem extends ComponentSystem { initializeComponentData(component, data, properties) { for (let i = 0; i < _properties.length; i++) { const property = _properties[i]; - if (data.hasOwnProperty(property)) { + // Skip explicit `undefined` so the component's class-field defaults survive. + // The old initializer normalized dragThreshold / useMouseWheel / + // mouseWheelSensitivity when they were `=== undefined`; a `hasOwnProperty` + // guard alone would clobber those defaults with `undefined` if a caller + // shipped `{ dragThreshold: undefined }`. + if (data[property] !== undefined) { component[property] = data[property]; } } diff --git a/test/framework/components/scroll-view/component.test.mjs b/test/framework/components/scroll-view/component.test.mjs index 8578a8faa48..146a2227342 100644 --- a/test/framework/components/scroll-view/component.test.mjs +++ b/test/framework/components/scroll-view/component.test.mjs @@ -89,6 +89,26 @@ describe('ScrollViewComponent', function () { expect(e.scrollview.verticalScrollbarEntity).to.equal(null); }); + it('preserves class-field defaults when data contains explicit undefined values', function () { + // The legacy initializer normalized dragThreshold / useMouseWheel / + // mouseWheelSensitivity when they were `=== undefined`, so callers shipping + // `{ dragThreshold: undefined }` still got the default 10. The refactored + // loop must do the same so an explicit undefined in the data argument does + // not clobber the class-field defaults. + const e = new Entity(); + e.addComponent('scrollview', { + dragThreshold: undefined, + useMouseWheel: undefined, + mouseWheelSensitivity: undefined + }); + + expect(e.scrollview.dragThreshold).to.equal(10); + expect(e.scrollview.useMouseWheel).to.equal(true); + expect(e.scrollview.mouseWheelSensitivity).to.be.an.instanceof(Vec2); + expect(e.scrollview.mouseWheelSensitivity.x).to.equal(1); + expect(e.scrollview.mouseWheelSensitivity.y).to.equal(1); + }); + it('round-trips every property passed via the data argument', function () { const { e, viewport, content, hScrollbar, vScrollbar } = buildScrollViewEntity(); const sensitivity = new Vec2(2, 3);