-
Notifications
You must be signed in to change notification settings - Fork 100
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 support for more HTML attributes and style declarations in structured content #450
Conversation
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
Correct, this is the rationale for not originally adding them. That and they can't be themed easily by custom CSS, which leads to an inconsistent UI/UX. There is probably a better way to support this, such as by having the extension expose custom CSS vars for dictionaries to use for colors, but ultimately that would still just involve using custom string values for color/background-color.
Because I initially designed structured content with a simpler use case in mind. For what it's currently approaching, I don't think there is still a real reason to try use integer number types, it was originally just to ensure valid CSS is always used, but strings should be fine. The only use case that I can think that may be dangerous is a
Was originally just to show how what value is used when the field is omitted so people don't have to dive into the code. Feel free to omit. |
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.
Don't see any issue.
I think the better way is to allow Yomitan dictionaries to use stylesheets somehow (which could be more easily edited by users) instead of having inline styles be defined on every element. Not something we can do anything about here and today, though.
I updated the code to allow string values for these
Sounds good. I just removed those ugly "unset" values. |
This does sound like a better idea. Given that Yomitan is no longer targeting legacy browsers, this can probably be done relatively easily with shadow DOM for CSS scoping as well. Would have to look more into it to see what difficulties may arise form that however, but either way a task for another time. |
FWIW, perhaps not quite related, you can see a (kinda hacky) attempt at coloring dictionaries here: We settled on introducing two variables, --dict-color, and --dict-bg-opacity, which then get processed into actual color and background-color here: https://github.com/themoeway/yomichan-dict-css/blob/main/custom.css#L15. It seemed like the minimum required to get a decently balanced color palate, while reducing the redundancy of manually defining both color and background-color. Perhaps we could consider some sort of similarly 'partially-restricted' coloring options that force users to define things in a way that plays better with light&dark mode. Anyways, this PR seems okay so I will merge, let's consider how to further improve it in the future. |
I used a couple of Yomitan's color variables in the styles of the new version of my dictionary. One thing to bear in mind is that these variables don't get exported to Anki. Users have to add some custom styles to their cards if they want everything to look correct. Fortunately for my use-case they still look acceptable even without those custom styles. .card {
--text-color: black;
--background-color: white;
}
.card.nightMode {
--text-color: white;
--background-color: black;
} |
This PR is to add some more valid HTML attributes and style options to structured content glossary terms.
I am currently using embedded SVG images in my dictionary to implement many of these features (colors, borders, tooltip text). It's an awkward workaround with a few issues, so it would be nice if Yomitan could support these features natively without the SVG hack.
Here is a list of the style properties I have added.
I have also allowed for the
title
attribute to be set on container elements (div, span, ol, ul, li). This enables support for tooltip hover text.I added a new term bank file to the test valid dictionary containing a term for ばね【発条】. This term uses all of the above new features. I also exported this term to Anki from Chromium and it seemed to work fine.
There are a few more things to consider.
color
andbackground-color
styles are dangerous because Yomitan supports light and dark themes. Colors that are legible in one theme may not be in the other, so dictionary authors must be cautious. Still, I don't think that's a good reason to forbid their usage in structured content. I am already achieving the same effect via SVG images at any rate.margin-top
,margin-left
, etc. properties are currently restricted to number values in the term bank schema. These properties can accept a variety of different string values (5%
,10px
,1em
, etc.). There don't seem to be any security reasons to disallow arbitrary strings in these fields, and I think CSS always fails gracefully when dealing with invalid values. I think this restriction makes things needlessly difficult. When I convert an HTML page to Yomitan JSON, I have to parse and convert the numbers from these style strings.I added new
padding
andmargin
properties that will accept arbitrary string values, but I addedpadding-top
,padding-left
etc. properties that use only numbers similar to the correspondingmargin-*
properties. If we want, we can also update all of these properties to accept string values."default"
values in the term-bank schema is. JSON schema validators usually include an option to add the default values to structures that haven't set them, but this behavior isn't used by Yomitan and it's not even something you'd want to do. For example, the schema technically allows thelist-style-type
style to be set on any styled element (div, span, td, etc.) but it only makes sense to use that style with list elements.I set the "default" schema values for many of the new properties to
unset
. To be honest, I don't fully understand the difference between these types of CSS values (unset, revert, initial, inherit, etc.). I assume thatunset
is fine.