Skip to content
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

Bug in Postbox's Range.compareBoundaryPoints #179

Closed
adam-p opened this issue May 3, 2014 · 1 comment
Closed

Bug in Postbox's Range.compareBoundaryPoints #179

adam-p opened this issue May 3, 2014 · 1 comment

Comments

@adam-p
Copy link
Owner

adam-p commented May 3, 2014

While writing new code I have encountered a bug in Postbox's implementation of Range.compareBoundaryPoints. This is exacerbated by the fact that it uses an old version of the Gecko JavaScript engine -- so old that Range.intersectsNode isn't supported.

I have exchanged some email with a Postbox dev(?), and I'm going to capture them here for reference.


From me

I’m not sure if you got the links from my original email, but here’s the Range.intersectsNode fallback code in a StackOverflow answer I was referring to: https://stackoverflow.com/questions/1482832/how-to-get-all-elements-that-are-highlighted/1483487#1483487

The reproduction scenario is like:

  1. Compose an email (of one word, even). Select the entire contents.
  2. Call rangeIntersectsNode(document.defaultView.getSelection().getRangeAt(0), document.body)
  3. It will return false.

(You can use my extension for this and stick the code in, say, markdown-here.js:markdownHere(). But be aware the Markdown Here code that it’s in that Github branch you mentioned doesn’t work with Postbox. I have some uncommitted changes here — like removing use of TreeWalker. Postbox uses older Gecko, etc.)

If you do the same repro scenario in Thunderbird (altering the rangeIntersectsNode code to force the fallback) it will return true.

It’s returning false in Postbox because both of these calls are returning 1:

range.compareBoundaryPoints(
                node.ownerDocument.defaultView.Range.END_TO_START,
                nodeRange);

range.compareBoundaryPoints(
                node.ownerDocument.defaultView.Range.START_TO_END,
                nodeRange);

…Where nodeRange is the the selected body. But that doesn’t make sense. Both of those returning 1 means that: “the start of the selection-range is after the end of the body-range and the end of the selection-range is after the start of the body-range”. (Well, the latter part of that statement is correct, but the former isn’t. And, sure enough, Thunderbird returns -1, 1.)


From Postbox

Can you try this:

rangeIntersectsNode(GetCurrentEditor().document.defaultView.getSelection().getRangeAt(0), GetCurrentEditor().document.body)

In Postbox, that returns true for me because:

range.compareBoundaryPoints(Range.END_TO_START, nodeRange) == -1
and
range.compareBoundaryPoints(Range.START_TO_END, nodeRange) == 1

I was calling that from chrome.

In your email you wrote:

rangeIntersectsNode(document.defaultView.getSelection().getRangeAt(0), document.body)

I wonder if that was the XUL document and XUL document body, and that is why it failed?


From me

At first I still couldn’t replicate your results, but then I moved code around and went back to the original rangeIntersectsNode. And… maybe I’ve figured out the problem? (Or a problem.)

TL;DR: In Postbox, node.ownerDocument.defaultView.Range.END_TO_START and node.ownerDocument.defaultView.Range.START_TO_END are undefined. However, Range.END_TO_START and Range.START_TO_END are fine.

Output from some logs that I’ll provide the code for below…

Postbox:

[object ChromeWindow]
[object Window]
END_TO_START: undefined, 3
START_TO_END: undefined, 1
rangeIntersectsNode1: false
rangeIntersectsNode2: true

Earlybird:

[object ChromeWindow]
[object XrayWrapper [object Window]]
END_TO_START: 3, 3
START_TO_END: 1, 1
rangeIntersectsNode1: true
rangeIntersectsNode2: true

Here’s the diff:

diff --git a/src/firefox/chrome/content/ff-overlay.js b/src/firefox/chrome/content/ff-overlay.js
index 0de0d5c..b94b1b3 100644
--- a/src/firefox/chrome/content/ff-overlay.js
+++ b/src/firefox/chrome/content/ff-overlay.js
@@ -29,6 +29,60 @@ var markdown_here = {
   onMenuItemCommand: function(e) {
     var mdReturn, focusedElem, self = this;

+    function rangeIntersectsNode1(range, node) {
+        var nodeRange;
+        if (range.intersectsNode) {
+          return range.intersectsNode(node);
+        }
+        else {
+          nodeRange = node.ownerDocument.createRange();
+          try {
+            nodeRange.selectNode(node);
+          }
+          catch (e) {
+            nodeRange.selectNodeContents(node);
+          }
+
+          return range.compareBoundaryPoints(
+                    node.ownerDocument.defaultView.Range.END_TO_START,
+                    nodeRange) === -1 &&
+                 range.compareBoundaryPoints(
+                    node.ownerDocument.defaultView.Range.START_TO_END,
+                    nodeRange) === 1;
+        }
+    }
+
+    function rangeIntersectsNode2(range, node) {
+        var nodeRange;
+        if (range.intersectsNode) {
+          return range.intersectsNode(node);
+        }
+        else {
+          nodeRange = node.ownerDocument.createRange();
+          try {
+            nodeRange.selectNode(node);
+          }
+          catch (e) {
+            nodeRange.selectNodeContents(node);
+          }
+
+          return range.compareBoundaryPoints(
+                    Range.END_TO_START,
+                    nodeRange) === -1 &&
+                 range.compareBoundaryPoints(
+                    Range.START_TO_END,
+                    nodeRange) === 1;
+        }
+    }
+
+    markdown_here.log(window);
+    markdown_here.log(window.GetCurrentEditor().document.defaultView);
+    markdown_here.log('END_TO_START: ' + GetCurrentEditor().document.body.ownerDocument.defaultView.Range.END_TO_START + ', ' + Range.END_TO_START);
+    markdown_here.log('START_TO_END: ' + GetCurrentEditor().document.body.ownerDocument.defaultView.Range.START_TO_END + ', ' + Range.START_TO_END);
+    markdown_here.log('rangeIntersectsNode1: ' + rangeIntersectsNode1(GetCurrentEditor().document.defaultView.getSelection().getRangeAt(0), GetCurrentEditor().document.body));
+    markdown_here.log('rangeIntersectsNode2: ' + rangeIntersectsNode2(GetCurrentEditor().document.defaultView.getSelection().getRangeAt(0), GetCurrentEditor().document.body));
+    return;
+
     focusedElem = markdown_here.imports.markdownHere.findFocusedElem(window.document);
     if (!focusedElem) {
       // Shouldn't happen. But if it does, just silently abort.

So… that seems weird? Like, node.ownerDocument.defaultView.Range exists but node.ownerDocument.defaultView.Range.START_TO_END doesn’t…? I can maybe work around this, but it’s going to be very hackish.

Am I missing something?


From Postbox

I think this is https://bugzilla.mozilla.org/show_bug.cgi?id=665279

We don't have that fix in Postbox yet.

See mozilla/addon-sdk@3ef0d9b for a way that bug # 665279 was worked around before it was fixed.

But that might be more hackish that the hack you have in mind.

Let me know what you come up with.

ps: At first I thought it was because the object wasn't a XPCNativeWrapper (XrayWrapper), but I don't think that's it:

[object XrayWrapper [object Window]]

vs

[object Window]

But in case you're curious:

https://developer.mozilla.org/en-US/docs/XPCNativeWrapper
https://developer.mozilla.org/en-US/docs/Safely_accessing_content_DOM_from_chrome

@adam-p
Copy link
Owner Author

adam-p commented May 10, 2014

After corresponding with Postbox, we figured out the problem. Updated the original issue comment.

@adam-p adam-p closed this as completed May 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant