Range normalization fix #238
Conversation
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.
| @@ -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. | |||
There was a problem hiding this comment.
"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.
Here is the issue for PhantomJS: ariya/phantomjs#11479
There was a problem hiding this comment.
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.
|
Thanks for this. Could you please add a test to the test suite that exhibits the behaviour you're trying to fix? I have no idea what "weird" page structure or what kind of selection bugs you're talking about, and thus there's every chance someone refactoring |
|
I have already added the test cases, see this commit: d08f698 . 6 of the 16 new test cases are failing. The proposed fix fixes them. (I have have shamelessly pushed those to the master branch, to highlight the problem.) |
| @@ -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.
I might change these to "getPreviousTextNode" and "getNextTextNode". Maybe clearer?
There was a problem hiding this comment.
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.)
|
Looks good overall. |
This should fix selection bugs seen on pages with weird structure.
(Thus closing #235, and taking care of the failed test cases, too.)