diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d5c9ffaa1..898c94dd8 100644 --- a/CHANGELOG.asciidoc +++ b/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 diff --git a/README.asciidoc b/README.asciidoc index e735d0832..5b99c28e2 100644 --- a/README.asciidoc +++ b/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 diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index 530138c23..7b098fd25 100644 --- a/qutebrowser/browser/hints.py +++ b/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() - if not config.get('ui', 'zoom-text-only'): - left /= zoom - top /= zoom - log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}', " - "zoom level {}".format(label, left, top, elem, zoom)) + log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}'" + .format(label, left, top, elem)) 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 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. - # e.g. parse (-webkit-)border-radius correctly and click text fields at - # the bottom right, and everything else on the top left or so. - # https://github.com/The-Compiler/qutebrowser/issues/70 - rect = self._get_first_rectangle(elem) + # Click the center of the largest square fitting into the top/left + # corner of the rectangle, this will help if part of the element + # is hidden behind other elements + # https://github.com/The-Compiler/qutebrowser/issues/1005 + 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" diff --git a/qutebrowser/browser/webelem.py b/qutebrowser/browser/webelem.py index cc387bc9a..cac214102 100644 --- a/qutebrowser/browser/webelem.py +++ b/qutebrowser/browser/webelem.py @@ -173,9 +173,14 @@ class WebElementWrapper(collections.abc.MutableMapping): """ return is_visible(self._elem, mainframe) - def rect_on_view(self): - """Get the geometry of the element relative to the webview.""" - return rect_on_view(self._elem) + def rect_on_view(self, *, adjust_zoom=True): + """Get the geometry of the element relative to the webview. + + 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 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 diff --git a/tests/end2end/features/hints.feature b/tests/end2end/features/hints.feature index 9d4470ebe..5bccf7ab4 100644 --- a/tests/end2end/features/hints.feature +++ b/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 diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py index adaebd168..91519d558 100644 --- a/tests/helpers/stubs.py +++ b/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': diff --git a/tests/manual/hints/issue824.html b/tests/manual/hints/issue824.html index 510f9758d..881e1b263 100644 --- a/tests/manual/hints/issue824.html +++ b/tests/manual/hints/issue824.html @@ -8,5 +8,6 @@

When using hints (f) on this page, the hint should be drawn over the link.

See #824.

+

This was fixed by #1433.

diff --git a/tests/manual/hints/other.html b/tests/manual/hints/other.html index bbbdb24d4..2ee87a890 100644 --- a/tests/manual/hints/other.html +++ b/tests/manual/hints/other.html @@ -13,19 +13,19 @@
  • Links should be correctly positioned on xkcd.org - see #824.
    - Current state: bad + Current state: good - fixed in #1433
  • - Links should be correctly positioned on this exherbo.org page - see #1003.
    - Current state: bad + links should be correctly positioned on this exherbo.org page - see #1003.
    + current state: bad
  • - Links should be correctly positioned on this ctl.io page - see #824 (comment).
    - Current state: bad + links should be correctly positioned on this ctl.io page - see #824 (comment).
    + Current state: good - fixed in #1433
  • When clicking titles under the images on etsy, the correct item should be selected (sometimes the one on the right is selected instead) - see #1005.
    - Current state: bad + Current state: good - fixed in #1433
  • diff --git a/tests/manual/hints/zoom.html b/tests/manual/hints/zoom.html new file mode 100644 index 000000000..84c816882 --- /dev/null +++ b/tests/manual/hints/zoom.html @@ -0,0 +1,14 @@ + + + + + Drawing hints with zoom + + +

    + When you press 2+ then f on this page, the hint + should be drawn at the correct position. + link. +

    + + diff --git a/tests/unit/browser/test_webelem.py b/tests/unit/browser/test_webelem.py index 8ab0eaa53..10917e4e5 100644 --- a/tests/unit/browser/test_webelem.py +++ b/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