[css-highlight-api] Change highlightsFromPoint return type #12031 #12215
Conversation
css-highlight-api-1/Overview.bs
Outdated
| descending [=priority=] order. | ||
| involving user interaction with [=custom highlights=]. The method returns a [=sequence=] containing {{HighlightHitResult}} objects | ||
| which encapsulate the [=custom highlights=] and their [=ranges=] that are hit at a given <var>x</var>, <var>y</var> coordinate. | ||
| This sequence is ordered in [=priority|reverse paint order=] of its {{HighlightHitResult}}'s [=custom highlights|highlights=]. |
There was a problem hiding this comment.
I'm not sure whether I like the change to show the priority link as reverse paint order. In https://drafts.csswg.org/css-highlight-api-1/#priorities we basically define the paint order as being derived from the concept of https://drafts.csswg.org/css-highlight-api-1/#priority, so I think it's more direct just to keep the link as having that same word. That way there's no question of which concept is derived from which.
I don't feel super strongly about this though, so if you disagree feel free to disregard this comment.
There was a problem hiding this comment.
I agree with you, I'm not strongly opposed to using paint order either but I think it's a bit clearer to use the original concept for it.
| as <var>h2</var> has higher priority than <var>h1</var>. | ||
| * <code>[ HighlightHitResult {highlight: <var>h1</var>, ranges: [<var>r1</var>]} ]</code>, if the user clicks on character "a". | ||
| Note that only <var>r1</var> is included in the {{HighlightHitResult}} returned since that's the only range in <var>h1</var> that was hit. | ||
| * <code>[ HighlightHitResult {highlight: <var>h2</var>, ranges: [<var ignore=''>r2</var>]}, HighlightHitResult {highlight: <var>h1</var>, ranges: [<var>r1</var>]} ]</code>, |
There was a problem hiding this comment.
What does the ignore='' here do?
There was a problem hiding this comment.
This is to supress a warning from the preprocessor because this variable is used only once, but I wanted to keep the <var> tag on it so it gets the same style as other variables in the spec.
css-highlight-api-1/Overview.bs
Outdated
| * The topmost [=box=] in the [=viewport=] in paint order that would be a target for hit testing at coordinates <var>x</var>,<var>y</var> when applying | ||
| the [=transforms=] that apply to the descendants of the viewport, has an element associated to it that's in a [=shadow tree=] whose | ||
| [=shadow root=] is not [=list/contains|contained by=] <var>options</var>.<var>shadowRoots</var>. | ||
| 1. Otherwise, return a [=sequence=] of {{HighlightHitResult}} objects ordered in [=priority|reverse paint order=] of their <var>highlight</var> attributes, |
There was a problem hiding this comment.
I think this semi-declarative way of defining this is getting somewhat unclear with the added layer of including the ranges. For example it's no longer obvious why a HighlightHitResult for a given Highlight would be included or not, because now the question of whether to include something or not is focused on the ranges. The point that a HighlightHitResult is only included if the resulting ranges array is empty could be lost, it's just kind of implied that ranges is non-empty.
The bit about allowing a Range object to be constructed from a StaticRange and then using the range in the next bullet point is also hard to follow, it's like a bit of imperative logic mixed in with the declarative.
I think it's worth trying a more fully imperative way of defining this, a bit closer to how it'd actually be implemented. Basically:
- Let
resultbe an empty list. - For each Highlight in our registry:
- Create a HighlightHitResult with the given highlight.
- For each Range in the Highlight:
- Do the hit test, and add range to HighlightHitResult if it passes.
- For each Range in the Highlight:
- If the HighlightHitResult has non-empty
ranges, add it toresult.
- Create a HighlightHitResult with the given highlight.
- Sort
resultby priority of the highlights // or alternatively, the outerforloop could iterate the highlights in prority order - Return result.
There was a problem hiding this comment.
That makes sense, I was using declarative because I thought that was the norm but imperative makes more sense in this context. Let me know your thoughts on this last commit.
| [=shadow root=] is not [=list/contains|contained by=] <var>options</var>.<var>shadowRoots</var>. | ||
| 1. Otherwise, let <var>results</var> be an empty [=sequence=]. | ||
| 1. For each {{Highlight}} <var>highlight</var> in this {{HighlightRegistry}}: | ||
| 1. Create a {{HighlightHitResult}} <var>result</var> with the given <var>highlight</var> as <var>result</var>.{{HighlightHitResult/highlight}} attribute. |
There was a problem hiding this comment.
| 1. Create a {{HighlightHitResult}} <var>result</var> with the given <var>highlight</var> as <var>result</var>.{{HighlightHitResult/highlight}} attribute. | |
| 1. Let <var>result</var> be a new {{HighlightHitResult}} with {{HighlightHitResult/highlight}} set to <var>highlight</var>. |
I think this is a bit closer to the usual spec language.
| 1. For each {{Highlight}} <var>highlight</var> in this {{HighlightRegistry}}: | ||
| 1. Create a {{HighlightHitResult}} <var>result</var> with the given <var>highlight</var> as <var>result</var>.{{HighlightHitResult/highlight}} attribute. | ||
| 1. For each {{AbstractRange}} <var>abstractRange</var> in <var>highlight</var>: | ||
| 1. If <var>abstractRange</var> is a live {{Range}} or a [=StaticRange/valid=] {{StaticRange}}, construct a {{Range}} object <var>range</var> |
There was a problem hiding this comment.
| 1. If <var>abstractRange</var> is a live {{Range}} or a [=StaticRange/valid=] {{StaticRange}}, construct a {{Range}} object <var>range</var> | |
| 1. If <var>abstractRange</var> is a live {{Range}} or a [=StaticRange/valid=] {{StaticRange}}, let <var>range</var> be a new {{Range}} |
| {{highlightsFromPoint()}} are too. Hit testing will | ||
| hopefully be defined in a future revision of CSS or HTML. | ||
|
|
||
| 1. If <var>result</var>.{{HighlightHitResult/ranges}} is not empty, sort it in the same way as [=custom highlights=] [=set entries=] and then add <var>result</var> to <var>results</var>. |
There was a problem hiding this comment.
Since we are iterating over the ranges in that same order, I don't think we need to sort them here.
| {{highlightsFromPoint()}} are too. Hit testing will | ||
| hopefully be defined in a future revision of CSS or HTML. | ||
|
|
||
| 1. If <var>result</var>.{{HighlightHitResult/ranges}} is not empty, sort it in the same way as [=custom highlights=] [=set entries=] and then add <var>result</var> to <var>results</var>. |
There was a problem hiding this comment.
I think append is usually used for adding to a sequence instead of add, e.g. looking at examples in https://drafts.csswg.org/cssom-view/
| 1. For each {{Highlight}} <var>highlight</var> in this {{HighlightRegistry}}: | ||
| 1. Create a {{HighlightHitResult}} <var>result</var> with the given <var>highlight</var> as <var>result</var>.{{HighlightHitResult/highlight}} attribute. | ||
| 1. For each {{AbstractRange}} <var>abstractRange</var> in <var>highlight</var>: | ||
| 1. If <var>abstractRange</var> is a live {{Range}} or a [=StaticRange/valid=] {{StaticRange}}, construct a {{Range}} object <var>range</var> |
There was a problem hiding this comment.
It's not really clear what the algorithm is supposed to do if abstractRang is not a live range or a valid staticrange. I think we need to continue if it's not, or add another layer of nesting after this if.
There was a problem hiding this comment.
It might also be more understandable to check the condition that abstractRange is an invalid staticrange.
| 1. If <var>abstractRange</var> is a live {{Range}} or a [=StaticRange/valid=] {{StaticRange}}, construct a {{Range}} object <var>range</var> | ||
| whose [=start node=] and [=end node=] are set to <var>abstractRange</var>'s [=start node=] and [=end node=] respectively, | ||
| and [=start offset=] and [=end offset=] are set to <var>abstractRange</var>'s [=start offset=] and [=end offset=] respectively. | ||
| 1. If <var>range</var> could be successfully constructed and the coordinates <var>x</var>,<var>y</var> fall inside at least one of the {{DOMRect}}s |
There was a problem hiding this comment.
I think checking after the fact whether the range was "successfully constructed" is a little hand-wavy. We should probably be explicitly checking for thrown exceptions. There are a couple examples of that to look at in the DOM spec; in https://dom.spec.whatwg.org/ search for "catching any exceptions", or "If this throws an exception".
There was a problem hiding this comment.
Also, when would we actually expect this to fail/throw?
[css-highlight-api] Change highlightsFromPoint return type #12031
As discussed and resolved in #12031, this PR changes the return type for
highlightsFromPointtosequence<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.