Permalink
Browse files

Merge branch 'lahwaacz-hints_positioning'

  • Loading branch information...
The-Compiler committed Jun 6, 2016
2 parents 7db9b85 + 45da93a commit b1914d641437c80e2cc17ae8fd44edba58b5851c
View
@@ -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
View
@@ -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
@@ -26,7 +26,7 @@
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 @@ def _set_style_properties(self, elem, label):
('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 @@ def _set_style_position(self, elem, label):
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 @@ def _show_url_error(self):
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 @@ def _click(self, elem, context):
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 <a> 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"
@@ -173,9 +173,14 @@ def is_visible(self, mainframe):
"""
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 <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
@@ -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
View
@@ -77,14 +77,15 @@ class FakeWebFrame:
"""
def __init__(self, geometry=None, *, scroll=None, plaintext=None,
- html=None, parent=None):
+ html=None, parent=None, zoom=1.0):
"""Constructor.
Args:
geometry: The geometry of the frame as QRect.
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 @@ def __init__(self, geometry=None, *, scroll=None, plaintext=None,
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':
@@ -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>
@@ -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/>
- Current state: <span style="color: red;">bad</span>
+ 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>
</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/>
- Current state: <span style="color: red;">bad</span>
+ 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: 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>
@@ -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>
@@ -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 _style_property(name, strategy):
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 @@ def test_iframe(self, stubs):
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

0 comments on commit b1914d6

Please sign in to comment.