Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hints don't show on some pages #178

Open
The-Compiler opened this issue Oct 10, 2014 · 47 comments
Open

hints don't show on some pages #178

The-Compiler opened this issue Oct 10, 2014 · 47 comments
Labels
component: hints Issues related to hinting ("f" key). priority: 2 - low Issues which are currently not very important.

Comments

@The-Compiler
Copy link
Member

The-Compiler commented Oct 10, 2014

2015-08-31-144805-1107x93-scrot

Pages which seem to work in the meantime:

  • mobile.de: this page. - It seems they wrap all <a>-tags in <div> tags and that maybe trips hint searching up.
  • http://codecore.ca/
  • Google searches (the "gle"/next link - the hint at the top left of that is for page 10)
@The-Compiler The-Compiler self-assigned this Oct 10, 2014
@The-Compiler The-Compiler added this to the v0.1 milestone Oct 10, 2014
@The-Compiler The-Compiler removed this from the v0.1 milestone Oct 27, 2014
@The-Compiler The-Compiler changed the title hints broken on mobile.de search hints don't show on some pages Oct 29, 2014
@The-Compiler The-Compiler added the component: hints Issues related to hinting ("f" key). label Oct 30, 2014
@B0073D
Copy link

B0073D commented Jun 25, 2015

'Mark as Read' button in IRCCloud. This button appears once you have been away from the channel for a while and there has been a fair amount of activity.

@The-Compiler The-Compiler added the priority: 2 - low Issues which are currently not very important. label Oct 1, 2015
@The-Compiler The-Compiler removed their assignment Oct 1, 2015
@NoSuck
Copy link

NoSuck commented Jan 12, 2016

Add to this list: Gmail links to messages (the subject line) in HTML mode.

@error800
Copy link
Contributor

error800 commented Feb 9, 2016

Test case for the buildbot.net "fork me on github" banner.
The hint is actually there, but moved out of the visible screen by CSS:

<a style="position: relative; left: -1em;" href="http://qutebrowser.org">
  link link link
</a>

@error800
Copy link
Contributor

error800 commented Feb 9, 2016

The relayrides.com dropdown does not work because there is no link at all, there is just a <span> that gets a click event attached by JS.

@NoSuck
Copy link

NoSuck commented Feb 28, 2016

Is issue #292 a duplicate of this one?

Another for the ongoing list: The Arch Mirror Status page has sortable headings (Mirror URL, Protocol, Country...) that do not get link hints.

@The-Compiler
Copy link
Member Author

I don't think so - this is about no hints showing up at all, #292 is about hints being there but nothing happens when activating them.

On that page, that's because it's simply a th/div element with a javascript click handler, which is currently not detected as clickable.

@rcorre
Copy link
Contributor

rcorre commented Sep 16, 2016

'Member Login' on https://www.nearlyfreespeech.net/

@The-Compiler
Copy link
Member Author

@rcorre Hmm, that's an interesting one - it actually does get a hint, just a misplaced one. In word hinting mode:

word

Also, it seems the "label" when inspecting it via Chromium is off too:

inspect

@The-Compiler
Copy link
Member Author

Another one: <details> elements, e.g. the arrows on this page. Interestingly, adding them to the CSS selector makes it possible to open them, but closing doesn't work.

@The-Compiler
Copy link
Member Author

I've been talking with @nemanjan00 about this a bit, and we found two possible solutions to catch elements with a JS click handler:

Checking for the cursor

Get the computed style for all elements, and see where the cursor has been overridden via CSS:

