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

Improve hinting mechanism #1400

Open
1 of 4 tasks
aartaka opened this issue May 12, 2021 · 9 comments
Open
1 of 4 tasks

Improve hinting mechanism #1400

aartaka opened this issue May 12, 2021 · 9 comments
Labels
good-first-issue Marks a good issue for first-time contributors. hint-mode

Comments

@aartaka
Copy link
Contributor

aartaka commented May 12, 2021

Our hint-mode can still be greatly improved. We need to hint more elements and be better at filtering false positives.

Sources of inspiration:

@Ambrevar
Copy link
Member

Ambrevar commented May 13, 2021 via email

@Ambrevar Ambrevar added the good-first-issue Marks a good issue for first-time contributors. label May 17, 2021
@aartaka
Copy link
Contributor Author

aartaka commented May 21, 2021

ARIA roles are something to consider too.

@aartaka aartaka mentioned this issue Jun 30, 2021
@aadcg aadcg changed the title Hint more elements Improve hinting mechanism Aug 12, 2022
@heiwiper
Copy link
Contributor

I would like to work on fixing this issue but I have a one question after checking the references mentioned above.

Where does Vimium do better in detecting hintable elements ? I have checked the referenced function and It seems to include Shadow DOM and also there is the part where it checks whether the corners of the elements are clickable. But I still don't really understand the reason behind this so I would really appreciate if you could send me a reproducible case about these two points points or something else needed.

In the other hand, the overlapping issue should've been fixed in #2820 .

As for the selectors I will try to add those all while considering ARIA roles as mentioned above.

@aartaka
Copy link
Contributor Author

aartaka commented Apr 19, 2023

I have checked the referenced function and It seems to include Shadow DOM

Shadow DOM unawareness is severely lacking in our hinting mechanism, and help would be much appreciated if one knows what they're doing :) See #1828 for more context and examples.

There's also #1526 which I'm trying to fix, and which would also benefit from some help in #2821.

and also there is the part where it checks whether the corners of the elements are clickable.

Yeah, this one's weird, not sure what to do with it 🤔

@heiwiper
Copy link
Contributor

I have made progress on the issue related to Shadow DOMs, I did inspire from the pull request you have mentioned which tackles IFrame hinting. However, I'm currently stuck somewhere and I wanted to ask for advice.

I'm using almost the same function as the one you have added in this commit but for some reason when I inspect the values of ,node I notice that the root node is going through the second if statement which generates the following external error:

WARNING:
   Error on separate prompter thread: There is no applicable method for the generic function
                                        #<STANDARD-GENERIC-FUNCTION NYXT/DOM:GET-NYXT-ID (1)>
                                      when called with arguments
                                        (#<PLUMP-DOM:ROOT {100C750493}>).
See also:
  The ANSI Standard, Section 7.6.6

I tried to add progn to both statements so I can add (chain console (log ...)) to both of them and check if the root node is really going through the second if statements but it seems like the logs print the logs from the first statements only.

I know this doesn't really make sense but I was wondering if you had any similar issues or if you could let me know how I can debug this, there might be a parenscript feature that I am not aware of. I was also suspecting it might be a bug in parenscript in case I am not doing anything wrong.

For reference here's the function I'm using:

(export-always 'nyxt-node)
(defpsmacro nyxt-node (node)
  "Get the DOM element via JavaScript, based on the data from the Lisp side DOM.
NODE should be a nyxt/dom element."
  `(if (lisp (plump:root-p ,node))
       (progn (chain console (log "true"))
              (chain console (log (lisp (plump:root-p ,node))))
              document)
       (progn (chain console (log "false"))
              (chain console (log (lisp (plump:root-p ,node))))
              (labels ((get-by-ids (node ids)
                         (if (> (@ ids length) 1)
                             (get-by-ids
                              (ps:chain
                               (nyxt/ps:qs-nyxt-id node (elt ids 0)) shadow-root)
                              (chain ids (slice 1)))
                             (nyxt/ps:qs-nyxt-id node (elt ids 0)))))
                (get-by-ids
                 document (lisp (coerce (mapcar (quote ,(uiop:find-symbol* :get-nyxt-id :nyxt/dom))
                                                (uiop:symbol-call :nyxt/dom :shadow-root-parents ,node))
                                        'vector)))))))

@aadcg
Copy link
Member

aadcg commented May 23, 2023

@heiwiper I can't help much on this topic, but @aartaka may be able to help you.

With respect to debugging, my advice is as follows. Write an HTML file that captures the feature you're developing. Start Nyxt with (nyxt:start :urls '("/path/to/html/file")) and open the Inspector. You're able to run and inspect JS code since the PS you wrote and the HTML file are both locally available. Notice that in the same way that you're throwing console.log statements in your PS, you can also introduce debug statements that will trigger the WebKit inspector. Oftentimes, I start by writing the JS code (interactively, in the Inspector) and then I translate it to PS. Hope this helps.

@aartaka
Copy link
Contributor Author

aartaka commented May 23, 2023

@heiwiper, I remember encountering this issue while working on IFrame support. In my case, the solution was to hard-code the nyxt-node of the plump:root to be document. I'm not sure if it's on the iframe-hinting branch, though.

@heiwiper
Copy link
Contributor

ARIA roles are something to consider too.

I went through the ARIA roles in MDN and I came up with this list of roles that should be interactive:

I will try to work on adding support for these roles.

@aartaka
Copy link
Contributor Author

aartaka commented Jun 26, 2023

@heiwiper, thanks for this huge work, it all makes sense! Will be glad to merge 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Marks a good issue for first-time contributors. hint-mode
Development

No branches or pull requests

4 participants