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.

Sign up for GitHub

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

Change Selection to be associated with a composed live range #345

Draft
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Draft

Change Selection to be associated with a composed live range #345

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 64 additions & 61 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ <h2>
[=editing hosts=] in the <a>document</a>.
</p>
<p>
Each <a>selection</a> can be associated with a single <a>range</a>.
Each <a>selection</a> can be associated with a single
<a href="TODO">composed live range</a> <dfn>range</dfn>.
When there is no <a>range</a> associated with the <a>selection</a>, the
selection is <dfn>empty</dfn>. The selection must be initially
<a>empty</a>.
Expand All @@ -127,6 +128,20 @@ <h2>
dev do not. We follow Gecko/WebKit, because it lessens the chance of
getRangeAt(0) throwing.
</p>
<p>To <dfn>reset the range</dfn> of [=this=], given the
<a>boundary point</a> <var>start</var> and <a>boundary point</a>
<var>end</var>, run these steps:
</p>
<ol>
<li>Let <var>newCachedRange</var> be a new {{Range}} object.</li>
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, the {{Range}} should refer to the Range interface defined in [dom]. However, it keeps pointing to range, which I redefined in this PR. How can I overwrite this behavior?

Copy link
Member

Choose a reason for hiding this comment

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

I think I would just remove the dfn of range that you introduced here. We already know the type of "composed live range", once it's able to link to DOM.

<li>[=Range/Set the start=] of <var>newCachedRange</var> to <var>start</var>.
</li>
<li>[=Range/Set the end=] of <var>newCachedRange</var> to <var>end</var>.</li>
<li>Let <var>newRange</var> be a new <a href="TODO">composed live range</a>
Copy link
Member

Choose a reason for hiding this comment

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

We should add a note in this step to indicate that while this looks identical to the above steps, newCachedRange and composed selection range could actually end up in very different states, because when we update newCachedRange we go through the "set the start|end" algorithms which collapse in the cross-root case, while for composed selection range we just manually set the members without any algorithm collapsing or anything.

