override scroll methods to return void until browser implementation changes #2237

Bashamega · 2025-11-05T05:35:00Z

Related #2053

github-actions · 2025-11-05T05:35:20Z

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

saschanaz · 2025-11-05T06:26:24Z

inputfiles/patches/scroll.kdl

@@ -0,0 +1,9 @@
+interface Element {
+    method scroll overrideType=void


type=undefined should do it as an IDL type

As you see in #2196 it's more than just Element but also Window.

saschanaz · 2025-11-05T06:28:16Z

Title should be "override scroll methods to return void until browser implementation changes" or something.

saschanaz · 2025-11-05T06:29:22Z

src/build/patches.ts

-        method[methodName] = handleMethod(child);
+        const m = handleMethod(child);
+        if (method[methodName]) {
+          // ts: The goal here is to merge multiple method signatures together for methods with the same name.


~~we already have a better solution in another PR, no? This is weird.~~

I take this back and added another comment below.

saschanaz · 2025-11-05T06:42:53Z

src/build/patches.ts

+    | DeepPartial<Signature>[]
+    | Record<string, DeepPartial<Signature>> = child.properties?.overrideType
+    ? {
+        "0": {


This certainly is the key but I don't think we should do it this way, What if we only want to override the second signature but leave the first one as-is?

What about something like:

method name signatureIndex=0 type=undefined

And then we return object as signature list instead of an array if signatureIndex exists?

…or better handling of overloaded methods

Bashamega · 2025-11-05T09:24:15Z

I have updated it @saschanaz

I couldn't update window, because it gave me this error
file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:766
method.signature.forEach((sig) => emitSignature(sig, prefix, name, printLine, shouldResolvePromise));
^

TypeError: method.signature.forEach is not a function
at emitSignatures (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:766:30)
at emitMethod (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:751:9)
at file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:790:33
at Array.forEach ()
at emitMethods (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:790:18)
at emitMembers (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:830:9)
at emitInterface (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:1063:9)
at emitNonCallbackInterfaces (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:1092:17)
at emit (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:1184:9)
at emitWebIdl (file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:156:21)

saschanaz

Looks much better, thanks.

saschanaz · 2025-11-05T10:19:16Z

src/build/patches.ts

  : T;

+interface Method extends Omit<originalMethod, "signature"> {
+  signature: DeepPartial<Signature>[] | Record<string, DeepPartial<Signature>>;


It's more like Record<number, ...>, and maybe name the interface something like OverridableMethod, so that we don't have to rename the pure Method

saschanaz · 2025-11-05T10:23:12Z

src/build/patches.ts

+      ? handleTyped(typeNode)
+      : {
+          type: string(child.properties?.returns),
+          subtype: undefined,


why subtype?

With out it it will become undefind<undefind>

Hmm, fair. if one needs subtype they should use nested types.

saschanaz · 2025-11-05T10:26:55Z

src/build/patches.ts

+  };
+
+  let signature: Method["signature"];
+  const signatureIndex = child.properties?.signatureIndex;


time to add number()?

Why? Sometimes it is undefined

saschanaz · 2025-11-05T10:29:23Z

I couldn't update window, because it gave me this error file:///home/adam/os/TypeScript-DOM-lib-generator/lib/build/emitter.js:766 method.signature.forEach((sig) => emitSignature(sig, prefix, name, printLine, shouldResolvePromise)); ^

Meaning you may have touched a non-existent method. signatureIndex would work only for an existing method. Note that not all Element methods exist in Window and try again based on that?

saschanaz · 2025-11-05T10:38:32Z

Much more concise than the initial PR. Thanks, LGTM

github-actions · 2025-11-05T10:38:53Z

There was an issue merging, maybe try again saschanaz. Details

Bashamega · 2025-11-05T10:39:10Z

I am still working on the rest

saschanaz · 2025-11-05T10:40:53Z

Oh right, the window methods.

Bashamega · 2025-11-05T10:46:00Z

Done @saschanaz

saschanaz · 2025-11-05T11:07:20Z

inputfiles/patches/scroll.kdl

+    method scrollBy returns=void signatureIndex=1
+    method scrollTo returns=void signatureIndex=0
+    method scrollTo returns=void signatureIndex=1
+}


Lastly let's put this in cssom-view.kdl

With some comments like "No implementation of Promise return values as of 2025-11"

Bashamega · 2025-11-05T11:11:59Z

I have updated it @saschanaz

inputfiles/patches/cssom-view.kdl

saschanaz · 2025-11-05T11:13:53Z

LGTM2

github-actions · 2025-11-05T11:14:06Z

There was an issue merging, maybe try again saschanaz. Details

Bashamega · 2025-11-05T15:55:19Z

Can you review this @jakebailey
Thank you 😊

jakebailey

I feel like this is futile given 6.0 isn't even out and neither is the change, but a pure revert for now I guess is fine. Just seems silly to argue that this matters.

turansky · 2025-11-06T01:00:23Z

Can we use Promise<void> | void as return type?
It's expected baseline for nearest 2 years.

saschanaz · 2025-11-06T01:08:00Z

We will migrate to Promise<void> if at least two browser engines are ready.

saschanaz · 2025-11-06T01:09:02Z

LGTM

github-actions · 2025-11-06T01:09:16Z

There was an issue merging, maybe try again saschanaz. Details

Bashamega added 2 commits November 5, 2025 07:34

Fix: Async scroll...() methods are non-async in fact

3402469

-

dd5502e

saschanaz reviewed Nov 5, 2025

View reviewed changes

Bashamega changed the title ~~Fix: Async scroll...() methods are non-async in fact~~ override scroll methods to return void until browser implementation changes Nov 5, 2025

Bashamega added 6 commits November 5, 2025 10:53

Refactor: Update method signatures in KDL to include signatureIndex f…

de9e7bd

…or better handling of overloaded methods

-

6c19cb1

-

8c2b39b

-

abe52d7

-

588890f

OverrideType

3f96aea

saschanaz reviewed Nov 5, 2025

View reviewed changes

-

8f5ab49

Bashamega added 2 commits November 5, 2025 12:45

-

add63b1

-

29a3216

saschanaz reviewed Nov 5, 2025

View reviewed changes

Move

2730b62

saschanaz reviewed Nov 5, 2025

View reviewed changes

inputfiles/patches/cssom-view.kdl Outdated Show resolved Hide resolved

Update inputfiles/patches/cssom-view.kdl

b7761c0

jakebailey approved these changes Nov 5, 2025

View reviewed changes

jakebailey merged commit 3ebbf4f into microsoft:main Nov 6, 2025
7 checks passed

Bashamega deleted the remove-promise branch November 6, 2025 03:49

		@@ -0,0 +1,9 @@
		interface Element {
		method scroll overrideType=void

override scroll methods to return void until browser implementation changes #2237

override scroll methods to return void until browser implementation changes #2237

Conversation

Bashamega commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Nov 5, 2025

Uh oh!

saschanaz Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Nov 5, 2025

Uh oh!

saschanaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Nov 5, 2025

Uh oh!

saschanaz commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Bashamega commented Nov 5, 2025

Uh oh!

saschanaz commented Nov 5, 2025

Uh oh!

Bashamega commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Nov 5, 2025

Uh oh!

Uh oh!

saschanaz commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Bashamega commented Nov 5, 2025

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

turansky commented Nov 6, 2025

Uh oh!

saschanaz commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Nov 6, 2025

Uh oh!

github-actions bot commented Nov 6, 2025

saschanaz Nov 5, 2025 •

edited

Loading

saschanaz Nov 5, 2025 •

edited

Loading

saschanaz commented Nov 6, 2025 •

edited

Loading