No hints with JS hints #1568

Closed
The-Compiler opened this Issue Jun 9, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@The-Compiler
Collaborator

The-Compiler commented Jun 9, 2016

piou in IRC reported they don't see any hints anymore on a particular machine (32bit VM with Qt 5.6).

Reverting to 7db9b85 (before @lahwaacz changes) fixes it, as does commenting out the JS hint code.

It's reproducible for them with --temp-basedir. Will follow up ASAP with logs.

If clicking using JS works for them, I'm considering adding a Python/JS setting for drawing. If that doesn't either, probably two separate settings. Now not sure what the defaults should be, of course.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

It's not 100% reproducible, and easier with the keybinding (bound to :hint) than with :hint. Log for a failing example: http://paste.the-compiler.org/view/raw/6e80993f

This is on https://en.wikipedia.org/wiki/Systemd#References

I can't see anything weird in that log...

Collaborator

The-Compiler commented Jun 9, 2016

It's not 100% reproducible, and easier with the keybinding (bound to :hint) than with :hint. Log for a failing example: http://paste.the-compiler.org/view/raw/6e80993f

This is on https://en.wikipedia.org/wiki/Systemd#References

I can't see anything weird in that log...

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

Comparing a good and bad log: http://paste.the-compiler.org/view/bf250c80 (first keybinding, doesn't work, then :hint which works, no resizing or scrolling)

Both logs are equal... I don't get this.

Collaborator

The-Compiler commented Jun 9, 2016

Comparing a good and bad log: http://paste.the-compiler.org/view/bf250c80 (first keybinding, doesn't work, then :hint which works, no resizing or scrolling)

Both logs are equal... I don't get this.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

We couldn't find any reason why it only fails sometimes. Things we tried:

  • Replacing return rect for the JS codepath by break so it's executed but not actually used -> works
  • Replacing the JS code path by a time.sleep with same length -> works

Comparing the rects returned we see some subtle differences, and then some weird ones:

js                                       python
--                                       ------
PyQt5.QtCore.QRect(806, 349, 131, 22)    PyQt5.QtCore.QRect(806, 350, 131, 21)
PyQt5.QtCore.QRect(343, 387, 26, 19)     PyQt5.QtCore.QRect(343, 388, 26, 18)
PyQt5.QtCore.QRect(241, 5694, 8, 20)     PyQt5.QtCore.QRect(241, -99854, 8, 105567)
PyQt5.QtCore.QRect(241, 5716, 8, 20)     PyQt5.QtCore.QRect(241, -99854, 8, 105589)

So the python code seems to produce some weird results? Might be some other,
unrelated bug?

Collaborator

The-Compiler commented Jun 9, 2016

We couldn't find any reason why it only fails sometimes. Things we tried:

  • Replacing return rect for the JS codepath by break so it's executed but not actually used -> works
  • Replacing the JS code path by a time.sleep with same length -> works

Comparing the rects returned we see some subtle differences, and then some weird ones:

js                                       python
--                                       ------
PyQt5.QtCore.QRect(806, 349, 131, 22)    PyQt5.QtCore.QRect(806, 350, 131, 21)
PyQt5.QtCore.QRect(343, 387, 26, 19)     PyQt5.QtCore.QRect(343, 388, 26, 18)
PyQt5.QtCore.QRect(241, 5694, 8, 20)     PyQt5.QtCore.QRect(241, -99854, 8, 105567)
PyQt5.QtCore.QRect(241, 5716, 8, 20)     PyQt5.QtCore.QRect(241, -99854, 8, 105589)

So the python code seems to produce some weird results? Might be some other,
unrelated bug?

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

I get the same Python elements - with the piou branch I pushed: http://paste.the-compiler.org/view/09c34d2c

Seems like Python is just wrong with the accessibility links, might be a red herring.

Collaborator

The-Compiler commented Jun 9, 2016

I get the same Python elements - with the piou branch I pushed: http://paste.the-compiler.org/view/09c34d2c

Seems like Python is just wrong with the accessibility links, might be a red herring.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

piou tried using the Python result when they're "weird", and the JS result otherwise, which seems to work.

working: http://paste.devys.org/paygapayjui
broken: http://paste.devys.org/gufeulupau

(line 82 changes)

Collaborator

The-Compiler commented Jun 9, 2016

piou tried using the Python result when they're "weird", and the JS result otherwise, which seems to work.

working: http://paste.devys.org/paygapayjui
broken: http://paste.devys.org/gufeulupau

(line 82 changes)

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

Apparently doing things the other way around works too?!

        left = js_rect.x() if rect.y() < 0 else rect.x()
        top  = js_rect.y() if rect.y() < 0 else rect.y()

For future reference, the master..piou diff (without that change) is:

diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py
index 3bb1723..d206ca9 100644
--- a/qutebrowser/browser/hints.py
+++ b/qutebrowser/browser/hints.py
@@ -381,7 +381,14 @@ class HintManager(QObject):
             elem: The QWebElement to set the style attributes for.
             label: The label QWebElement.
         """
-        rect = elem.rect_on_view(adjust_zoom=False)
+        rect = elem.rect_on_view(adjust_zoom=False, no_js=True)
+        js_rect = elem.rect_on_view(adjust_zoom=False)
+
+        log.misc.debug("-----> Element {!r}".format(elem))
+        log.misc.debug("rect: {!r}".format(rect))
+        log.misc.debug("js_rect: {!r}".format(js_rect))
+        log.misc.debug("")
+
         left = rect.x()
         top = rect.y()
         log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}'"
diff --git a/qutebrowser/browser/webelem.py b/qutebrowser/browser/webelem.py
index e85b0b2..d0679e4 100644
--- a/qutebrowser/browser/webelem.py
+++ b/qutebrowser/browser/webelem.py
@@ -173,14 +173,9 @@ class WebElementWrapper(collections.abc.MutableMapping):
         """
         return is_visible(self._elem, mainframe)

-    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 rect_on_view(self, **kwargs):
+        """Get the geometry of the element relative to the webview."""
+        return rect_on_view(self._elem, **kwargs)

     def is_writable(self):
         """Check whether an element is writable."""
@@ -368,7 +363,7 @@ def focus_elem(frame):
     return WebElementWrapper(elem)


-def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True):
+def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True, no_js=False):
     """Get the geometry of the element relative to the webview.

     We need this as a standalone function (as opposed to a WebElementWrapper
@@ -391,13 +386,14 @@ def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True):
                        want to avoid doing it twice.
         adjust_zoom: Whether to adjust the element position based on the
                      current zoom level.
+        no_js: Fall back to the Python implementation
     """
     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:
+    if elem_geometry is None and not no_js:
         rects = elem.evaluateJavaScript("this.getClientRects()")
         text = utils.compact_text(elem.toOuterXml(), 500)
         log.hints.vdebug("Client rectangles of element '{}': {}".format(text,
Collaborator

The-Compiler commented Jun 9, 2016

Apparently doing things the other way around works too?!

        left = js_rect.x() if rect.y() < 0 else rect.x()
        top  = js_rect.y() if rect.y() < 0 else rect.y()

For future reference, the master..piou diff (without that change) is:

diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py
index 3bb1723..d206ca9 100644
--- a/qutebrowser/browser/hints.py
+++ b/qutebrowser/browser/hints.py
@@ -381,7 +381,14 @@ class HintManager(QObject):
             elem: The QWebElement to set the style attributes for.
             label: The label QWebElement.
         """
-        rect = elem.rect_on_view(adjust_zoom=False)
+        rect = elem.rect_on_view(adjust_zoom=False, no_js=True)
+        js_rect = elem.rect_on_view(adjust_zoom=False)
+
+        log.misc.debug("-----> Element {!r}".format(elem))
+        log.misc.debug("rect: {!r}".format(rect))
+        log.misc.debug("js_rect: {!r}".format(js_rect))
+        log.misc.debug("")
+
         left = rect.x()
         top = rect.y()
         log.hints.vdebug("Drawing label '{!r}' at {}/{} for element '{!r}'"
diff --git a/qutebrowser/browser/webelem.py b/qutebrowser/browser/webelem.py
index e85b0b2..d0679e4 100644
--- a/qutebrowser/browser/webelem.py
+++ b/qutebrowser/browser/webelem.py
@@ -173,14 +173,9 @@ class WebElementWrapper(collections.abc.MutableMapping):
         """
         return is_visible(self._elem, mainframe)

-    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 rect_on_view(self, **kwargs):
+        """Get the geometry of the element relative to the webview."""
+        return rect_on_view(self._elem, **kwargs)

     def is_writable(self):
         """Check whether an element is writable."""
@@ -368,7 +363,7 @@ def focus_elem(frame):
     return WebElementWrapper(elem)


-def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True):
+def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True, no_js=False):
     """Get the geometry of the element relative to the webview.

     We need this as a standalone function (as opposed to a WebElementWrapper
@@ -391,13 +386,14 @@ def rect_on_view(elem, *, elem_geometry=None, adjust_zoom=True):
                        want to avoid doing it twice.
         adjust_zoom: Whether to adjust the element position based on the
                      current zoom level.
+        no_js: Fall back to the Python implementation
     """
     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:
+    if elem_geometry is None and not no_js:
         rects = elem.evaluateJavaScript("this.getClientRects()")
         text = utils.compact_text(elem.toOuterXml(), 500)
         log.hints.vdebug("Client rectangles of element '{}': {}".format(text,

The-Compiler added a commit that referenced this issue Jun 9, 2016

Add a hints -> find-implementation setting
This makes it possible to switch to an alternative implementation if
there are weird issues like #1568. Some users might also prefer the
slightly better performance over more accurate hints.
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 9, 2016

Collaborator

So after >3.5h of painful debugging we decided to drop this for now, and I added a setting (hints -> find-implementation) which allows to go back to the Python implementation.

This might also make sense for other people who prefer the slightly better performance over accuracy.

Collaborator

The-Compiler commented Jun 9, 2016

So after >3.5h of painful debugging we decided to drop this for now, and I added a setting (hints -> find-implementation) which allows to go back to the Python implementation.

This might also make sense for other people who prefer the slightly better performance over accuracy.

@The-Compiler The-Compiler removed this from the v0.7.0 milestone Jun 10, 2016

@lahwaacz

This comment has been minimized.

Show comment
Hide comment
@lahwaacz

lahwaacz Jun 11, 2016

Collaborator

I'm also unable to reproduce. The hint labels should show up as <span class="qutehint"> elements in the web inspector (near the end of the HTML). Do they show up for piou or are they missing as well? Is it only a problem with drawing the labels or clicking as well?

Collaborator

lahwaacz commented Jun 11, 2016

I'm also unable to reproduce. The hint labels should show up as <span class="qutehint"> elements in the web inspector (near the end of the HTML). Do they show up for piou or are they missing as well? Is it only a problem with drawing the labels or clicking as well?

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Sep 8, 2016

Collaborator

Closing this as I don't see how we could do anything here... I've also not heard about anyone else having the same issue so far.

Collaborator

The-Compiler commented Sep 8, 2016

Closing this as I don't see how we could do anything here... I've also not heard about anyone else having the same issue so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment