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

Add support for more HTML attributes and style declarations in structured content #450

Merged
merged 4 commits into from
Dec 27, 2023

Conversation

stephenmk
Copy link

@stephenmk stephenmk commented Dec 26, 2023

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.

declaration values
color any string
background-color any string
text-decoration-style "solid", "double", "dotted", "dashed", "wavy"
text-decoration-color any string
border-color any string
border-style any string
border-radius any string
border-width any string
margin any string
padding any string
padding-top number
padding-left number
padding-right number
padding-bottom number
word-break "normal", "break-all", "keep-all"
white-space any string
cursor any string

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 and background-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.

  • I don't understand why the 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 and margin properties that will accept arbitrary string values, but I added padding-top, padding-left etc. properties that use only numbers similar to the corresponding margin-* properties. If we want, we can also update all of these properties to accept string values.

  • I'm not sure what the purpose of the "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 the list-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 that unset is fine.

@stephenmk stephenmk requested a review from a team as a code owner December 26, 2023 00:25
Copy link

github-actions bot commented Dec 26, 2023

✔️ No visual differences introduced by this PR.

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

@stephenmk stephenmk changed the title Add support for more HTML attributes and style declarations Add support for more HTML attributes and style declarations in structured content Dec 26, 2023
@toasted-nutbread
Copy link

toasted-nutbread commented Dec 26, 2023

color and background-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.

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.

I don't understand why the 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 and margin properties that will accept arbitrary string values, but I added padding-top, padding-left etc. properties that use only numbers similar to the corresponding margin-* properties. If we want, we can also update all of these properties to accept string values.

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 url(), but that should also be blocked by CSP so it really shouldn't be a problem.

I'm not sure what the purpose of the "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 the list-style-type style to be set on any styled element (div, span, td, etc.) but it only makes since 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 that unset is fine.

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.

Copy link

@toasted-nutbread toasted-nutbread left a 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.

@stephenmk
Copy link
Author

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.

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 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.

I updated the code to allow string values for these margin-* properties. I went ahead and made the padding-* properties string-only. I think the only reason we need number values is to maintain backwards compatibility.

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.

Sounds good. I just removed those ugly "unset" values.

@toasted-nutbread
Copy link

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.

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.

@djahandarie
Copy link
Collaborator

FWIW, perhaps not quite related, you can see a (kinda hacky) attempt at coloring dictionaries here:
https://github.com/themoeway/yomichan-dict-css
https://github.com/themoeway/yomichan-dict-css/blob/main/custom.css

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.

@djahandarie djahandarie added this pull request to the merge queue Dec 27, 2023
Merged via the queue into themoeway:master with commit adc17f4 Dec 27, 2023
5 checks passed
@djahandarie djahandarie added the kind/enhancement The issue or PR is a new feature or request label Dec 29, 2023
@stephenmk
Copy link
Author

I used a couple of Yomitan's color variables in the styles of the new version of my dictionary.
stephenmk/Jitendex#46

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;
}

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.

3 participants