:jseval Array.from(document.querySelectorAll("*")).filter(element => {return window.getComputedStyle(element, null).cursor == "pointer"}).forEach(e => e.style.background = "yellow");`

This might be a bit of a performance problem though (but it seems to runs faster than I thought it would), and doesn't catch all cases (especially not fake input elements).

Checking for addEventListener calls

We should get any element which had addEventListener('click', ...) called on it. We can't easily find this out after the fact, but we can do it similar to Vimium: Inject a script which adds a special attribute to elements in an addEventListener wrapper. However, it remains to be seen whether we can inject that after the DOM is built but before the page's JS runs.

@The-Compiler
Copy link
Member Author

vimb also shows hints for elements with a tabindex attribute. That catches e.g. the fake checkboxes for GMail mails. We might do that as well, but I'd rather wait until we have configurable selectors (#2773) as there are some false-positives as well.

@The-Compiler
Copy link
Member Author

The-Compiler commented Mar 16, 2018

FWIW Vimperator has an xpath which uses tabindex too:

const DEFAULT_HINTTAGS =
    util.makeXPath(["input[not(@type='hidden' or @disabled)]", "a", "area", "iframe", "textarea", "button", "select"])
        + " | //*[@onclick or @onmouseover or @onmousedown or @onmouseup or @oncommand or @role='link'or @role='button' or @role='checkbox' or @role='combobox' or @role='listbox' or @role='listitem' or @role='menuitem' or @role='menuitemcheckbox' or @role='menuitemradio' or @role='option' or @role='radio' or @role='scrollbar' or @role='slider' or @role='spinbutton' or @role='tab' or @role='textbox' or @role='treeitem' or @tabindex]"
        + (config.name == "Muttator" ?
            " | //xhtml:div[@class='wrappedsender']/xhtml:div[contains(@class,'link')]" :
            "");

@olmokramer
Copy link
Contributor

@The-Compiler re checking for addEventListener calls with special attributes, even that way it's not waterproof. There's this pattern for adding click handlers to elements that adds an event handler to the document that only triggers when some certain element is clicked. The advantage of this pattern is that you don't have to put a handler on every single element that you want to handle events on, even if they are added to the page dynamically. Wrapping addEventListener would put the attribute on document instead of the element you want. That said, I don't know if the pattern is still in use much, but it was a few years ago.

document.addEventListener('click', function onClick(event) {
    if (event.target.closest('.click-handler-class')) {
        // Handle click on .click-handler-class
    }
}, true);

@The-Compiler
Copy link
Member Author

@olmokramer That makes me wonder how (or whether) other projects handle this. Still I guess it'd be an improvement over what we have now.

@olmokramer
Copy link
Contributor

olmokramer commented Apr 8, 2018

@The-Compiler Well, you could get a lot of false positives, i.e. hints that do nothing. It wasn't really clear from my example, but document could really be any element, and that element would then get the hint, but because it only handles the event when it triggers on one of its children, nothing would happen.

That said, this problem would of course still exist if you don't wrap the addEventListener method but could get event listeners some other way.

@The-Compiler
Copy link
Member Author

@ykahveci Looks like Bootstrap CSS uses opacity: 0 to hide the underlying browser checkbox. This is now handled correctly in 798fabd, though that fix is Bootstrap specific. If you see more of those, please let me know!

@amacfie
Copy link
Contributor

amacfie commented Oct 11, 2020

FYI Pentadactyl gets elements with onclick events via a Firefox API. VisualEvent can get them if they were created with certain JS libraries.

@The-Compiler
Copy link
Member Author

Some more from #6278:

Those seem to work without JS. It looks like Vimium gives them hints because it looks at label elements which are not associated with any form control?

image

Those look like the usual JS event handler case.

@Soundtoxin
Copy link

Thanks for the Pleroma button fix. Those work now.

There are some more Pleroma buttons not getting hinted, but you have to be logged in to see them. (they show on the left of every page when logged in) They're for setting post scope (4 options ranging from DM to Public), uploading files, picking an emoji for your post (also a smaller button to do the same for the subject line), or starting a poll. Is there a way I can find out what I'd have to add to my config by investigating the page somehow?
And then if I find that out, what will my config line look like since I am already appending the stuff from before? Will I just write a second similar line?

2021-03-15-122908_grim

@The-Compiler
Copy link
Member Author

@Soundtoxin You can use :devtools to inspect the element (right-click) and then usually find some kind of ID/class to add to the selector. Then, something like c.hints.selectors['all'] += ['label.tocitem', '...'] in config.py should work.

If you have more questions, let's keep that in #6278 though please, as I'd like to keep this one focused on collecting cases which need work.

@Soundtoxin
Copy link

Checkboxes on eBay watchlist don't get hints
https://www.ebay.com/mye/myebay/watchlist

2021-03-15-185132_grim

@The-Compiler
Copy link
Member Author

https://cookieconsentspeed.run/ doesn't have hints for the cookie toggles:

image

Despite those being checkboxes:

image

image

@Soundtoxin
Copy link

Feedback buttons on eBay aren't hinted.
2021-03-28-075744_grim
2021-03-28-075936_grim

@The-Compiler
Copy link
Member Author

Another one not involving JS at all is Google's non-JS pages:

image

apparently those links show up as 0x0 elements:

image

which we discard, see #1298 and #1311.

With this patch:

diff --git i/qutebrowser/browser/webengine/webengineelem.py w/qutebrowser/browser/webengine/webengineelem.py
index 5d4c6ad9a..30f6e2420 100644
--- i/qutebrowser/browser/webengine/webengineelem.py
+++ w/qutebrowser/browser/webengine/webengineelem.py
@@ -179,33 +179,27 @@ def rect_on_view(self, *, elem_geometry: QRect = None,
         """
         utils.unused(elem_geometry)
         utils.unused(no_js)
-        rects = self._js_dict['rects']
-        for rect in rects:
-            # FIXME:qtwebengine
-            # width = rect.get("width", 0)
-            # height = rect.get("height", 0)
-            width = rect['width']
-            height = rect['height']
-            left = rect['left']
-            top = rect['top']
-            if width > 1 and height > 1:
-                # Fix coordinates according to zoom level
-                # We're not checking for zoom.text_only here as that doesn't
-                # exist for QtWebEngine.
-                zoom = self._tab.zoom.factor()
-                rect = QRect(int(left * zoom), int(top * zoom),
-                             int(width * zoom), int(height * zoom))
-                # FIXME:qtwebengine
-                # frame = self._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
-        log.webelem.debug("Couldn't find rectangle for {!r} ({})".format(
-            self, rects))
-        return QRect()
+
+        rect = self._js_dict['rects'][0]
+        width = rect['width']
+        height = rect['height']
+        left = rect['left']
+        top = rect['top']
+
+        # Fix coordinates according to zoom level
+        # We're not checking for zoom.text_only here as that doesn't
+        # exist for QtWebEngine.
+        zoom = self._tab.zoom.factor()
+        rect = QRect(int(left * zoom), int(top * zoom),
+                     int(width * zoom), int(height * zoom))
+        # FIXME:qtwebengine
+        # frame = self._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
 
     def remove_blank_target(self) -> None:
         if self._js_dict['attributes'].get('target') == '_blank':
diff --git i/qutebrowser/browser/webkit/webkitelem.py w/qutebrowser/browser/webkit/webkitelem.py
index 5bf96a610..adec6d776 100644
--- i/qutebrowser/browser/webkit/webkitelem.py
+++ w/qutebrowser/browser/webkit/webkitelem.py
@@ -204,25 +204,25 @@ def _rect_on_view_js(self) -> Optional[QRect]:
             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 = self._elem.webFrame().zoomFactor()
-                if not config.val.zoom.text_only:
-                    rect["left"] *= zoom
-                    rect["top"] *= zoom
-                    width *= zoom
-                    height *= zoom
-                rect = QRect(int(rect["left"]), int(rect["top"]),
-                             int(width), int(height))
-
-                frame = cast(Optional[QWebFrame], self._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
+            #if width > 1 and height > 1:
+            # fix coordinates according to zoom level
+            zoom = self._elem.webFrame().zoomFactor()
+            if not config.val.zoom.text_only:
+                rect["left"] *= zoom
+                rect["top"] *= zoom
+                width *= zoom
+                height *= zoom
+            rect = QRect(int(rect["left"]), int(rect["top"]),
+                         int(width), int(height))
+
+            frame = cast(Optional[QWebFrame], self._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 None
 

interestingly tests still seem to work fine? I want to understand what's actually going on before pushing that change, though.

@nemanjan00

This comment has been minimized.

@user202729

This comment has been minimized.

@nemanjan00

This comment has been minimized.

@crocket
Copy link

crocket commented Sep 21, 2021

Adding

c.hints.selectors['all'].append("label[for]")

to config.py makes :hint display hints for searx category headers.

Is it going to be okay to add label[for] to hints.selectors.all?

@The-Compiler
Copy link
Member Author

Is it going to be okay to add label[for] to hints.selectors.all?

I'd rather not if possible, as this results in a lot of duplicate hints for normal usages for label elements (which are much more common).

@crocket
Copy link

crocket commented Sep 21, 2021

Is label[for] common? In my experiences, most label elements don't have for attribute which links a label to an input element.

@neurosnap
Copy link
Contributor

neurosnap commented Oct 21, 2021

I spent some time groking the qutebrowser codebase to figure out how linting works. It seems like what we do is provide a list of selectors via config.hints.selectors and use that to grab all elements that match those selectors.

This seems like a relatively simple and comprehensive way to get pretty good results for hinting. However, I cross-referenced qutebrowser's implementation with Vimium's and they do more than just a css selector based approach. They actually iterate over every element in the DOM and then perform logic to figure out whether or not a link should be hinted.

For example, they do not simply check for all [tabindex] -- they make sure the tabindex >= 0 and is a number. This kind of logic we can get sort of close to just with CSS selectors but it's not exhaustive.

I think if we want to get results as good as vimium we should probably try to figure out how to apply logic to figuring out whether an element should have a hint visible beyond just the logic with CSS selectors.

I would love to a stab at an implementation that mimics Vimium's but I need to make sure the contributors would be receptive to such a massive overhaul to how we figure out what elements need hinted.

@The-Compiler thoughts?

@The-Compiler
Copy link
Member Author

I'm not really convinced this is a good idea, but it does warrant some further exploration. A couple of thoughts:

  • For the majority of cases, CSS selectors should be enough
  • CSS selectors are probably much faster, and hint performance was an issue in the past
  • CSS selectors make it easy for users to customize the behavior, which is nice and something I'd like to keep
  • Maybe we could add custom logic to post-process hints after hints.selectors - but I'd like to keep the JS code relatively "dumb", since we have less tooling around it (harder to write tests, for example), and less experienced contributors for it, given that qutebrowser is primarily a Python project. We could call JS snippets from Python, but see above re performance
  • We don't really have any good way to quantify whether changes we do make things better or worse, or how good our approach works when compared to Vimium's. Thus, I think the first step towards this should be some kind of collection of hinting test cases.

@neurosnap
Copy link
Contributor

We don't really have any good way to quantify whether changes we do make things better or worse, or how good our approach works when compared to Vimium's. Thus, I think the first step towards this should be some kind of collection of hinting test cases.

I'd be happy to help here. Any ideas on how you'd like to tackle this?

@The-Compiler
Copy link
Member Author

Sorry for the delay, a lot of stuff going on right now.

We already have a couple such test cases here: tests/end2end/data/hints/html. The corresponding test code lives in tests/end2end/test_hints_html.py. If you drop new HTML files in there, they should automatically be picked up, see the README of the directory for details.

Perhaps for some JS-heavy stuff the tests need some small adjustments so that it doesn't check for a target: ... but just checks whether a hint was found. But in general, the scaffolding should be there. The rather time-consuming part is extracting such test cases from real-world websites in a way that they're still real-world testcases, but still minimized enough. If that's something you'd want to take a look at, that'd be great!

@Tau5
Copy link

Tau5 commented Mar 15, 2023

Most buttons on classroom.google.com don't have hinting, they are normal div elements and they appear to have a jsaction property with the value click: followed by some obfuscated string

@twouters

This comment was marked as off-topic.

@The-Compiler
Copy link
Member Author

@twouters This is due to it being a cross-origin iframe:

@hakan-demirli
Copy link

Bing chat has some missing hints:
image

@The-Compiler
Copy link
Member Author

@hakan-demirli Seems to be due to it using shadow roots, so see #3569 / #7617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: hints Issues related to hinting ("f" key). priority: 2 - low Issues which are currently not very important.
Projects
None yet
Development

No branches or pull requests