From 75264c37c674210ed5f9c4338db6fca0964c5113 Mon Sep 17 00:00:00 2001 From: Kristof Csillag Date: Sat, 13 Jul 2013 02:18:46 +0200 Subject: [PATCH 1/3] Tighten/fix up range normalization This is next attempt at fixing this long-standing issue. Some background: When we are normalizing a browser range, we must find a text node and offset to point to, even if the browser range is pointing to some other (nearby) node element. (p, div, br, img, whatever). To do this, we must do some DOM crawling. Earlier we were trying to take some shortcuts, but that failed on some crazy page structures. Now I added full recursive crawling ability to locate the wanted text nodes. This fixes all the failing Range test cases, and also fixes #235. --- src/range.coffee | 73 ++++++++++++++++++++++++++++++++------------------------ src/util.coffee | 38 +++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 31 deletions(-) diff --git a/src/range.coffee b/src/range.coffee index c0d44b37..07ee64de 100644 --- a/src/range.coffee +++ b/src/range.coffee @@ -133,42 +133,53 @@ class Range.BrowserRange @tainted = true r = {} - nr = {} - - for p in ['start', 'end'] - node = this[p + 'Container'] - offset = this[p + 'Offset'] - - if node.nodeType is Node.ELEMENT_NODE - # Get specified node. - it = node.childNodes[offset] - # If it doesn't exist, that means we need the end of the - # previous one. - node = it or node.childNodes[offset - 1] - - # if node doesn't have any children, it's a
or
or - # other self-closing tag, and we actually want the textNode - # that ends just before it - if node.nodeType is Node.ELEMENT_NODE and not node.firstChild - it = null # null out ref to node so offset is correctly calculated below. - node = node.previousSibling - - while node.nodeType isnt Node.TEXT_NODE - node = node.firstChild - - offset = if it then 0 else node.nodeValue.length - - r[p] = node - r[p + 'Offset'] = offset + # Look at the start + if @startContainer.nodeType is Node.ELEMENT_NODE + # We are dealing with element nodes + r.start = Util.getFirstTextNodeNotBefore @startContainer.childNodes[@startOffset] + r.startOffset = 0 + else + # We are dealing with simple text nodes + r.start = @startContainer + r.startOffset = @startOffset + + # Look at the end + if @endContainer.nodeType is Node.ELEMENT_NODE + # Get specified node. + node = @endContainer.childNodes[@endOffset] + + if node? # Does that node exist? + # Look for a text node either at the immediate beginning of node + n = node + while n? and (n.nodeType isnt Node.TEXT_NODE) + n = n.firstChild + if n? # Did we find a text node at the start of this element? + r.end = n + r.endOffset = 0 + + unless r.end? + # We need to find a text node in the previous node. + node = @endContainer.childNodes[@endOffset - 1] + r.end = Util.getLastTextNodeUpTo node + r.endOffset = r.end.nodeValue.length + + else # We are dealing with simple text nodes + r.end = @endContainer + r.endOffset = @endOffset + + # We have collected the initial data. + + # Now let's start to slice & dice the text elements! + nr = {} nr.start = if r.startOffset > 0 then r.start.splitText(r.startOffset) else r.start - if r.start is r.end - if (r.endOffset - r.startOffset) < nr.start.nodeValue.length + if r.start is r.end # is the whole selection inside one text element ? + if nr.start.nodeValue.length > (r.endOffset - r.startOffset) nr.start.splitText(r.endOffset - r.startOffset) nr.end = nr.start - else - if r.endOffset < r.end.nodeValue.length + else # no, the end of the selection is in a separate text element + if r.end.nodeValue.length > r.endOffset# does the end need to be cut? r.end.splitText(r.endOffset) nr.end = r.end diff --git a/src/util.coffee b/src/util.coffee index 6b4f3580..79258e0d 100644 --- a/src/util.coffee +++ b/src/util.coffee @@ -59,6 +59,44 @@ Util.getTextNodes = (jq) -> jq.map -> Util.flatten(getTextNodes(this)) +# Public: determine the last text node inside or before the given node +Util.getLastTextNodeUpTo = (n) -> + switch n.nodeType + when Node.TEXT_NODE + return n # We have found our text node. + when Node.ELEMENT_NODE + # This is an element, we need to dig in + if n.lastChild? # Does it have children at all? + result = Util.getLastTextNodeUpTo n.lastChild + if result? then return result + else + # Not a text node, and not an element node. + # Could not find a text node in current node, go backwards + n = n.previousSibling + if n? + Util.getLastTextNodeUpTo n + else + null + +# Public: determine the first text node in or after the given jQuery node. +Util.getFirstTextNodeNotBefore = (n) -> + switch n.nodeType + when Node.TEXT_NODE + return n # We have found our text node. + when Node.ELEMENT_NODE + # This is an element, we need to dig in + if n.firstChild? # Does it have children at all? + result = Util.getFirstTextNodeNotBefore n.firstChild + if result? then return result + else + # Not a text or an element node. + # Could not find a text node in current node, go forward + n = n.nextSibling + if n? + Util.getFirstTextNodeNotBefore n + else + null + Util.xpathFromNode = (el, relativeRoot) -> try result = simpleXPathJQuery.call el, relativeRoot From e8decbb3eac5c6351a50d0744caaca2e696f60e2 Mon Sep 17 00:00:00 2001 From: Kristof Csillag Date: Sat, 13 Jul 2013 03:00:59 +0200 Subject: [PATCH 2/3] Work around test failure caused by PhantomJS bug --- src/util.coffee | 10 ++++++++++ test/helpers/spec_helper.coffee | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/util.coffee b/src/util.coffee index 79258e0d..ed264517 100644 --- a/src/util.coffee +++ b/src/util.coffee @@ -33,6 +33,16 @@ Util.flatten = (array) -> flatten(array) +# Public: decides whether node A is an ancestor of node B. +# +# This function purposefully ignores the native browser function for this, because it acts weird in PhantomJS. +Util.contains = (parent, child) -> + node = child + while node? + if node is parent then return true + node = node.parentNode + return false + # Public: Finds all text nodes within the elements in the current collection. # # Returns a new jQuery collection of text nodes. diff --git a/test/helpers/spec_helper.coffee b/test/helpers/spec_helper.coffee index 1f878a58..2ddf6699 100644 --- a/test/helpers/spec_helper.coffee +++ b/test/helpers/spec_helper.coffee @@ -15,7 +15,7 @@ class this.MockSelection @description = data[5] @commonAncestor = @startContainer - while not @commonAncestor.contains @endContainer + while not Util.contains(@commonAncestor, @endContainer) @commonAncestor = @commonAncestor.parentNode @commonAncestorXPath = Util.xpathFromNode($(@commonAncestor))[0] From 9d1af9d397cc6fda3a9820490ed90649abc796fa Mon Sep 17 00:00:00 2001 From: Kristof Csillag Date: Mon, 15 Jul 2013 23:30:45 +0200 Subject: [PATCH 3/3] Added link to PhantomJS issue --- src/util.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util.coffee b/src/util.coffee index ed264517..8b6f4243 100644 --- a/src/util.coffee +++ b/src/util.coffee @@ -35,7 +35,9 @@ Util.flatten = (array) -> # Public: decides whether node A is an ancestor of node B. # -# This function purposefully ignores the native browser function for this, because it acts weird in PhantomJS. +# This function purposefully ignores the native browser function for this, +# because it acts weird in PhantomJS. +# Issue: https://github.com/ariya/phantomjs/issues/11479 Util.contains = (parent, child) -> node = child while node?