whose [=Range/start=] is <var>start</var>, [=Range/end=] is <var>end</var>
and <var>cached live range</var> is <var>newCachedRange</var>.</li>
<li>Set [=this=]'s <a>range</a> to be <var>newRange</var>.</li>
</ol>
<p>
Once a <a>selection</a> is associated with a given <a>range</a>, it
must continue to be associated with that same <a>range</a> until this
Expand Down Expand Up @@ -167,13 +182,14 @@ <h2>
<a>selection</a> must be preserved.
</p>
<p>
Each <a>selection</a>s also have an <dfn>anchor</dfn> and a
Each <a>selection</a> also has an <dfn>anchor</dfn> and a
<dfn>focus</dfn>. If the <a>selection</a>'s <a>range</a> is null, its
<a>anchor</a> and <a>focus</a> are both null. If the <a>selection</a>'s
<a>range</a> is not null and its <a>direction</a> is <a>forwards</a>,
its <a>anchor</a> is the <a>range</a>'s [=range/start=], and its
<a>focus</a> is the [=range/end=]. Otherwise, its <a>focus</a> is the
[=range/start=] and its <a>anchor</a> is the [=range/end=].
its <a>anchor</a> is the <a>range</a>'s cached live range's
[=range/start=], and its <a>focus</a> is the [=range/end=]. Otherwise,
its <a>focus</a> is the [=range/start=] and its <a>anchor</a> is the
[=range/end=].
</p>
<p class="note">
<a>anchor</a> and <a>focus</a> of <a>selection</a> need not to be in
Expand Down Expand Up @@ -320,7 +336,7 @@ <h2>
<var>index</var> is not <code>0</code>, or if [=this=] is
<a>empty</a> or either <a>focus</a> or <a>anchor</a> is not in the
[=document tree=]. Otherwise, it must return a reference to (not a
copy of) [=this=]'s <a>range</a>.
copy of) [=this=]'s <a>range</a>'s cached live range.
</p>
<p class="note">
Thus subsequent calls of this method returns the same <a>range</a>
Expand All @@ -345,8 +361,8 @@ <h2>
<li>If <code>rangeCount</code> is not <code>0</code>, abort these
steps.
</li>
<li>Set [=this=]'s range to <var>range</var> by a strong reference
(not by making a copy).
<li>Set [=this=]'s range's cached live range to <var>range</var> by
a strong reference (not by making a copy).
</li>
</ol>
<p class="note">
Expand Down Expand Up @@ -379,8 +395,8 @@ <h2>
<dd>
<p>
The method must make [=this=] <a>empty</a> by disassociating its
<a>range</a> if [=this=]'s <a>range</a> is <var>range</var>.
Otherwise, it must throw a {{NotFoundError}}.
<a>range</a> if [=this=]'s <a>range</a>'s cached live range is
<var>range</var>. Otherwise, it must throw a {{NotFoundError}}.
</p>
</dd>
<dt>
Expand Down Expand Up @@ -476,13 +492,8 @@ <h2>
[=shadow-including inclusive ancestor=] of <var>node</var>, abort
these steps.
</li>
<li>Otherwise, let <var>newRange</var> be a new <a>range</a>.
</li>
<li>[=Range/Set the start=] the [=range/start=] and the
[=range/end=] of <var>newRange</var> to (<var>node</var>,
<var>offset</var>).
</li>
<li>Set [=this=]'s <a>range</a> to <var>newRange</var>.
<li>Otherwise, <a>reset the range</a> with (<var>node</var>,
<var>offset</var>) and (<var>node</var>, <var>offset</var>).
</li>
</ol>
</dd>
Expand All @@ -500,13 +511,17 @@ <h2>
</dt>
<dd>
<p>
The method must throw {{InvalidStateError}} exception if the
[=this=] is <a>empty</a>. Otherwise, it must create a new
<a>range</a>, [=Range/set the start=] both its [=range/start=] and
[=range/end=] to the [=range/start=] of [=this=]'s <a>range</a>,
and then set [=this=]'s <a>range</a> to the newly-created
<a>range</a>.
The method must follow these steps:
</p>
<ol>
<li>If [=this=] is <a>empty</a>, throw an {{InvalidStateError}}
exception and abort these steps.
</li>
<li>Let <var>bp</var> be the [=Range/start=] of [=this=]'s <a>range</a>.
</li>
<li><a>Reset the range</a> with <var>bp</var> and <var>bp</var>.
</li>
</ol>
<p class="note">
For collapseToStart/End, IE9 mutates the existing range, while
Firefox 9.0a2 and Chrome 15 dev replace it with a new one. The spec
Expand All @@ -518,13 +533,18 @@ <h2>
<dfn>collapseToEnd()</dfn> method
</dt>
<dd>
<p>
The method must throw {{InvalidStateError}} exception if the
[=this=] is <a>empty</a>. Otherwise, it must create a new
<a>range</a>, [=Range/set the start=] both its [=range/start=] and
[=range/end=] to the [=range/end=] of [=this=]'s <a>range</a>, and
then set [=this=]'s <a>range</a> to the newly-created <a>range</a>.
</p>
<p>
The method must follow these steps:
</p>
<ol>
<li>If [=this=] is <a>empty</a>, throw an {{InvalidStateError}}
exception and abort these steps.
</li>
<li>Let <var>bp</var> be the [=Range/end=] of [=this=]'s <a>range</a>.
</li>
<li><a>Reset the range</a> with <var>bp</var> and <var>bp</var>.
</li>
</ol>
</dd>
<dt>
<dfn>extend()</dfn> method
Expand All @@ -546,24 +566,18 @@ <h2>
<var>newFocus</var> be the <a>boundary point</a> (<var>node</var>,
<var>offset</var>).
</li>
<li>Let <var>newRange</var> be a new <a>range</a>.
</li>
<li>If <var>node</var>'s [=tree/root=] is not the same as the
[=this=]'s <a>range</a>'s [=tree/root=], [=Range/set the start=]
<var>newRange</var>'s [=range/start=] and [=range/end=] to
<var>newFocus</var>.
<li>If <var>node</var>'s [=tree/root=] is not the same as
[=this=]'s <a>range</a>'s [=tree/root=], <a>reset the range</a> with
<var>newFocus</var> and <var>newFocus</var>.
</li>
<li>Otherwise, if <var>oldAnchor</var> is [=boundary point/before=]
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about this. It looks like DOM's position computing algorithm that determines "before" and "after" expects that the two input nodes always have the same root, which might not be true, since after all DOM's "set the start or end" algorithm has cross-root handling logic that we add on to in whatwg/dom#1342.

