Skip to content

[css-highlight-api] Change highlightsFromPoint return type #12031 #12215

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

Merged
merged 7 commits into from
Jun 22, 2025

Conversation

ffiori
Copy link
Member

@ffiori ffiori commented May 20, 2025

[css-highlight-api] Change highlightsFromPoint return type #12031

As discussed and resolved in #12031, this PR changes the return type for highlightsFromPoint to sequence<HighlightHitResult> to include the ranges that are hit at a given point in addition to the custom highlights.

This PR also resolves the following issues:

@ffiori ffiori requested a review from dandclark May 20, 2025 21:46
@ffiori ffiori assigned ffiori and unassigned ffiori May 20, 2025
@ffiori ffiori marked this pull request as draft May 20, 2025 23:02
@ffiori ffiori marked this pull request as ready for review May 21, 2025 17:40
Copy link
Contributor

@dandclark dandclark left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

For #12045 #12003 #12002, can you include a sentence or so in your PR description for each to mention the way that this PR resolves the issue?

Copy link
Contributor

@dandclark dandclark left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, I think this is ready.
Just one more question on the thread about the exception.

@dandclark dandclark requested a review from frivoal June 4, 2025 19:48
@dandclark
Copy link
Contributor

@frivoal as an editor of https://drafts.csswg.org/css-highlight-api-1/ do you want a chance to take a look at this before we land it?

As a sidenote, I think it would make sense for @ffiori and I to be added as editors if that sounds alright with you. Sanket Joshi is no longer at Microsoft, and Fernando and I have both been involved with this spec for a long time.

@ffiori
Copy link
Member Author

ffiori commented Jun 10, 2025

Hi @frivoal, friendly pinging on this issue and following up on @dandclark's comment. I'd appreciate if you could PTAL, and also wanted to confirm if it makes sense to add me and Daniel as editors.

Thanks in advance!

@ffiori
Copy link
Member Author

ffiori commented Jun 11, 2025

Adding @megangardner as reviewer as well as editor of https://drafts.csswg.org/css-highlight-api-1/. Could you PTAL?

We're also thinking of adding @dandclark and me as editors since Sanket Joshi left Microsoft, does that sound good to you too?

Thanks in advance.

@ffiori ffiori requested a review from megangardner June 11, 2025 20:36
@leotlee leotlee requested a review from sanketj June 20, 2025 19:19
@leotlee
Copy link

leotlee commented Jun 20, 2025

Hi @sanketj , you think you can help review this and maybe add Dan and Fernando as editors? We haven't had luck getting responses from an editor here. Appreciate it, thanks.

Copy link

@megangardner megangardner left a comment

Choose a reason for hiding this comment

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

Looks good to me! Also I am not against adding you both as editors. @frivoal thoughts?

@atanassov
Copy link
Contributor

atanassov commented Jun 20, 2025

It's great to see the PR moving forward. Adding to the agenda to decide and formalize the editors.

@leotlee leotlee self-requested a review June 20, 2025 21:32
@leotlee
Copy link

leotlee commented Jun 20, 2025

Looks good to me! Also I am not against adding you both as editors. @frivoal thoughts?

@frivoal , are you OK with us merging this PR? Also I'd love to get your thoughts on updating editors.

@frivoal
Copy link
Collaborator

frivoal commented Jun 22, 2025

Sorry for the slow answer, I had missed that this conversation was happening. I'm arriving after the battle, but at this point it all looks good. Since we have an outstanding resolution in favor of this change, and multiple people being happy with the details, I'm merging without waiting for the PR to make it to the top of the agenda+ list (though we can mention it on the call when we get to it).

As for adding you as editors, I am certainly in favor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment