-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Add hinting support for Shadow DOM nested elements #3001
Conversation
I have just tested the following interactive functions in
EDIT: I will actually revert changes for both EDIT: update |
a7a6883
to
acd7648
Compare
Right, new stylesheeds are not needed for Shadow DOM, because it inherits from the main frame already!
Yes, that'd be much cleaner. No hurry, though—this can be addressed separately. |
It's only used in heading scrolling, so testing it on the headings inside Shadow DOM seems the only testcase :P It should also be used for
Right, it'd need quite some refactoring to work with Shadow DOM. Let's figure it out with hints first and leave lots of |
Shadow DOM elements don't inherit stylesheet by default, the reason why I dropped that method is because I didn't see a point in drawing hint overlays in every Shadow DOM root element (this could be also the case for IFrames) when I can just append them to the main DOM and they would all be visible and would use the same stylesheet.
It doesn't seem complicated now that I implemented the the feature for hints, I will be working on it after I fix the hint scope probably. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, it's extremely elegant! I've left some minor notes regarding style here.
Regarding the non-working commands/macros/functions: proceed as you think best and ping me when you consider it ready :)
What won't hurt is a set of links you've tested the implementation on, so that I can see how it works.
source/parenscript-macro.lisp
Outdated
@@ -151,13 +166,15 @@ | |||
(radius (parse-float (chain computed-style border-top-left-radius))) | |||
(rounded-border-offset (ceiling (* radius (- 1 (sin (/ pi 4)))))) | |||
(offset (max coord-truncation-offset rounded-border-offset)) | |||
(el (chain document (element-from-point (+ (chain rect left) offset) | |||
(+ (chain rect top) offset))))) | |||
(el (chain ,element (get-root-node ,element) (element-from-point (+ (chain rect left) offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, it was there all this time?! That'd simplify a lot of code for IFrame indexing...
@@ -167,3 +184,16 @@ | |||
0) | |||
(= (chain window (get-computed-style ,element) "visibility") | |||
"hidden"))) | |||
|
|||
(export-always 'rqsa) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why define new variations for the existing Parenscript macros? Maybe simply replace qsa
with this rqsa
? Or is there something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that rqsa
will perform worse for the cases that qsa
currently covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Here's an idea for optimization: test whether there are any Shadow DOM nodes in the page, and call a regular qsa
when there's none.
Although I'm not sure how easy that'd be. Leaving it to your judgement, @heiwiper :)
EDIT: Typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Here's an idea for optimization: test whether there are any Shadow DOM nodes in the page, and call a regular
qsa
when there's none.
This would not be possible as far as I know, Shadow DOMs don't seem to have a straighforward API to select them from the main DOM, one would need to check every element and call the shadowRoot function to test whether it a Shadow root or not. I did some tests on pages containing Shadow DOMs and others that do not contain this type of elements using performance.now()
. It is definitely lacking in terms of speed, but it doesn't seem to have a signigicant impact but that is also relative to every webpage and how much content it contains.
There's actually a workaround that we could implement, we could check if an element is a shadow root in the function update-document-model
in buffer.lisp
and we add an attribute nyxt-shadow-root
to the shadow root elements. we already do some checks in that function and we modify elements by adding nyxt-identifier
. This would allow us to query for Shadow DOM elements using that attribute and we would be able to implement the optimization that you have suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, update-document-model
(or the underlying nyxt/dom::get-document-body-json
) is where I'd put it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @heiwiper! Not much to add. I second the request for testing URLs. |
Thanks a lot for the review! I will finish testing the remaining elements and review your comments as soon as possible. In the meantime, you can test the feature on the link that was posted as an example on the issue linked to this PR (here https://developer.servicenow.com/dev.do). |
acd7648
to
30d8f4b
Compare
I'm getting a weird behaviour when trying to fix this, the following expression returns |
My guess would be that the underlying (position (element (current-heading buffer))
headings
:key (compose #'nyxt/dom:body #'element)
:test #'equal) |
30d8f4b
to
4e751aa
Compare
This seems to solve the issue, please see the commit 4e751aa97. The PR should be ready now for another review @aartaka. Please let me know if there's any other changes to add. EDIT: Could please let me know if it is okay with you that I am force pushing changes ? I'm not sure if that's the best way to do it, but I try to make sure the PR commits are up to date with master so I cherry pick commits on top of master and force push. |
4e751aa
to
e9afd29
Compare
@heiwiper it's ok to force push to branches when a single person pushes commits to it, which it is the case here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks perfect to me, I've nothing to add.
So let's
- Rebase over master.
- Squash commits (I believe squashing into one would be the most sensible thing).
- Fixup commit messages (no backtics—plaintext commit messages are more accessible and don't depend on the Git/GUI parsing Markdown.)
- Go through the PR checklist once again (documentation and changelogs, mostly).
(i 0)) | ||
(dolist (element (nyxt/ps:qsa document "[nyxt-hintable]")) | ||
(let ((hint (aref hints i))) | ||
(nyxt-identifiers (ps:lisp (list 'quote nyxt-identifiers)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you can also (coerce nyxt-identifiers 'vector)
, although I'm not sure about the performance of such an approach 😛
Anyway, not critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should I update the doctstring for the function
nyxt:update-document-model
to mention the new attributenyxt-shadow-dom
? - Should I add docstring for already existing functions that I have modified ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you can also
(coerce nyxt-identifiers 'vector)
, although I'm not sure about the performance of such an approach stuck_out_tongueAnyway, not critical.
I tried to use the same code as the one used for hint
binding
This commit contains the following changes: dom: Make Nyxt DOM Shadow DOM aware buffer: Add nyxt-identifier attribute to Shadow DOMs elements buffer: Add new attribute to identify Shadow Root elements The new attribute allows checking if the main DOM has any Shadow DOMs by using the browser API querySelector and similar functions, otherwise it would be necessary to loop through all elements and check if the property shadowRoot is not null. (See https://developer.mozilla.org/en-US/docs/Web/API/Element/shadowRoot) parenscript-macro: Add Shadow DOM aware recursive qsa like macro This macro allows querying elements behind Shadow DOMs which is normally not possible due to encapsulation. mode/hint: Search for elements recursively through Shadow DOMs parenscript-macro: Add Shadow DOM aware qs-nyxt-id like macro parenscript-macro: Make qs-nyxt-id JS friendly This commit is inspired by atlas-engineer@3fd4402 It is reused to avoid compilation warning concerning undefined symbol nyxt/mode/hint::nyxt-identifier. dom: Get Interactive elements recursively through Shadow DOM mode/hint: Add hint overlays by nyxt-identifier parenscript-macro: Handle overlapped Shadow DOM child elements mode/document: Fix heading scrolling for pages that use Shadow DOMs
e9afd29
to
b855533
Compare
Hello! I would like to follow up on this pull request to ask whether there's anything else to be done on my side. |
Hey @heiwiper thanks for the ping, and I'm sorry for our negligence. I'll take a look at this on Monday. I believe it is ready. So I'll handle the conflicts and hopefully merge it as well! |
@heiwiper sorry for the delay. Looks ready to me. @jmercouris can I merge it? |
Yes, feel free André! |
@heiwiper I've tried to test the feature but, in my understanding, I'm not sure whether it's working. Steps:
Thanks. |
I would like to first thank you for your getting back to this PR. |
@heiwiper thanks! Indeed, my understanding of the changes is too basic, sorry about that. Could you provide an example of a URL that benefits from these changes? |
Alright, I will elaborate on this tomorrow! |
Yes, IFrame hinting is dropped and is totally unrelated to this PR.
|
Description
Shadow DOM nested elements are not currently handled by the hinting feature. This pull request aims to add support for these type of elements by expanding the search for hintable elements through Shadow DOM trees.
Fixes #1828
Discussion
I tried as much as I can to introduce less code additions and changes but I had to use the new macros
rqs
andrqsa
instead ofqs
andqsa
respectively inmode/hint.lisp
anddom.lisp
(Please keep in mind that for the replacements I made indom.lisp
I mainly tested the functionclick-element
and then added the same changes to the functions that are similar likecheck-element
, I wasn't sure how to test the rest and I did not want to waste more time figuring that out so I thought I would add this working code and confirm the other functions are working as expected).I would also like to precise that this pull request was mostly inspired by #2821 in the beginning due to the similarity between Shadow DOM and IFrame elements, but then I had to change most of the code due to some changes that introduced bugs like the one I have mentioned here. One of the changes that I had to drop was adding multiple stylesheets such as in the commit from the pull request I linked earlier.
I also want to suggest moving all the hint elements into a single parent element outside the main DOM body, the current behavior is appending the hint overlay elements to the body which makes it very cluttered and we would have to remove them one by one which wouldn't be the case if we had all elements under the same custom nyxt node, this is the case for saka-key and LinkHints .
Checklist:
cd /path/to/nyxt/checkout git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
:documentation
s written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)changelog.lisp
with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).migration.lisp
entry for all compatibility-breaking changes.(asdf:test-system :nyxt)
and(asdf:test-system :nyxt/gi-gtk)
) and they pass.