Skip to content
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

[css-highlight-api-1] Add highlightsFromPoint() to CSS.highlights #7513 #11507

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ffiori
Copy link
Contributor

@ffiori ffiori commented Jan 15, 2025

Add highlightsFromPoint() to CSS.highlights as discussed in #7513.

@ffiori ffiori marked this pull request as ready for review January 15, 2025 21:29
@ffiori ffiori marked this pull request as draft January 15, 2025 21:33
@ffiori ffiori changed the title add spec for highlightsFromPoint [css-highlight-api] Add highlightsFromPoint() to CSS.highlights Jan 15, 2025
@ffiori ffiori changed the title [css-highlight-api] Add highlightsFromPoint() to CSS.highlights [css-highlight-api-1] Add highlightsFromPoint() to CSS.highlights Jan 15, 2025
@ffiori ffiori changed the title [css-highlight-api-1] Add highlightsFromPoint() to CSS.highlights [css-highlight-api-1] Add highlightsFromPoint() to CSS.highlights #7513 Jan 15, 2025
@ffiori ffiori requested a review from siliu1 January 17, 2025 20:59
Interacting with Custom Highlights</h2>

User interaction with [=custom highlights=] can be done via the
{{highlightsFromPoint}}(<var>x</var>, <var>y</var>, <var>options</var>) method.
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to add another sentence to explain in further details what this API do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even a code sample showing its use in a click event handler or something like that.

<a>viewport</a> height excluding the size of a rendered scroll bar
(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add link to the definition of range?

(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
1. The coordinates <var>x</var>,<var>y</var> fall inside at least one of the rectangles returned by calling getClientRects() on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe link to the definition of getClientRects()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also instead of "calling getClientRects() on it" let's say "calling getClientRects() on <var>range</var>".

Copy link
Contributor

Choose a reason for hiding this comment

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

And instead of "one of the rectangles" we can say "one of the {{DOMRects}}"

@siliu1 siliu1 requested a review from dandclark January 17, 2025 21:22
Interacting with Custom Highlights</h2>

User interaction with [=custom highlights=] can be done via the
{{highlightsFromPoint}}(<var>x</var>, <var>y</var>, <var>options</var>) method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even a code sample showing its use in a click event handler or something like that.

The <dfn method for="HighlightRegistry">highlightsFromPoint(<var>x</var>, <var>y</var>, <var>options</var>)</dfn>
method must return the result of running these steps:

1. If there is no <a>viewport</a> associated with the document, return the empty sequence.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this to link to the definition of sequence?

Suggested change
1. If there is no <a>viewport</a> associated with the document, return the empty sequence.
1. If there is no <a>viewport</a> associated with the document, return the empty [=sequence=].

method must return the result of running these steps:

1. If there is no <a>viewport</a> associated with the document, return the empty sequence.
1. If either argument is negative, <var>x</var> is greater
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest this change to make this step a bit more precise and easy to read:

1. If any of the following are true, return the empty sequence:
    * <var>x</var>is negative
    * <var>y</var>is negative
    * <var>x</var> is greater than the <a>viewport</a> width excluding the size of a rendered scroll bar (if any)
    * <var>y</var> is greater than the <a>viewport</a> height excluding the size of a rendered scroll bar (if any)

1. The range's {{commonAncestorContainer}} is not in a shadow DOM, or is one of the shadow roots passed in <var>options</var> or a
descendant of one of them.

Note: The specifics of hit testing are out of scope of this
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend moving this Note to be adjacent to the step in the algorithm that hand-waves over the hit-testing details.

1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
1. The coordinates <var>x</var>,<var>y</var> fall inside at least one of the rectangles returned by calling getClientRects() on it.
1. The range's {{commonAncestorContainer}} is not in a shadow DOM, or is one of the shadow roots passed in <var>options</var> or a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. The range's {{commonAncestorContainer}} is not in a shadow DOM, or is one of the shadow roots passed in <var>options</var> or a
1. The range's {{commonAncestorContainer}} is not in a [=shadow tree=] or is in a [=shadow tree=]s whose [=shadow root=] is [=contains|contained by=] by <var>options.shadowRoots</var>.

(Please confirm that the definition references here resolve properly. I'm trying to reference the shadow tree definition https://dom.spec.whatwg.org/#concept-shadow-tree, shadow root at https://dom.spec.whatwg.org/#concept-shadow-root, and contains at https://infra.spec.whatwg.org/#list-contain).

scroll bar (if any), or <var>y</var> is greater than the
<a>viewport</a> height excluding the size of a rendered scroll bar
(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
Copy link
Contributor

Choose a reason for hiding this comment

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

The step should say something about where we're getting these highlights, e.g. "Return a sequence of [=custom highlights=] given by ordering the highlights contained in this {{HighlightRegistry}} by [=priority=], excluding the highlights without at least one range that satisfies the following constraints..."

<a>viewport</a> height excluding the size of a rendered scroll bar
(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
where each highlight contains at least one range that satisfies the following constraints:
where each highlight contains at least one {{Range}} <var>range</var> that satisfies the following constraints:

Making this a variable so we can reference it below in the getClientRects() call.

(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
1. The coordinates <var>x</var>,<var>y</var> fall inside at least one of the rectangles returned by calling getClientRects() on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also instead of "calling getClientRects() on it" let's say "calling getClientRects() on <var>range</var>".

(if any), return the empty sequence.
1. Otherwise, return a sequence of custom higlights ordered by [=priority=],
where each highlight contains at least one range that satisfies the following constraints:
1. The coordinates <var>x</var>,<var>y</var> fall inside at least one of the rectangles returned by calling getClientRects() on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

And instead of "one of the rectangles" we can say "one of the {{DOMRects}}"

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.

3 participants