Make DomUtils.getVisibleClientRects default to expected behaviour

This requires passing of an extra truthy argument in order to access the
(generally) unexpected behaviour of sometimes returning the rects of
child elements. All locations in the code that *actually* wanted this
behaviour have been updated to continue using it.

Also add a comment about the unexpected behaviour in the function
description.

4 files changed, 25 insertions, 21 deletions

diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index 51e5df35..3fa6f20f 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -197,7 +197,7 @@ LinkHints =
       isClickable = onlyHasTabIndex = true
 
     if isClickable
-      clientRect = DomUtils.getVisibleClientRect element
+      clientRect = DomUtils.getVisibleClientRect element, true
       if clientRect != null
         visibleElements.push {element: element, rect: clientRect, secondClassCitizen: onlyHasTabIndex}
 
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index e7aea163..967e1e1e 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -395,7 +395,7 @@ extend window,
       visibleInputs =
         for i in [0...resultSet.snapshotLength] by 1
           element = resultSet.snapshotItem i
-          rect = DomUtils.getVisibleClientRect element
+          rect = DomUtils.getVisibleClientRect element, true
           continue if rect == null
           { element: element, rect: rect }
 
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index e0ab09e2..30b9f68c 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -55,21 +55,25 @@ DomUtils =
   #
   # Returns the first visible clientRect of an element if it exists. Otherwise it returns null.
   #
-  getVisibleClientRect: (element) ->
+  # WARNING: If testChildren = true then the rects of visible (eg. floated) children may be returned instead.
+  # This is used for LinkHints and focusInput, **BUT IS UNSUITABLE FOR MOST OTHER PURPOSES**.
+  #
+  getVisibleClientRect: (element, testChildren = false) ->
     # Note: this call will be expensive if we modify the DOM in between calls.
     clientRects = (Rect.copy clientRect for clientRect in element.getClientRects())
 
     for clientRect in clientRects
-      # If the link has zero dimensions, it may be wrapping visible
-      # but floated elements. Check for this.
-      if (clientRect.width == 0 || clientRect.height == 0)
+      # If the link has zero dimensions, it may be wrapping visible but floated elements. Check for this.
+      if (clientRect.width == 0 or clientRect.height == 0) and testChildren
         for child in element.children
           computedStyle = window.getComputedStyle(child, null)
           # Ignore child elements which are not floated and not absolutely positioned for parent elements with
           # zero width/height
+          # NOTE(mrmr1993): This ignores floated/absolutely positioned descendants nested within inline
+          # children.
           continue if (computedStyle.getPropertyValue('float') == 'none' &&
             computedStyle.getPropertyValue('position') != 'absolute')
-          childClientRect = @getVisibleClientRect(child)
+          childClientRect = @getVisibleClientRect child, true
           continue if childClientRect == null or childClientRect.width < 3 or childClientRect.height < 3
           return childClientRect
 
diff --git a/tests/dom_tests/dom_utils_test.coffee b/tests/dom_tests/dom_utils_test.coffee
index ad8bde3c..e98dc958 100644
--- a/tests/dom_tests/dom_utils_test.coffee
+++ b/tests/dom_tests/dom_utils_test.coffee
@@ -4,19 +4,19 @@ context "Check visibility",
     document.getElementById("test-div").innerHTML = """
     <div id='foo'>test</div>
     """
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'foo') != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true) != null
 
   should "detect display:none links as hidden", ->
     document.getElementById("test-div").innerHTML = """
     <a id='foo' style='display:none'>test</a>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
 
   should "detect visibility:hidden links as hidden", ->
     document.getElementById("test-div").innerHTML = """
     <a id='foo' style='visibility:hidden'>test</a>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
 
   should "detect elements nested in display:none elements as hidden", ->
     document.getElementById("test-div").innerHTML = """
@@ -24,7 +24,7 @@ context "Check visibility",
       <a id='foo'>test</a>
     </div>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
 
   should "detect links nested in visibility:hidden elements as hidden", ->
     document.getElementById("test-div").innerHTML = """
@@ -32,23 +32,23 @@ context "Check visibility",
       <a id='foo'>test</a>
     </div>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
 
   should "detect links outside viewport as hidden", ->
     document.getElementById("test-div").innerHTML = """
     <a id='foo' style='position:absolute;top:-2000px'>test</a>
     <a id='bar' style='position:absolute;left:2000px'>test</a>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'bar'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'bar'), true)
 
   should "detect links only partially outside viewport as visible", ->
     document.getElementById("test-div").innerHTML = """
     <a id='foo' style='position:absolute;top:-10px'>test</a>
     <a id='bar' style='position:absolute;left:-10px'>test</a>
     """
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'foo') != null
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'bar') != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true) != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'bar'), true) != null
 
   should "detect links that contain only floated / absolutely-positioned divs as visible", ->
     document.getElementById("test-div").innerHTML = """
@@ -56,14 +56,14 @@ context "Check visibility",
       <div style='float:left'>test</div>
     </a>
     """
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'foo') != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true) != null
 
     document.getElementById("test-div").innerHTML = """
     <a id='foo'>
       <div style='position:absolute;top:0;left:0'>test</div>
     </a>
     """
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'foo') != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true) != null
 
   should "detect links that contain only invisible floated divs as invisible", ->
     document.getElementById("test-div").innerHTML = """
@@ -71,7 +71,7 @@ context "Check visibility",
       <div style='float:left;visibility:hidden'>test</div>
     </a>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)
 
   should "detect links inside opacity:0 elements as visible", ->
     # XXX This is an expected failure. See issue #16.
@@ -80,7 +80,7 @@ context "Check visibility",
       <a id='foo'>test</a>
     </div>
     """
-    assert.isTrue (DomUtils.getVisibleClientRect document.getElementById 'foo') != null
+    assert.isTrue (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true) != null
 
   should "Detect links within SVGs as visible", ->
     # XXX this is an expected failure
@@ -91,4 +91,4 @@ context "Check visibility",
       </a>
     </svg>
     """
-    assert.equal null, DomUtils.getVisibleClientRect document.getElementById 'foo'
+    assert.equal null, (DomUtils.getVisibleClientRect (document.getElementById 'foo'), true)