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

Allow clip-path, background, text-emphasis styles in structured content #556

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Conversation

stephenmk
Copy link

@stephenmk stephenmk commented Jan 22, 2024

The clip-path style will allow me to resolve this issue regarding fonts with weird line heights.

The background style will allow me to to add some nice color gradients.

This is almost exactly identical to my previous pull request. Hope these PRs aren't too troublesome.

Thanks!

@stephenmk stephenmk requested a review from a team as a code owner January 22, 2024 07:16
Copy link

github-actions bot commented Jan 22, 2024

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

@toasted-nutbread
Copy link

Out of curiosity, how are you currently achieving the circle background?

@stephenmk
Copy link
Author

stephenmk commented Jan 22, 2024

Currently using a combination of padding and border-radius. You can see an example in the commit diff for term_bank_2.json.

If you know a better way, I'd be very interested to learn it. I've been struggling to find a clean way to implement this effect. I have a hunch that the best way would probably involve first-class support for SVG in Yomitan, but that might be overkill.

@toasted-nutbread
Copy link

How about before, in the issue you linked in the OP?

@stephenmk
Copy link
Author

The example in Yomitan's valid test dictionary (term_bank_2.json) was pulled from an older version of jitendex. I believe the screenshots that marv and I posted in that github issue are all from jitendex dated 2024-01-14. The padding values and border styles are a little different in that version of jitendex, but it still has the same problem.

Here's the style on the green 優 span from the 2024-01-14 version.

  color: white;
  background-color: green;
  border-color: green;
  border-style: solid;
  border-radius: 100%;
  border-width: 0.75px;
  padding: 0.15em 0.15em 0.05em;
  cursor: help;

Regardless of the version, I can reproduce the error in all of them by setting the font-family style on the element to Noto Serif CJK JP.

@stephenmk stephenmk marked this pull request as draft January 27, 2024 08:52
@stephenmk
Copy link
Author

I'd like to add support for the text-emphasis style to this PR (see this issue), so I'm converting this to a draft PR for now. I'll have time to add it tomorrow.

@stephenmk stephenmk changed the title Allow clip-path and background styles in structured content Allow clip-path, background, text-emphasis styles in structured content Jan 27, 2024
@stephenmk stephenmk marked this pull request as ready for review January 27, 2024 19:40
@djahandarie
Copy link
Collaborator

Introducing background potentially exposes us to external resources being referred to using url(), which could allow a malicious dictionary to start leaking some information like user IPs, right?

I assume the Content-Security-Policy of default-src 'self'; script-src 'self' 'unsafe-eval'; img-src blob: 'self'; style-src 'self' 'unsafe-inline'; media-src *; connect-src * blocks this currently, but if we ever allow external images to be loaded (e.g., for an automatic image search functionality), then it would open up the extension to an attack here... 🤔

@toasted-nutbread
Copy link

IMO we should never allow the CSP to load external resources. Anything that should need this can already be done in another way, for example, loading the image in the background page and creating a temp blob: resource for it. But I can't think of a great reason why this would ever need to be done.

The closest thing that we presently do for this is audio play back from 3rd party sites, and this is obviously separate from what CSS can do.

@stephenmk
Copy link
Author

Just noting that url() can also be used as a value for border and cursor properties, which have already been merged into Yomitan.

@toasted-nutbread
Copy link

I don't think border itself can do images, since it's only a shorthand for -style, -width, and -color, but you're right about cursor.

Even if this ever does become a concern, it's easy to just detect unsafe CSS (i.e. using url()) and not apply it.

@stephenmk
Copy link
Author

Haven't tested it with border myself; just going by what Mozilla says. It sounds very odd to me too.

The url() function can be included as a value for background, background-image, border, border-image, border-image-source, content, cursor, filter, list-style, list-style-image, mask, mask-image, offset-path, clip-path, src as part of a @font-face block, and @counter-style/symbol

@toasted-nutbread
Copy link

I'd just chalk that up MDN having some incorrect info, or there's some legacy/rare browser case I'm not aware of, or i'm doing something wrong.

Setting a property to border: url('/media/examples/border-diamonds.png'); (example from here) in the web inspector shows it as invalid.

Oh well ¯\_(ツ)_/¯

Copy link
Collaborator

@djahandarie djahandarie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to keep a mental note (or better, document) the importance of the Content Security Policy somewhere

@djahandarie djahandarie added this pull request to the merge queue Jan 31, 2024
Merged via the queue into themoeway:master with commit 6807b05 Jan 31, 2024
5 checks passed
@djahandarie djahandarie added the kind/enhancement The issue or PR is a new feature or request label Feb 10, 2024
MarvNC added a commit to MarvNC/yomichan-dict-builder that referenced this pull request Jul 7, 2024
MarvNC added a commit to MarvNC/yomichan-dict-builder that referenced this pull request Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circles Are Ovally When the Display Font Is Changed
3 participants