venantius / accountant

ClojureScript navigation for single-page applications, made simple.
Eclipse Public License 1.0
250 stars 39 forks source link

Accountant doesn't block page reloads for image links on IE11 #49

Closed eerohele closed 6 years ago

eerohele commented 6 years ago

Given:

<a href="/"> <!-- Given that "/" is a known route -->
  <img src="logo.png"/>
</a>

If you click the image on IE11 (at least, possibly other IE versions, too), Accountant doesn't block the page reload.

This is because find-href-node uses the href DOM property, and on IE11, for some reason, document.querySelector("img").href returns the value of the src attribute of the img element. logo.png isn't a known route, which leads to the browser attempting to reload the page.

One possible fix would be to use .getAttribute instead. Something like this, for example:

diff --git a/src/accountant/core.cljs b/src/accountant/core.cljs
index fcbe6e0..42a8137 100644
--- a/src/accountant/core.cljs
+++ b/src/accountant/core.cljs
@@ -18,11 +18,18 @@
       (let [token (.-token e)]
         (nav-handler token)))))

+(defn- get-href
+  "Given a DOM node, if it is an element node, return its href attribute.
+  Otherwise, return nil."
+  [node]
+  (when (and node (= (.-nodeType node) js/Node.ELEMENT_NODE))
+    (.getAttribute node "href")))
+
 (defn- find-href-node
   "Given a DOM element that may or may not be a link, traverse up the DOM tree
   to see if any of its parents are links. If so, return the node."
   [e]
-  (if (.-href e)
+  (if (get-href e)
     e
     (when-let [parent (.-parentNode e)]
       (recur parent))))
venantius commented 6 years ago

Ugh, browsers, amirite?

I'd be open to accepting a PR with the approach you've outlined above.

eerohele commented 6 years ago

Cool — I'll run some more tests to try and make sure the change doesn't break anything and then submit a PR.

venantius commented 6 years ago

Thanks!