Skip to content

Performance fix for long documents in 1.2.x branch #577

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

Merged
rufuspollock merged 2 commits into from
Apr 11, 2026
+27 −28
Merged

Performance fix for long documents in 1.2.x branch #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34e7581
Performance fix for long documents in 1.2.x branch, based off https:/…
mwidner Nov 17, 2015
9a12030
Backporting performance improvement in https://github.com/openannotat…
mwidner Nov 19, 2015
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
10 changes: 7 additions & 3 deletions src/annotator.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,15 +411,19 @@ class Annotator extends Delegator
highlightRange: (normedRange, cssClass='annotator-hl') ->
white = /^\s*$/

hl = $("<span class='#{cssClass}'></span>")

results = []
# Ignore text nodes that contain only whitespace characters. This prevents
# spans being injected between elements that can only contain a restricted
# subset of nodes such as table rows and lists. This does mean that there
# may be the odd abandoned whitespace node in a paragraph that is skipped
# but better than breaking table layouts.
for node in normedRange.textNodes() when not white.test(node.nodeValue)
$(node).wrapAll(hl).parent().show()[0]
hl = document.createElement('span')
hl.className = cssClass
node.parentNode.replaceChild(hl, node)
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

highlightRange now assumes every text node has a parentNode (node.parentNode.replaceChild(...)). This will throw for detached text nodes and breaks existing unit tests that pass unattached document.createTextNode(...) nodes into highlightRange. Handle the parentNode-missing case (e.g., wrap without replaceChild, or skip with a clear behavior) so the method is robust and tests continue to pass.

Suggested change
node.parentNode.replaceChild(hl, node)
if node.parentNode?
node.parentNode.replaceChild(hl, node)

Copilot uses AI. Check for mistakes.
hl.appendChild(node)
results.push(hl)
return results

# Public: highlight a list of ranges
#
Expand Down
19 changes: 14 additions & 5 deletions src/range.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,22 @@ class Range.NormalizedRange
#
# Returns updated self or null.
limit: (bounds) ->
nodes = $.grep this.textNodes(), (node) ->
node.parentNode == bounds or $.contains(bounds, node.parentNode)
if @commonAncestor == bounds or $.contains(bounds, @commonAncestor)
return this

return null unless nodes.length
if not $.contains(@commonAncestor, bounds)
return null
document = bounds.ownerDocument
Comment on lines +258 to +260
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

NormalizedRange#limit can return a non-null range even when the original range does not intersect bounds (e.g., selection outside @wrapper[0] but with commonAncestor that contains bounds). The current logic only checks that bounds is within @commonAncestor, then rewrites @start/@end to the first/last text node in bounds, effectively moving the range into bounds instead of returning null. Add an explicit intersection check (e.g., compare document positions / boundary points) before adjusting endpoints, and add a test covering the disjoint-but-descendant case.

Copilot uses AI. Check for mistakes.

@start = nodes[0]
@end = nodes[nodes.length - 1]
if not $.contains(bounds, @start)
walker = document.createTreeWalker(bounds, NodeFilter.SHOW_TEXT)
@start = walker.firstChild()

if not $.contains(bounds, @end)
walker = document.createTreeWalker(bounds, NodeFilter.SHOW_TEXT)
@end = walker.lastChild()

return null unless @start and @end

startParents = $(@start).parents()
for parent in $(@end).parents()
Expand Down
26 changes: 6 additions & 20 deletions src/util.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,27 +50,13 @@ Util.contains = (parent, child) ->
#
# Returns a new jQuery collection of text nodes.
Util.getTextNodes = (jq) ->
getTextNodes = (node) ->
if node and node.nodeType != Node.TEXT_NODE
nodes = []

# If not a comment then traverse children collecting text nodes.
# We traverse the child nodes manually rather than using the .childNodes
# property because IE9 does not update the .childNodes property after
# .splitText() is called on a child text node.
if node.nodeType != Node.COMMENT_NODE
# Start at the last child and walk backwards through siblings.
node = node.lastChild
while node
nodes.push getTextNodes(node)
node = node.previousSibling

# Finally reverse the array so that nodes are in the correct order.
return nodes.reverse()
else
return node
getTextNodes = (root) ->
document = root.ownerDocument
walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT)
nodes = (node while node = walker.nextNode())
return nodes

jq.map -> Util.flatten(getTextNodes(this))
jq.map -> getTextNodes(this)

# Public: determine the last text node inside or before the given node
Util.getLastTextNodeUpTo = (n) ->
Expand Down