Skip to content

Feature: Add method handling to mixin processing in KDL #2100

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.

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
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Bashamega
Copy link
Contributor

related #2053

Copy link
Contributor

github-actions bot commented Aug 3, 2025

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.

@@ -0,0 +1,12 @@
// Manually moved from Document
Copy link
Contributor

Choose a reason for hiding this comment

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

cssom-view.kdl

function handleMethod(child: Node): Partial<Method> {
const name = child.values[0] as string;
// Build the overrideSignatures array with the method signature string
const overrideSignatures = [
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use overrideSignatures unless it's explicitly used. Take a look at widlprocess.ts to see how to proceed: (this PR does not need to cover everything in that file)

} else if (member.type === "operation") {
const operation = convertOperation(member, result.exposed);
const { method } = result.methods;
if (!member.name) {
result.anonymousMethods!.method.push(operation);
} else if (method.hasOwnProperty(member.name)) {
method[member.name].signature.push(...operation.signature);
} else {
method[member.name] = operation as Browser.Method;
}
if (member.name) {
addComments(method[member.name], commentMap, i.name, member.name);
}

Bashamega and others added 3 commits August 3, 2025 16:50
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
…meters and update return types for elementFromPoint and elementsFromPoint methods. Enhance method handling in patches.ts to accommodate nullable return types and array signatures.
@Bashamega
Copy link
Contributor Author

I've updated it, @saschanaz.
I used array instead of wrapping the return value with a sequence. If you still prefer your version, I can make it work with a regex. That said, I believe the current implementation is better for the parser.

@@ -0,0 +1,12 @@
// Manually moved from Document
Copy link
Contributor

Choose a reason for hiding this comment

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

.kdl, not .kd 😛

Also maybe 2-space indent to be consistent with the rest of the code 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed the name

Copy link
Contributor

Choose a reason for hiding this comment

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

but not the indent 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot, I have updated them

param x type=long
param y type=long
}
method elementsFromPoint returns="Element" array {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to follow the syntax of

export interface Typed {
type: string | Typed[];
subtype?: Typed | Typed[];
nullable?: boolean;
overrideType?: string;
additionalTypes?: string[];
allowShared?: boolean;
}

But that doesn't seem possible as a property. Perhaps we should allow (but not force) type as a member for complex cases like this.

method elementsFromPoint {
  type sequence {
     type Element
  }
  param x type=long
  param y type=long
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@Bashamega
Copy link
Contributor Author

I have updated it @saschanaz

@Bashamega
Copy link
Contributor Author

Hello @saschanaz
Do you have any more requests for this PR?

// Manually moved from Document
// See https://github.com/w3c/csswg-drafts/issues/5886 and https://github.com/w3c/csswg-drafts/issues/556
interface-mixin DocumentOrShadowRoot {
method elementFromPoint returns=Element nullable=#true {
Copy link
Contributor

Choose a reason for hiding this comment

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

nullable is logically a property of the return type rather than the method, so in this case it would be also nicer to use separate type node as below.


const signature: Method["signature"] = [
{
type: type.name == "type" ? (type.values[0] as string) : returnType,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably get similar situation on other things than methods, this should probably be done in a helper function so that others can reuse it, something like handleTyped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made it a helper function

const nullable = child.properties.nullable as boolean;

const params = child.children
.filter((c) => c.values[0] != "sequence")
Copy link
Contributor

Choose a reason for hiding this comment

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

It should filter in based on the type param, not filter out based on name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Bashamega
Copy link
Contributor Author

I have updated it @saschanaz

@Bashamega Bashamega requested a review from saschanaz August 7, 2025 06:23
@Bashamega
Copy link
Contributor Author

Hello @saschanaz
Do you have any more requests for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants