Handle visibility of elements on screen correctly

qutebrowser/browser/hints.py

@@ -244,7 +244,10 @@ class HintManager(QObject):
         else:
             target = self._target
         self.set_open_target.emit(Target[target])
-        pos = webelem.pos_on_screen(elem)
+        # 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.
+        pos = webelem.rect_on_screen(elem).center()
         logging.debug("Clicking on \"{}\" at {}/{}".format(
             elem.toPlainText(), pos.x(), pos.y()))
         events = [
@@ -346,12 +349,12 @@ class HintManager(QObject):
             return
         self.openurl.emit(link, newtab)
 
-    def start(self, frame, baseurl, group=webelem.Group.all,
+    def start(self, mainframe, baseurl, group=webelem.Group.all,
               target=Target.normal):
         """Start hinting.
 
         Args:
-            frame: The QWebFrame to place hints in.
+            mainframe: The main QWebFrame.
             baseurl: URL of the current page.
             group: Which group of elements to hint.
             target: What to do with the link. See attribute docstring.
@@ -361,19 +364,19 @@ class HintManager(QObject):
         """
         self._target = target
         self._baseurl = baseurl
-        if frame is None:
+        if mainframe is None:
             # This should never happen since we check frame before calling
             # start. But since we had a bug where frame is None in
             # on_mode_left, we are extra careful here.
             raise ValueError("start() was called with frame=None")
-        self._frames = webelem.get_child_frames(frame)
+        self._frames = webelem.get_child_frames(mainframe)
         elems = []
         for f in self._frames:
             elems += f.findAllElements(webelem.SELECTORS[group])
         filterfunc = webelem.FILTERS.get(group, lambda e: True)
         visible_elems = []
         for e in elems:
-            if filterfunc(e) and webelem.is_visible(e):
+            if filterfunc(e) and webelem.is_visible(e, mainframe):
                 visible_elems.append(e)
         if not visible_elems:
             message.error("No elements found.")

qutebrowser/utils/webelem.py

@@ -58,56 +58,47 @@ FILTERS = {
 }
 
 
-def is_visible(elem):
-    """Check whether the element is currently visible in its frame.
+def is_visible(elem, mainframe):
+    """Check whether the element is currently visible on the screen.
 
     Args:
         elem: The QWebElement to check.
+        mainframe: The main QWebFrame.
 
     Return:
         True if the element is visible, False otherwise.
     """
     if elem.isNull():
         raise ValueError("Element is a null-element!")
-    ## We're starting in the innermost (element) frame and check if every frame
-    ## is visible in its parent.
-    base = elem.webFrame()
-    parent = base.parentFrame()
-    while parent is not None:
-        parentgeom = parent.geometry()
-        parentgeom.translate(parent.scrollPosition())
-        if not parentgeom.intersects(base.geometry()):
-            return False
-        base = parent
-        parent = parent.parentFrame()
-    rect = elem.geometry()
-    ## Now check if the element is visible in the frame.
-    if (not rect.isValid()) and rect.x() == 0:
+    if (not elem.geometry().isValid()) and elem.geometry().x() == 0:
         # Most likely an invisible link
         return False
-    frame = elem.webFrame()
-    framegeom = frame.geometry()
-    framegeom.moveTo(0, 0)
-    framegeom.translate(frame.scrollPosition())
-    if framegeom.intersects(rect):
-        return True
-    return False
+    # First check if the element is visible on screen
+    elem_rect = rect_on_screen(elem)
+    visible_on_screen = mainframe.geometry().intersects(elem_rect)
+    # Then check if it's visible in its frame if it's not in the main frame.
+    elem_frame = elem.webFrame()
+    if elem_frame.parentFrame() is not None:
+        framegeom = elem_frame.geometry()
+        framegeom.moveTo(0, 0)
+        framegeom.translate(elem_frame.scrollPosition())
+        visible_in_frame = framegeom.intersects(elem.geometry())
+    else:
+        visible_in_frame = visible_on_screen
+    return all([visible_on_screen, visible_in_frame])
 
 
-def pos_on_screen(elem):
-    """Get the position of the element on the screen."""
-    # 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.
+def rect_on_screen(elem):
+    """Get the geometry of the element relative to the screen."""
     frame = elem.webFrame()
-    pos = elem.geometry().center()
+    rect = elem.geometry()
     while frame is not None:
-        pos += frame.geometry().topLeft()
-        logging.debug("After adding frame pos: {}".format(pos))
-        pos -= frame.scrollPosition()
-        logging.debug("After removing frame scrollpos: {}".format(pos))
+        rect.translate(frame.geometry().topLeft())
+        logging.debug("After adding frame pos: {}".format(rect))
+        rect.translate(frame.scrollPosition() * -1)
+        logging.debug("After removing frame scrollpos: {}".format(rect))
         frame = frame.parentFrame()
-    return pos
+    return rect
 
 
 def javascript_escape(text):