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

Sanitize css before sending to Anki #1722

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented Dec 29, 2024

Prevents malformed css from making it to Anki. Browsers already sanitize css when adding it to the DOM so there's no need to add sanitization there.

@Kuuuube Kuuuube added kind/bug The issue or PR is regarding a bug area/anki The issue or PR is related to Anki integration labels Dec 29, 2024
@Kuuuube Kuuuube requested a review from a team as a code owner December 29, 2024 21:22
@Kuuuube Kuuuube changed the title Sanitize css before sending to anki Sanitize css before sending to Anki Dec 29, 2024
@djahandarie djahandarie added this pull request to the merge queue Dec 30, 2024
Merged via the queue into yomidevs:master with commit 8f4cf51 Dec 30, 2024
20 checks passed
@stephenmk
Copy link

A CSS parser such as the one provided by CSSStyleSheet will also be needed to resolve issue #1500, so it may be worthwhile to either require the functionality (i.e., produce a fatal error if it's not implemented by the user's browser rather than quietly failing) or include it in some third party library.

@Kuuuube
Copy link
Member Author

Kuuuube commented Dec 30, 2024

The reason for adding that is I highly suspect it isn't available in kiwi browser (to avoid #1701 part 2). Full on breaking compatibility with kiwi is a no-go since so many people use it for yomitan on android.

@jamesmaa if you could test if this that would be appreciated. KTY dicts are an easy test since all of the term dicts have a styles.css included. Or just run this in console and see if it errors:

sanitizer = new CSSStyleSheet();
sanitizer.replaceSync(".test { color: red; }");
[...sanitizer.cssRules].map((rule) => rule.cssText || '').join('\n');

It also does log the failures.

@stephenmk
Copy link

For what it's worth, csstree seems to be standards-compliant, actively developed, and usable within a browser. Maybe it could be used to parse CSS without introducing a dependency upon new specific browser features.

@djahandarie
Copy link
Collaborator

I agree we should test on kiwi, but my understanding is that:

  • The latest version of kiwi (124.0.6327.3) is based on Chromium 116
  • CSSStylesheet() constructor has been available since Chromium 76
  • replaceSync function has been available since Chromium 76

So I think it will probably be fine.

Pulling in new node deps and shipping them with the extension is best to avoid if possible.

@jamesmaa
Copy link
Collaborator

image

Seems to have worked on kiwi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/anki The issue or PR is related to Anki integration kind/bug The issue or PR is regarding a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants