hints: Integrate _get_first_rectangle into webelem

qutebrowser/browser/hints.py

@@ -26,7 +26,7 @@ import re
 import string
 
 from PyQt5.QtCore import (pyqtSignal, pyqtSlot, QObject, QEvent, Qt, QUrl,
-                          QTimer, QRect)
+                          QTimer)
 from PyQt5.QtGui import QMouseEvent
 from PyQt5.QtWebKit import QWebElement
 from PyQt5.QtWebKitWidgets import QWebPage
@@ -376,7 +376,7 @@ class HintManager(QObject):
             elem: The QWebElement to set the style attributes for.
             label: The label QWebElement.
         """
-        rect = self._get_first_rectangle(elem, adjust_zoom=False)
+        rect = elem.rect_on_view(adjust_zoom=False)
         left = rect.x()
         top = rect.y()
         log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}'"
@@ -417,55 +417,6 @@ class HintManager(QObject):
         message.error(self._win_id, "No suitable link found for this element.",
                       immediately=True)
 
-    def _get_first_rectangle(self, elem, *, adjust_zoom=True):
-        """Return the element's first client rectangle with positive size.
-
-        Uses the getClientRects() JavaScript method to obtain the collection of
-        rectangles containing the element and returns the first rectangle which
-        is large enough (larger than 1px times 1px). If all rectangles returned
-        by getClientRects() are too small, falls back to elem.rect_on_view().
-
-        Skipping of small rectangles is due to <a> elements containing other
-        elements with "display:block" style, see
-        https://github.com/The-Compiler/qutebrowser/issues/1298
-
-        Args:
-            elem: The QWebElement of interest.
-            adjust_zoom: Whether to adjust the element position based on the
-                         current zoom level.
-        """
-        rects = elem.evaluateJavaScript("this.getClientRects()")
-        log.hints.vdebug("Client rectangles of element '{}': {}"
-                .format(elem.debug_text(), rects))
-        for i in range(int(rects.get("length", 0))):
-            rect = rects[str(i)]
-            width = rect.get("width", 0)
-            height = rect.get("height", 0)
-            if width > 1 and height > 1:
-                # fix coordinates according to zoom level
-                zoom = elem.webFrame().zoomFactor()
-                if not config.get('ui', 'zoom-text-only') and adjust_zoom:
-                    rect["left"] *= zoom
-                    rect["top"] *= zoom
-                    width *= zoom
-                    height *= zoom
-                rect = QRect(rect["left"], rect["top"], width, height)
-                frame = elem.webFrame()
-                while frame is not None:
-                    # Translate to parent frames' position
-                    # (scroll position is taken care of inside getClientRects)
-                    rect.translate(frame.geometry().topLeft())
-                    frame = frame.parentFrame()
-                return rect
-
-        # No suitable rects found via JS, try via the QWebElement API
-        rect = elem.rect_on_view()
-        zoom = elem.webFrame().zoomFactor()
-        if not config.get('ui', 'zoom-text-only'):
-            rect.setLeft(rect.left() / zoom)
-            rect.setTop(rect.top() / zoom)
-        return rect
-
     def _click(self, elem, context):
         """Click an element.
 
@@ -490,7 +441,7 @@ class HintManager(QObject):
         # corner of the rectangle, this will help if part of the <a> element
         # is hidden behind other elements
         # https://github.com/The-Compiler/qutebrowser/issues/1005
-        rect = self._get_first_rectangle(elem)
+        rect = elem.rect_on_view()
         if rect.width() > rect.height():
             rect.setWidth(rect.height())
         else:

qutebrowser/browser/webelem.py

@@ -173,9 +173,14 @@ class WebElementWrapper(collections.abc.MutableMapping):
         """
         return is_visible(self._elem, mainframe)
 
-    def rect_on_view(self):
+    def rect_on_view(self, *, adjust_zoom=True):
-        """Get the geometry of the element relative to the webview."""
+        """Get the geometry of the element relative to the webview.
-        return rect_on_view(self._elem)
+
+        Args:
+            adjust_zoom: Whether to adjust the element position based on the
+                         current zoom level.
+        """
+        return rect_on_view(self._elem, adjust_zoom=adjust_zoom)
 
     def is_writable(self):
         """Check whether an element is writable."""
@@ -363,21 +368,62 @@ def focus_elem(frame):
     return WebElementWrapper(elem)
 
 
-def rect_on_view(elem, elem_geometry=None):
+def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True):
     """Get the geometry of the element relative to the webview.
 
     We need this as a standalone function (as opposed to a WebElementWrapper
     method) because we want to run is_visible before wrapping when hinting for
     performance reasons.
 
+    Uses the getClientRects() JavaScript method to obtain the collection of
+    rectangles containing the element and returns the first rectangle which is
+    large enough (larger than 1px times 1px). If all rectangles returned by
+    getClientRects() are too small, falls back to elem.rect_on_view().
+
+    Skipping of small rectangles is due to <a> elements containing other
+    elements with "display:block" style, see
+    https://github.com/The-Compiler/qutebrowser/issues/1298
+
     Args:
         elem: The QWebElement to get the rect for.
         elem_geometry: The geometry of the element, or None.
                        Calling QWebElement::geometry is rather expensive so we
                        want to avoid doing it twice.