I guess we should either change "set the start or end" to accept cross-root nodes, or catch that case early on here, and skip the before/after comparison checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer option 2, "catch that case early on here, and skip the before/after comparison checks."

We will need to update the setBaseAndExtent and extend() functions to check that the boundary points are within the same root before we call "before/after" to "set the start/end". I can think of two easy way to achieve that:

  1. If anchor and focus has the same root and anchor is before focus, set range's start to anchor and range's end to focus.
  2. Otherwise, set range's start to focus and range's end to anchor.

OR

  1. If anchor and focus has different roots:
    4.1 If anchor is in a shadow tree, set anchor to its shadow host.
    4.2 If focus is in a shadow tree, set focus to its shadow host.
  2. If anchor is before focus, set range's start to anchor and range's end to focus.
  3. Otherwise, set range's start to focus and range's end to anchor.

or equal to <var>newFocus</var>, [=Range/set the start=]
<var>newRange</var>'s [=range/start=] to <var>oldAnchor</var>, then
set its [=range/end=] to <var>newFocus</var>.
</li>
<li>Otherwise, [=Range/set the start=] <var>newRange</var>'s
[=range/start=] to <var>newFocus</var>, then set its [=range/end=]
to <var>oldAnchor</var>.
or equal to <var>newFocus</var>, <a>reset the range</a>
with <var>oldAnchor</var> and <var>newFocus</var>.
</li>
<li>Set [=this=]'s <a>range</a> to <var>newRange</var>.
<li>Otherwise, <a>reset the range</a> with <var>newFocus</var>
and <var>oldAnchor</var>.
</li>

<li>If <var>newFocus</var> is [=boundary point/before=]
<var>oldAnchor</var>, set [=this=]'s <a>direction</a> to
<a>backwards</a>. Otherwise, set it to <a>forwards</a>.
Expand Down Expand Up @@ -604,15 +618,10 @@ <h2>
<var>focus</var> be the <a>boundary point</a>
(<var>focusNode</var>, <var>focusOffset</var>).
</li>
<li>Let <var>newRange</var> be a new <a>range</a>.
</li>
<li>If <var>anchor</var> is [=boundary point/before=]
<var>focus</var>, [=Range/set the start=] the <var>newRange</var>'s
[=range/start=] to <var>anchor</var> and its [=range/end=] to <var>
focus</var>. Otherwise, [=Range/set the start=] them to
<var>focus</var> and <var>anchor</var> respectively.
</li>
<li>Set [=this=]'s <a>range</a> to <var>newRange</var>.
<var>focus</var>, <a>reset the range</a> with <var>anchor</var> and
<var>focus</var>. Otherwise, <a>reset the range</a> with
<var>focus</var> and <var>anchor</var>.
</li>
<li>If <var>focus</var> is [=boundary point/before=]
<var>anchor</var>, set [=this=]'s <a>direction</a> to
Expand All @@ -634,17 +643,11 @@ <h2>
<li>If <var>node</var>'s [=tree/root=] is not the <a>document</a>
associated with [=this=], abort these steps.
</li>
<li>Let <var>newRange</var> be a new <a>range</a> and
<var>childCount</var> be the number of [=tree/children=] of
<li>Let <var>childCount</var> be the number of [=tree/children=] of
<var>node</var>.
</li>
<li>Set <var>newRange</var>'s [=range/start=] to (<var>node</var>,
<code>0</code>).
</li>
<li>Set <var>newRange</var>'s [=range/end=] to (<var>node</var>,
<var>childCount</var>).
</li>
<li>Set [=this=]'s <a>range</a> to <var>newRange</var>.
<li><a>Reset the range</a> with (<var>node</var>, <code>0</code>)
and (<var>node</var>, <var>childCount</var>).
</li>
<li>Set [=this=]'s <a>direction</a> to <var>forwards</var>.
</li>
Expand Down
Loading