Skip to content

feat: reimplement XFoldViewNg scrolling using CSS transforms and cust… #2299

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.

Sign up for GitHub

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

Open
PupilTong wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: reimplement XFoldViewNg scrolling using CSS transforms and cust… #2299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0014d21
feat: reimplement XFoldViewNg scrolling using CSS transforms and cust…
PupilTong Mar 4, 2026
6179978
+ changeset
PupilTong Mar 4, 2026
bf134ef
refactor: improve scrollTop value clamping and switch touch event han…
PupilTong Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/slimy-cows-skip.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@lynx-js/web-elements": minor
---

feat: reimplement `XFoldViewNg` scrolling using CSS transforms and custom scroll handling, updating related events, styles, and tests.

this breaks https://github.com/lynx-family/lynx-stack/pull/878

The position:fixed elements in x-foldview-header-ng and x-foldview-slot-ng will be affected.
1 change: 1 addition & 0 deletions packages/web-platform/web-elements/rsbuild.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default defineConfig({
output: {
assetPrefix: 'auto',
polyfill: 'off',
overrideBrowserslist: ['last 1 Chrome versions'],
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Browser targets across web-platform rsbuild configs =="
rg -n "overrideBrowserslist" packages/web-platform -g "rsbuild.config.ts"

echo
echo "== Polyfill settings in same configs =="
rg -n "polyfill\s*:" packages/web-platform -g "rsbuild.config.ts"

echo
echo "== Browser support policy docs =="
fd -HI "AGENTS.md|README.md|browserslist|.browserslistrc" packages/web-platform

Repository: lynx-family/lynx-stack

Length of output: 1496

🏁 Script executed:

cat packages/web-platform/web-elements/AGENTS.md

Repository: lynx-family/lynx-stack

Length of output: 9923

🏁 Script executed:

head -50 packages/web-platform/web-elements/README.md

Repository: lynx-family/lynx-stack

Length of output: 1461

Consider documenting the rationale for 'last 1 Chrome versions' in a comment.

The narrower browser target is intentional—web-elements has a documented compat module (e.g., LinearContainer) with polyfills for missing features. Setting polyfill: 'off' is consistent across web-platform packages and allows the package to manage its own compatibility layer. However, adding a comment in rsbuild.config.ts explaining why this target was chosen would improve clarity for future maintenance, especially since it's narrower than other web-platform packages (e.g., 'last 2 Chrome versions' in web-rsbuild-server-middleware).

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/web-platform/web-elements/rsbuild.config.ts` at line 14, Add a short
inline comment above the overrideBrowserslist setting in rsbuild.config.ts
explaining why it uses 'last 1 Chrome versions' (e.g., package manages its own
compatibility via the compat module like LinearContainer, polyfill: 'off' is
intentional, and this package intentionally targets a narrower Chrome range
unlike other packages). Reference the overrideBrowserslist property and mention
the compat module name (LinearContainer) and polyfill: 'off' so future
maintainers understand the rationale.

distPath: {
root: 'www',
css: '.',
Expand Down
40 changes: 27 additions & 13 deletions packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
*/
import { Component } from '../../element-reactive/index.js';
import { CommonEventsAndMethods } from '../common/CommonEventsAndMethods.js';
import { resizeObserver, type XFoldviewNg } from './XFoldviewNg.js';
import { getCombinedDirectParentElement } from '../common/getCombinedParentElement.js';
import { LinearContainer } from '../../compat/index.js';
import { updateHeaderHeight, XFoldviewNg } from './XFoldviewNg.js';

@Component<typeof XFoldviewHeaderNg>(
'x-foldview-header-ng',
Expand All @@ -17,19 +16,34 @@ import { LinearContainer } from '../../compat/index.js';
],
)
export class XFoldviewHeaderNg extends HTMLElement {
#parentResizeObserver: ResizeObserver | undefined = undefined;
#resizeObserver?: ResizeObserver;

connectedCallback() {
const parentElement = getCombinedDirectParentElement<XFoldviewNg>(
this,
'X-FOLDVIEW-NG',
);
this.#parentResizeObserver = parentElement?.[resizeObserver];
this.#parentResizeObserver?.observe(this);
this.#resizeObserver = new ResizeObserver((entries) => {
for (const entry of entries) {
this.#updateParent(entry.contentRect.height);
}
});
this.#resizeObserver.observe(this);
this.#updateParent(this.clientHeight);
}

disconnectedCallback() {
this.#resizeObserver?.disconnect();
this.#resizeObserver = undefined;
}

dispose() {
this.#parentResizeObserver?.unobserve(
this,
);
#updateParent(height: number) {
let parent = this.parentElement;
while (parent) {
if (parent instanceof XFoldviewNg) {
parent[updateHeaderHeight](height);
break;
}
if (parent.tagName !== 'LYNX-WRAPPER') {
break;
}
parent = parent.parentElement;
}
}
}
106 changes: 76 additions & 30 deletions packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,57 +6,108 @@
import { Component } from '../../element-reactive/index.js';
import { CommonEventsAndMethods } from '../common/CommonEventsAndMethods.js';
import { XFoldviewNgEvents } from './XFoldviewNgEvents.js';
import { XFoldviewNgTouchEventsHandler } from './XFoldviewNgTouchEventsHandler.js';
import { scrollContainerDom } from '../common/constants.js';
import type { XFoldviewSlotNg } from './XFoldviewSlotNg.js';
import { LinearContainer } from '../../compat/index.js';

export const scrollableLength = Symbol('scrollableLength');
export const isHeaderShowing = Symbol('isHeaderShowing');
export const resizeObserver = Symbol('resizeObserver');
export const slotKid = Symbol('slotKid');
export const updateHeaderHeight = Symbol('updateHeaderHeight');
export const updateToolbarHeight = Symbol('updateToolbarHeight');
export const scrollCallbacks = Symbol('scrollCallbacks');

@Component<typeof XFoldviewNg>('x-foldview-ng', [
LinearContainer,
CommonEventsAndMethods,
XFoldviewNgEvents,
XFoldviewNgTouchEventsHandler,
])
export class XFoldviewNg extends HTMLElement {
static readonly notToFilterFalseAttributes = new Set(['scroll-enable']);
[slotKid]?: XFoldviewSlotNg;
[resizeObserver]?: ResizeObserver = new ResizeObserver((resizeEntries) => {
for (const resize of resizeEntries) {
if (resize.target.tagName === 'X-FOLDVIEW-HEADER-NG') {
this.#headerHeight = resize.contentRect.height;
} else if (resize.target.tagName === 'X-FOLDVIEW-TOOLBAR-NG') {
this.#toolbarHeight = resize.contentRect.height;
}
}
if (this[slotKid]) {
this[slotKid].style.top = `${this.#headerHeight - this.#toolbarHeight}px`;
}
});
#headerHeight: number = 0;
#toolbarHeight: number = 0;
[scrollCallbacks]: Set<() => void> = new Set();

[updateHeaderHeight](height: number) {
this.#headerHeight = height;
this.style.setProperty(
'--foldview-scroll-height',
this.scrollHeight + 'px',
);
}

get [scrollableLength](): number {
[updateToolbarHeight](height: number) {
this.#toolbarHeight = height;
this.style.setProperty(
'--foldview-scroll-height',
this.scrollHeight + 'px',
);
}

override get scrollHeight(): number {
return this.#headerHeight - this.#toolbarHeight;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
get [isHeaderShowing](): boolean {
// This behavior cannot be reproduced in the current test, but can be reproduced in Android WebView
return this[scrollableLength] - this.scrollTop >= 1;
return this.scrollHeight - this.scrollTop >= 1;
}

get scrollableLength(): number {
return this.scrollHeight;
}

#scrollTop: number = 0;

override get scrollTop() {
return super.scrollTop;
return this.#scrollTop;
}

override set scrollTop(value: number) {
if (value > this[scrollableLength]) {
value = this[scrollableLength];
} else if (value < 0) {
value = 0;
const maxScroll = Math.max(this.scrollHeight, 0);
value = Math.max(0, Math.min(value, maxScroll));
if (this.#scrollTop === value) {
return;
}
this.#scrollTop = value;
this.style.setProperty(
'--foldview-scroll-top',
(0 - value).toString() + 'px',
);
this.dispatchEvent(new Event('scroll'));
for (const callback of this[scrollCallbacks]) {
callback();
}
}

override scrollTo(options?: ScrollToOptions): void;
override scrollTo(x: number, y: number): void;
override scrollTo(arg1?: any, arg2?: any): void {
if (typeof arg1 === 'object') {
const { top, behavior } = arg1;
if (typeof top === 'number') {
if (behavior === 'smooth') {
// TODO: implement smooth scroll if needed, for now just instant
this.scrollTop = top;
} else {
this.scrollTop = top;
}
}
} else if (typeof arg2 === 'number') {
this.scrollTop = arg2;
}
}
Comment on lines +83 to +97
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the XFoldViewNg file
fd "XFoldviewNg.ts" packages/web-platform/web-elements/src/

Repository: lynx-family/lynx-stack

Length of output: 140

🏁 Script executed:

# Get the file and see the full context around lines 73 and 83-97
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | head -120 | tail -60

Repository: lynx-family/lynx-stack

Length of output: 2224

🏁 Script executed:

# Also check the entire file structure to understand the class
wc -l packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts

Repository: lynx-family/lynx-stack

Length of output: 144

🏁 Script executed:

# Check the scrollBy method to see if there's a pattern
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | sed -n '99,111p'

Repository: lynx-family/lynx-stack

Length of output: 579

🏁 Script executed:

# Look for similar scrollTo implementations in other element files
fd "\.ts$" packages/web-platform/web-elements/src/elements/ | xargs grep -l "scrollTo" | head -5

Repository: lynx-family/lynx-stack

Length of output: 441

🏁 Script executed:

# Test what happens with NaN/Infinity in the clamping logic using node
node <<'EOF'
// Test Math.max/Math.min with NaN and Infinity
const maxScroll = 100;

// NaN case
let valueNaN = NaN;
valueNaN = Math.max(0, Math.min(valueNaN, maxScroll));
console.log('NaN result:', valueNaN, 'toString:', valueNaN.toString() + 'px');

// Infinity case  
let valueInf = Infinity;
valueInf = Math.max(0, Math.min(valueInf, maxScroll));
console.log('Infinity result:', valueInf, 'toString:', valueInf.toString() + 'px');

// Test null destructuring
try {
  const { top } = null;
  console.log('null destructure: no error');
} catch(e) {
  console.log('null destructure error:', e.message);
}
EOF

Repository: lynx-family/lynx-stack

Length of output: 217

🏁 Script executed:

# Let's verify the exact clamping behavior with NaN more carefully
node <<'EOF'
const maxScroll = 100;

// Detailed NaN test
let value = NaN;
console.log('Initial:', value);
console.log('After Math.min(value, maxScroll):', Math.min(value, maxScroll));
console.log('After Math.max(0, Math.min(value, maxScroll)):', Math.max(0, Math.min(value, maxScroll)));

// Verify Number.isFinite
console.log('Number.isFinite(NaN):', Number.isFinite(NaN));
console.log('Number.isFinite(Infinity):', Number.isFinite(Infinity));
console.log('Number.isFinite(100):', Number.isFinite(100));
EOF

Repository: lynx-family/lynx-stack

Length of output: 255

🏁 Script executed:

# Check if scrollBy has the same issue and if it's also affected
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | sed -n '101,111p'

Repository: lynx-family/lynx-stack

Length of output: 462

Add guards against null and non-finite values in scrollTo and scrollBy.

Line 84 accepts null because typeof null === 'object', causing a destructuring error at line 85. Additionally, NaN passes through the clamping logic in the scrollTop setter (line 66) and produces invalid CSS: "NaNpx". The scrollBy method has the same vulnerabilities.

🐛 Proposed fix 
  override scrollTo(arg1?: any, arg2?: any): void {
-    if (typeof arg1 === 'object') {
+    if (typeof arg1 === 'object' && arg1 !== null) {
      const { top, behavior } = arg1;
-      if (typeof top === 'number') {
+      if (typeof top === 'number' && Number.isFinite(top)) {
         if (behavior === 'smooth') {
           // TODO: implement smooth scroll if needed, for now just instant
           this.scrollTop = top;
         } else {
           this.scrollTop = top;
         }
       }
-    } else if (typeof arg2 === 'number') {
+    } else if (typeof arg2 === 'number' && Number.isFinite(arg2)) {
       this.scrollTop = arg2;
     }
   }

   override scrollBy(arg1?: any, arg2?: any): void {
-    if (typeof arg1 === 'object') {
+    if (typeof arg1 === 'object' && arg1 !== null) {
       const { top, behavior } = arg1;
       this.scrollTo({
         top: (typeof top === 'number' ? top : 0) + this.scrollTop,
         behavior,
       });
     } else {
       this.scrollTo(0, this.scrollTop + (arg2 || 0));
     }
   }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`
around lines 83 - 97, The scrollTo implementation can throw when arg1 is null
and can set NaN into the scrollTop setter; update scrollTo and scrollBy to first
guard that object arguments are non-null (arg1 !== null) and that extracted
numeric values (e.g., top, left, deltaY) are finite via Number.isFinite before
assigning to this.scrollTop/this.scrollLeft; additionally ensure the branch that
handles (typeof arg2 === 'number') also validates Number.isFinite(arg2) before
using it. Locate and modify the override scrollTo method and the scrollBy method
to perform these null and finite checks and skip/ignore invalid values so the
scrollTop setter never receives NaN or undefined.


override scrollBy(options?: ScrollToOptions): void;
override scrollBy(x: number, y: number): void;
override scrollBy(arg1?: any, arg2?: any): void {
if (typeof arg1 === 'object') {
const { top, behavior } = arg1;
this.scrollTo({
top: (typeof top === 'number' ? top : 0) + this.scrollTop,
behavior,
});
} else {
this.scrollTo(0, this.scrollTop + (arg2 || 0));
}
super.scrollTop = value;
}

setFoldExpanded(params: { offset: string; smooth: boolean }) {
Expand All @@ -71,9 +122,4 @@ export class XFoldviewNg extends HTMLElement {
get [scrollContainerDom]() {
return this;
}

disconnectedCallback() {
this[resizeObserver]?.disconnect();
this[resizeObserver] = undefined;
}
}
6 changes: 3 additions & 3 deletions packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
registerEventEnableStatusChangeHandler,
} from '../../element-reactive/index.js';
import { commonComponentEventSetting } from '../common/commonEventInitConfiguration.js';
import { scrollableLength, type XFoldviewNg } from './XFoldviewNg.js';
import { type XFoldviewNg } from './XFoldviewNg.js';

export class XFoldviewNgEvents
implements InstanceType<AttributeReactiveClass<typeof XFoldviewNg>>
Expand Down Expand Up @@ -46,7 +46,7 @@ export class XFoldviewNgEvents
scrollLength > this.#granularity
|| this.#dom.scrollTop === 0
|| Math.abs(
this.#dom.scrollHeight - this.#dom.clientHeight - this.#dom.scrollTop,
this.#dom.scrollHeight - this.#dom.scrollTop,
) <= 1
) {
this.#pervScroll = currentScrollTop;
Expand All @@ -55,7 +55,7 @@ export class XFoldviewNgEvents
...commonComponentEventSetting,
detail: {
offset: currentScrollTop,
height: this.#dom[scrollableLength],
height: this.#dom.scrollableLength,
},
}),
);
Expand Down
Loading
Loading