-
Notifications
You must be signed in to change notification settings - Fork 538
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
Range normalization fix #238
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -59,6 +69,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) -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might change these to "getPreviousTextNode" and "getNextTextNode". Maybe clearer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the proposed new names would be clearer, since "previous" sounds like we can not accept the current node, while in reality, we we can. So it would be a little bit misleading. (The same goes with next, too.) |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ class this.MockSelection | |
@description = data[5] | ||
|
||
@commonAncestor = @startContainer | ||
while not @commonAncestor.contains @endContainer | ||
while not Util.contains(@commonAncestor, @endContainer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can temporarily revert this one-liner change to see errors caused by the problematic .contains implementation by PhantomJS. |
||
@commonAncestor = @commonAncestor.parentNode | ||
@commonAncestorXPath = Util.xpathFromNode($(@commonAncestor))[0] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Weird" how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Weird" as in bogus; on some occasions, it returns "false", even when the node does contain the other.
Should I create a test case, and file a bug upstream? Yeah, I guess I should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the issue for PhantomJS: ariya/phantomjs#11479
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a link to the QT5 Phantom issue in the comments so that someone who finds this later can see that it can be removed if Phantom is updated.