Re: [whatwg/dom] Add definition for composed live range (PR #1342)

@domfarolino commented on this pull request.



> @@ -7911,10 +7919,10 @@ range.setEnd(secondText, 4)
 <a for=/>nodes</a>.
 
 <p>{{Range}} objects, unlike {{StaticRange}} objects, are affected by mutations to the
-<a>node tree</a>. Therefore they are also known as <a>live ranges</a>. Such mutations will not
-invalidate them and will try to ensure that it still represents the same piece of content.
-Necessarily, a <a>live range</a> might itself be modified as part of the mutation to the
-<a>node tree</a> when, e.g., part of the content it represents is mutated.
+<a>node tree</a>. Therefore they are <a>live ranges</a>. Such mutations will not invalidate them and
+will try to ensure that it still represents the same piece of content. Necessarily, a
+<a>live range</a> might itself be modified as part of the mutation to the <a>node tree</a> when,
+e.g., part of the content it represents is mutated.

Why do we make any changes to this paragraph? Were they requested from somewhere I missed?

> @@ -8141,12 +8149,21 @@ interface Range : AbstractRange {
 };
 </pre>
 
-<p>Objects implementing the {{Range}} interface are known as
-<dfn export id=concept-live-range>live ranges</dfn>.
+<p>A <dfn export id=concept-live-range>live range</dfn> is a <a>range</a> that is affected by
+mutations to the <a>node tree</a>.</p>
+
+<p>Objects implementing the {{Range}} interface are <a>live ranges</a>.

Before this PR, we have the following layout of definitions:

 - "range" == `AbstractRange` IDL object
 - "live range" == `Range` IDL object

After this PR, we have:

 - "range" == `AbstractRange` IDL object (no changes)
 - "live range" == any "range" (`AbstractRange`) responsive to mutations
    - "live range" includes `Range` IDL objects
    - "live range" includes "composed live range" `AbstractRange` IDL

So what we've done is make "live range" now include `AbstractRange` objects. I think you've done this to avoid making "composed live range" a full-blown `Range` IDL object, since it doesn't need to be one—as we discussed, implementations appropriately use some internal non-IDL object to represent a "composed live range". But even with your changes I think "composed live range" is still an IDL object—it's just an `AbstractRange` instead of a `Range`.

So I think the definition rejiggering in this PR is kinda half way in between two solutions. We can either:

 1. Stick to our guns that "composed live range" should NOT be an IDL object, and do what Fetch does where we have IDL objects like [`Request`](https://fetch.spec.whatwg.org/#request) and also "internal" spec concepts like [#concept-request](https://fetch.spec.whatwg.org/#concept-request). In our case we'd have `AbstractRange` and then an internal `#concept-range`-like struct. "composed live range" would be-a `#concept-range`, and not an IDL object. It would likely hold a "cached" `Range` inside.
 2. Decide we're OK with "composed live range" being an IDL object in the spec, even though it never is one in implementation, nor is it ever web-exposed. In that case, we can either keep it as an `AbstractRange` IDL object which it is in this PR. OR we can simplify this PR a bit and leave the definition of "live range" alone—then we'd make "composed live range" a `Range` IDL object that just so happens to never be exposed to script directly. That feels weird since there isn't a reason for "composed live range" to be an IDL object besides it would make this PR smaller/easier.

The details of these options are discussed in https://github.com/w3c/selection-api/issues/2#issuecomment-2599729387 and https://github.com/w3c/selection-api/issues/2#issuecomment-2428009143, and I'd love @annevk's thoughts.

>       to <var>bp</var>.
 
+     <li>Otherwise, if <var>bp</var> is

This doesn't need to be tucked behind an "Otherwise", right? Like it doesn't need to be exclusive from the first condition above I think, right? Plus I think it's confusing to have "Otherwise [....]. If" on the same line, when I think they can just be separate conditions. So what do you think about the following:

1. (same as what you have)
2. If bp is after the range's end, then:
    1. Set range's end to bp
    2. If _range_ is the cached live range of a composed live range _composed live range_, then set _composed live range_'s end to bp
3. (same as what you have)

>       <li>Set <var>range</var>'s
      <a for=range>start</a> to <var>bp</var>.
+     If <var>range</var> is the
+     <a for="composed live range">cached live range</a> of a
+     <a>composed live range</a> <var>composed live range</var>,

We can pull the _composed live range_ variable out early on. How about we detect it and set this variable right after step 2 or step 3, before we branch off into the "set the start" or "set the end" step 4 branches? That should be less redundant.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/pull/1342#pullrequestreview-2578889482
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/dom/pull/1342/review/2578889482@github.com>