Merge branch 'lahwaacz-hints_positioning'

CHANGELOG.asciidoc

@@ -63,7 +63,8 @@ Fixed
 
 - Fixed using `:hint links spawn` with flags - you can now use things like the
   `-v` argument for `:spawn` or pass flags to the spawned commands.
-- Various fixes for hinting corner-cases where following a link didn't work
+- Various fixes for hinting corner-cases where following a link didn't work or
+  the hint was drawn at the wrong position.
 - Fixed crash when downloading from an URL with SSL errors
 - Close file handles correctly when a download failed
 - Fixed crash when using `;Y` (`:hint links yank-primary`) on a system without

README.asciidoc

@@ -152,10 +152,10 @@ Contributors, sorted by the number of commits in descending order:
 * Joel Torstensson
 * Tarcisio Fedrizzi
 * Patric Schmitz
+* Jakub Klinkovský
 * Claude
 * Corentin Julé
 * meles5
-* Jakub Klinkovský
 * Philipp Hansch
 * Panagiotis Ktistakis
 * Artur Shaik

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
@@ -350,7 +350,7 @@ class HintManager(QObject):
             ('display', 'inline !important'),
             ('z-index', '{} !important'.format(int(2 ** 32 / 2 - 1))),
             ('pointer-events', 'none !important'),
-            ('position', 'absolute !important'),
+            ('position', 'fixed !important'),
             ('color', config.get('colors', 'hints.fg') + ' !important'),
             ('background', config.get('colors', 'hints.bg') + ' !important'),
             ('font', config.get('fonts', 'hints') + ' !important'),
@@ -376,15 +376,11 @@ class HintManager(QObject):
             elem: The QWebElement to set the style attributes for.
             label: The label QWebElement.
         """
-        rect = elem.geometry()
+        rect = elem.rect_on_view(adjust_zoom=False)
         left = rect.x()
         top = rect.y()
-        zoom = elem.webFrame().zoomFactor()
+        log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}'"
-        if not config.get('ui', 'zoom-text-only'):
+                         .format(label, left, top, elem))
-            left /= zoom
-            top /= zoom
-        log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}', "
-                         "zoom level {}".format(label, left, top, elem, zoom))
         label.setStyleProperty('left', '{}px !important'.format(left))
         label.setStyleProperty('top', '{}px !important'.format(top))
 
@@ -421,46 +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):
-        """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.
-        """
-        rects = elem.evaluateJavaScript("this.getClientRects()")
-        log.hints.debug("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'):
-                    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
-        return elem.rect_on_view()
-
     def _click(self, elem, context):
         """Click an element.
 
@@ -481,11 +437,15 @@ class HintManager(QObject):
         else:
             target_mapping[Target.tab] = usertypes.ClickTarget.tab
 
-        # FIXME Instead of clicking the center, we could have nicer heuristics.
+        # Click the center of the largest square fitting into the top/left
-        # e.g. parse (-webkit-)border-radius correctly and click text fields at
+        # corner of the rectangle, this will help if part of the <a> element
-        # the bottom right, and everything else on the top left or so.
+        # is hidden behind other elements
-        # https://github.com/The-Compiler/qutebrowser/issues/70
+        # 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:
+            rect.setHeight(rect.width())
         pos = rect.center()
 
         action = "Hovering" if context.target == Target.hover else "Clicking"

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/end2end/features/hints.feature

@@ -113,8 +113,6 @@ Feature: Using hints
     @xfail_norun
     Scenario: Using :follow-hint inside an iframe
         When I open data/hints/iframe.html
-        And I run :hint all normal
-        And I run :follow-hint a
         And I run :hint links normal
         And I run :follow-hint a
         Then "acceptNavigationRequest, url http://localhost:*/data/hello.txt, type NavigationTypeLinkClicked, *" should be logged

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/manual/hints/issue824.html

@@ -8,5 +8,6 @@
         <p>When using hints (f) on this page, the hint should be drawn over the
           link.</p>
         <p>See <a href="https://github.com/The-Compiler/qutebrowser/issues/824">#824</a>.</p>
+        <p>This was fixed by <a href="https://github.com/The-Compiler/qutebrowser/pull/1433">#1433</a>.</p>
     </body>
 </html>

tests/manual/hints/other.html

@@ -13,19 +13,19 @@
           </li>
           <li>
             Links should be correctly positioned on <a href="http://www.xkcd.org/">xkcd.org</a> - see <a href="https://github.com/The-Compiler/qutebrowser/issues/824">#824</a>.<br/>
-            Current state: <span style="color: red">bad</span>
+            Current state: <span style="color: green">good</span> - fixed in <a href="https://github.com/the-compiler/qutebrowser/pull/1433">#1433</a>
           </li>
           <li>
-            Links should be correctly positioned on this <a href="http://git.exherbo.org/summer/">exherbo.org page</a> - see <a href="https://github.com/The-Compiler/qutebrowser/pull/1003">#1003</a>.<br/>
+            links should be correctly positioned on this <a href="http://git.exherbo.org/summer/">exherbo.org page</a> - see <a href="https://github.com/the-compiler/qutebrowser/pull/1003">#1003</a>.<br/>
-            Current state: <span style="color: red;">bad</span>
+            current state: <span style="color: red;">bad</span>
           </li>
           <li>
-            Links should be correctly positioned on this <a href="https://www.ctl.io/developers/blog/post/optimizing-docker-images/">ctl.io page</a> - see <a href="https://github.com/The-Compiler/qutebrowser/issues/824#issuecomment-208859886">#824 (comment)</a>.<br/>
+            links should be correctly positioned on this <a href="https://www.ctl.io/developers/blog/post/optimizing-docker-images/">ctl.io page</a> - see <a href="https://github.com/The-Compiler/qutebrowser/issues/824#issuecomment-208859886">#824 (comment)</a>.<br/>
-            Current state: <span style="color: red;">bad</span>
+            Current state: <span style="color: green;">good</span> - fixed in <a href="https://github.com/the-compiler/qutebrowser/pull/1433">#1433</a>
           </li>
           <li>
             When clicking titles under the images on <a href="https://www.etsy.com/search?q=test">etsy</a>, the correct item should be selected (sometimes the one on the right is selected instead) - see <a href="https://github.com/The-Compiler/qutebrowser/issues/1005">#1005</a>.<br/>
-            Current state: <span style="color: red;">bad</span>
+            Current state: <span style="color: green;">good</span> - fixed in <a href="https://github.com/the-compiler/qutebrowser/pull/1433">#1433</a>
           </li>
         </ul>
     </body>

tests/manual/hints/zoom.html

@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <title>Drawing hints with zoom</title>
+    </head>
+    <body>
+      <p>
+        When you press 2+ then f on this page, the hint
+        should be drawn at the correct position.
+        <a href="https://www.qutebrowser.org/">link</a>.
+      </p>
+    </body>
+</html>

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