+        adjust_zoom: Whether to adjust the element position based on the
+                     current zoom level.
     """
     if elem.isNull():
         raise IsNullError("Got called on a null element!")
+
+    # First try getting the element rect via JS, as that's usually more
+    # accurate
+    if elem_geometry is None:
+        rects = elem.evaluateJavaScript("this.getClientRects()")
+        text = utils.compact_text(elem.toOuterXml(), 500)
+        log.hints.vdebug("Client rectangles of element '{}': {}".format(text,
+                                                                        rects))
+        for i in range(int(rects.get("length", 0))):
+            rect = rects[str(i)]
+            width = rect.get("width", 0)
+            height = rect.get("height", 0)
+            if width > 1 and height > 1:
+                # fix coordinates according to zoom level
+                zoom = elem.webFrame().zoomFactor()
+                if not config.get('ui', 'zoom-text-only') and adjust_zoom:
+                    rect["left"] *= zoom
+                    rect["top"] *= zoom
+                    width *= zoom
+                    height *= zoom
+                rect = QRect(rect["left"], rect["top"], width, height)
+                frame = elem.webFrame()
+                while frame is not None:
+                    # Translate to parent frames' position
+                    # (scroll position is taken care of inside getClientRects)
+                    rect.translate(frame.geometry().topLeft())
+                    frame = frame.parentFrame()
+                return rect
+
+    # No suitable rects found via JS, try via the QWebElement API
     if elem_geometry is None:
         elem_geometry = elem.geometry()
     frame = elem.webFrame()
@@ -386,6 +432,11 @@ def rect_on_view(elem, elem_geometry=None):
         rect.translate(frame.geometry().topLeft())
         rect.translate(frame.scrollPosition() * -1)
         frame = frame.parentFrame()
+    # We deliberately always adjust the zoom here, even with adjust_zoom=False
+    zoom = elem.webFrame().zoomFactor()
+    if not config.get('ui', 'zoom-text-only'):
+        rect.setLeft(rect.left() / zoom)
+        rect.setTop(rect.top() / zoom)
     return rect
 
 

tests/helpers/stubs.py

@@ -77,7 +77,7 @@ class FakeWebFrame:
     """
 
     def __init__(self, geometry=None, *, scroll=None, plaintext=None,
-                 html=None, parent=None):
+                 html=None, parent=None, zoom=1.0):
         """Constructor.
 
         Args:
@@ -85,6 +85,7 @@ class FakeWebFrame:
             scroll: The scroll position as QPoint.
             plaintext: Return value of toPlainText
             html: Return value of tohtml.
+            zoom: The zoom factor.
             parent: The parent frame.
         """
         if scroll is None:
@@ -95,6 +96,7 @@ class FakeWebFrame:
         self.focus_elem = None
         self.toPlainText = mock.Mock(return_value=plaintext)
         self.toHtml = mock.Mock(return_value=html)
+        self.zoomFactor = mock.Mock(return_value=zoom)
 
     def findFirstElement(self, selector):
         if selector == '*:focus':

tests/unit/browser/test_webelem.py

@@ -49,6 +49,7 @@ def get_webelem(geometry=None, frame=None, null=False, style=None,
         tagname: The tag name.
         classes: HTML classes to be added.
     """
+    # pylint: disable=too-many-locals
     elem = mock.Mock()
     elem.isNull.return_value = null
     elem.geometry.return_value = geometry
@@ -58,6 +59,25 @@ def get_webelem(geometry=None, frame=None, null=False, style=None,
     elem.toPlainText.return_value = 'text'
     elem.parent.return_value = parent
 
+    if geometry is not None:
+        if frame is None:
+            scroll_x = 0
+            scroll_y = 0
+        else:
+            scroll_x = frame.scrollPosition().x()
+            scroll_y = frame.scrollPosition().y()
+        elem.evaluateJavaScript.return_value = {
+            "length": 1,
+            "0": {
+                "left": geometry.left() - scroll_x,
+                "top": geometry.top() - scroll_y,
+                "right": geometry.right() - scroll_x,
+                "bottom": geometry.bottom() - scroll_y,
+                "width": geometry.width(),
+                "height": geometry.height(),
+            }
+        }
+
     attribute_dict = {}
     if attributes is None:
         pass
@@ -94,6 +114,17 @@ def get_webelem(geometry=None, frame=None, null=False, style=None,
     return wrapped
 
 
+@pytest.fixture(autouse=True)
+def stubbed_config(config_stub, monkeypatch):
+    """Add a zoom-text-only fake config value.
+
+    This is needed for all the tests calling rect_on_view or is_visible.
+    """
+    config_stub.data = {'ui': {'zoom-text-only': 'true'}}
+    monkeypatch.setattr('qutebrowser.browser.webelem.config', config_stub)
+    return config_stub
+
+
 class SelectionAndFilterTests:
 
     """Generator for tests for TestSelectionsAndFilters."""
@@ -618,9 +649,10 @@ class TestRectOnView:
 
     def test_passed_geometry(self, stubs):
         """Make sure geometry isn't called when a geometry is passed."""
-        raw_elem = get_webelem()._elem
+        frame = stubs.FakeWebFrame(QRect(0, 0, 200, 200))
+        raw_elem = get_webelem(frame=frame)._elem
         rect = QRect(10, 20, 30, 40)
-        assert webelem.rect_on_view(raw_elem, rect) == rect
+        assert webelem.rect_on_view(raw_elem, elem_geometry=rect) == rect
         assert not raw_elem.geometry.called