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

hint-mode Shadow DOM elements support #3295

Closed
aadcg opened this issue Dec 21, 2023 · 4 comments · Fixed by #3365
Closed

hint-mode Shadow DOM elements support #3295

aadcg opened this issue Dec 21, 2023 · 4 comments · Fixed by #3365

Comments

@aadcg
Copy link
Member

aadcg commented Dec 21, 2023

As mentioned in #3269 (comment), hint-mode's support for Shadow DOM may need to be re-designed from scratch.

Related to #3254.

Performance benchmarks are needed as to ensure that the addition of this feature won't impact performance. E.g., mock an HTML document with thousands of links and return the time it takes for the renderer to draw the hints.

@aadcg aadcg changed the title hint-mode Shadow DOM support hint-mode Shadow DOM elements support Jan 3, 2024
@heiwiper
Copy link
Contributor

heiwiper commented Mar 19, 2024

Hello @aadcg !

I've been working on this issue these past days and I think there's no need for a major rewrite. The main problem which was causing the performance issue was the use of the macro rqs-nyxt-id on every hintable element of the DOM. The reason I was using that macro instead of rqsa (which would've made more sense as a replacement for qsa) is because the results of that macro should match the exact same order as the function clss:select which is used to generate hints. At that time the macro rqs-nyxt-id didn't cause any noticeable performance issue so I made the choice to use that macro in order to avoid having to keep rqsa and clss:select using the same ordering logic, otherwise changing one of them would break the other.

Anyway, I have resorted to updating the rqsa macro to produce a depth-first ordered elements, which worked fine for now.

However there's one issue that I would like to discuss with you, and which I think would affect other websites. So basically when the document model is updated through update-document-model command (which happens twice for hint-mode, once after adding the nyxt-hintable attribute to hintable elements, and the other one is after removing them) the new elements that are added to the dom and which weren't part of it wouldn't get added to the dom for timing reasons I would assume.

(I'm getting 2 breakpoints in the video because I was setting a sticker inside the add-hints function)
https://github.com/atlas-engineer/nyxt/assets/12894695/162fd0ff-4612-4eac-b904-a450cb9034ca

@aadcg
Copy link
Member Author

aadcg commented Mar 19, 2024

@heiwiper thanks for the update.

I'm not sure if I fully understood the issue you're raising about update-document-model so I'd suggest coming up with an example of this issue that is not related to the feature that you're developing (support shadow DOM elements on hint-mode). If it turns out to be difficult to showcase, then it may be that the renderer itself schedules the DOM mutation in a way that is impossible to control.

See tests/renderer-offline/hint-mode-html-document.html, which is a mock HTML document to test hint-mode. You can expand it to include shadow DOM elements. See what conclusions you can draw from it.

See document-model-delta-threshold, which might be relevant to reason about update-document-model.

@heiwiper
Copy link
Contributor

heiwiper commented Mar 23, 2024

I have followed your advice about creating an example of the issue and after some debugging I found out that the issue had nothing to do with update-document-model. I updated the rqsa algorithm to make use of nyxt-shadow-root attribute to improve its performance but then I noticed that the function set-hintable-element gets called before update-document-model which causes rqsa macro that is used within set-hintable-element function to ignore some shadow root elements that were added to the DOM.

Now I have reverted to walking through all DOM elements using Tree Walker which apparently has good performance compared to the other javascript methods.

I have created a draft PR to fix this issue, please let me know when you take a look into it.

@aadcg
Copy link
Member Author

aadcg commented Mar 25, 2024

@heiwiper thanks for the update. I'll look at it in a week.

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

Successfully merging a pull request may close this issue.

2 participants