-
Notifications
You must be signed in to change notification settings - Fork 728
[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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
@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. |
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! |
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. |
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. |
There was a problem hiding this 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?
It's great to see the PR moving forward. Adding to the agenda to decide and formalize the editors. |
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. |
[css-highlight-api] Change highlightsFromPoint return type #12031
As discussed and resolved in #12031, this PR changes the return type for
highlightsFromPoint
tosequence<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:
[css-highlight-api] highlightsFromPoint(): Inconsistency between spec and test regarding shadow DOM #12045: Add a step where highlightsFromPoint returns the empty sequence if the topmost box hit by coordinates x,y has an element associated to it that's in a shadow tree whose shadow root is not in the list of shadowRoots passed as an option.
[css-highlight-api] HighlightsFromPoint: Not dealing with invalid static ranges #12003: In the steps for adding an AbstractRange to the ranges attribute of HighlightHitResult, add the necessity for the abstract range to be valid if it's a StaticRange before constructing a Range, and also add a sentence to make sure the Range was successfully constructed.