Skip to content

Range performance fixes #9

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

Merged
merged 5 commits into from
May 3, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 15 additions & 5 deletions src/range.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -189,13 +189,23 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

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

I would make this === for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not written CoffeeScript before yesterday, but the docs suggest that == in CoffeeScript compiles to === in JS.

Copy link
Member

@tilgovi tilgovi May 1, 2015 via email

Choose a reason for hiding this comment

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

return this

return null unless nodes.length
if not $.contains(@commonAncestor, bounds)
return null

@start = nodes[0]
@end = nodes[nodes.length - 1]
document = bounds.ownerDocument

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
40 changes: 6 additions & 34 deletions src/util.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -60,27 +60,13 @@ Util.getLastTextNodeUpTo = (n) ->
#
# Returns a new jQuery collection of text nodes.
Util.getTextNodes = (jq) ->
getTextNodes = (node) ->
if node and node.nodeType != Util.NodeTypes.TEXT_NODE
nodes = []
getTextNodes = (root) ->
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how this compares to:

walker = getGlobal().document.createTreeWalker(root, NodeFilter.SHOW_TEXT)
nodes = (node while node = walker.nextNode())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will write a little benchmark and investigate.

document = root.ownerDocument
walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT)
nodes = (node while node = walker.nextNode())
return 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 != Util.NodeTypes.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

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

Util.getGlobal = -> (-> this)()

Expand All @@ -96,20 +82,6 @@ Util.contains = (parent, child) ->
node = node.parentNode
return false

# Public: Flatten a nested array structure
#
# Returns an array
Util.flatten = (array) ->
flatten = (ary) ->
flat = []

for el in ary
flat = flat.concat(if el and $.isArray(el) then flatten(el) else el)

return flat

flatten(array)


# Export Util object
module.exports = Util
52 changes: 49 additions & 3 deletions test/spec/range_spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -155,25 +155,47 @@ describe 'Range', ->
headText = null
paraText = null
paraText2 = null
paraText3 = null
para2Text = null
para = null
root = null

beforeEach ->
headText = document.createTextNode("My Heading")
paraText = document.createTextNode("My paragraph")
paraText2 = document.createTextNode(" continues")
paraText2 = document.createTextNode(" conti")
paraText3 = document.createTextNode("nues")
para2Text = document.createTextNode("Another paragraph begins")

head = document.createElement('h1')
head.appendChild(headText)
para = document.createElement('p')
para.appendChild(paraText)
para.appendChild(paraText2)
span = document.createElement('span')
span.appendChild(paraText2)
span.appendChild(paraText3)
para.appendChild(span)
para2 = document.createElement('p')
para2.appendChild(para2Text)

root = document.createElement('div')
root.appendChild(head)
root.appendChild(para)
root.appendChild(para2)

it "should exclude any nodes not within the bounding element.", ->
it "should be a no-op if all nodes are within the bounding element.", ->
range = new Range.NormalizedRange({
commonAncestor: para
start: paraText
end: paraText2
})

range = range.limit(para)
assert.equal(range.commonAncestor, para)
assert.equal(range.start, paraText)
assert.equal(range.end, paraText2)

it "should exclude any nodes to the left of the bounding element.", ->
range = new Range.NormalizedRange({
commonAncestor: root
start: headText
Expand All @@ -185,6 +207,30 @@ describe 'Range', ->
assert.equal(range.start, paraText)
assert.equal(range.end, paraText2)

it "should exclude any nodes to the right of the bounding element.", ->
range = new Range.NormalizedRange({
commonAncestor: root
start: paraText
end: para2Text
})

range = range.limit(para)
assert.equal(range.commonAncestor, para)
assert.equal(range.start, paraText)
assert.equal(range.end, paraText3)

it "should exclude any nodes on either side of the bounding element.", ->
range = new Range.NormalizedRange({
commonAncestor: root
start: headText
end: para2Text
})

range = range.limit(para)
assert.equal(range.commonAncestor, para)
assert.equal(range.start, paraText)
assert.equal(range.end, paraText3)

it "should return null if no nodes fall within the bounds", ->
otherDiv = document.createElement('div')
range = new Range.NormalizedRange({
Expand Down
5 changes: 0 additions & 5 deletions test/spec/util_spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,6 @@ h = require('helpers')
$ = require('jquery')
Util = require('../../src/util')

describe 'Util.flatten()', ->
it "flattens the contents of an Array", ->
flattened = Util.flatten([[1,2], 'lorem ipsum', [{}]])
assert.deepEqual(flattened, [1, 2, 'lorem ipsum', {}])

describe 'Util.getTextNodes()', ->
$fix = null

Expand Down