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 alt attribute on structured content images #359

Merged
merged 6 commits into from
Dec 23, 2023
Merged

Add support for alt attribute on structured content images #359

merged 6 commits into from
Dec 23, 2023

Conversation

stephenmk
Copy link

@stephenmk stephenmk commented Dec 12, 2023

Dictionaries imported by yomitan may contain image files. The markup attributes for the images (width, height, alignment, etc.) are defined by the term bank schema. Currently the schema supports the title attribute, which allows users to view textual descriptions when hovering over images. However, the similar alt attribute is not currently supported. It would be useful if we could enable it.

For example, a dictionary might want to display the kanji 𬵪 (あおさば). This kanji is not supported by many fonts, so the text form (unicode character) likely will not display correctly for most users. We can replace it with an image of the kanji to ensure that users will be able to see it. It would also be nice if the users could highlight and copy the image and get the correct unicode character on their clipboard. This is possible by setting the alt attribute of the image equal to 𬵪.

It looks like this feature was intended to be added at some point, but the implementation was not finished. There is a description property defined for images within the term bank schema, but it is not used within the actual program (as far as I can see).

"title": {
"type": "string",
"description": "Hover text for the image."
},
"description": {
"type": "string",
"description": "Description of the image."
},

The "alt" attribute is currently set for images, but it is always set to an empty string.

const image = this._createElement('img', 'gloss-image');
image.alt = '';
imageContainer.appendChild(image);

I updated the program to set the alt attribute equal to the value of this description property instead of an empty string. After doing so, I noticed that it worked as expected with auto type (color) images, but it did not work with monochrome images. I found that for monochrome images, the display style on the img element was being set to hidden. It appears it was designed this way to allow monochrome images to change correctly when users switch between dark and light themes (monochrome text should be dark in light mode and light in dark mode). However, images cannot be selected when display is set to hidden, so the alt text would not copy to the clipboard. In order to work around this problem, I replaced the display: hidden style with opacity: 0 to achieve the same effect.

Everything seems to be working correctly now. I also created and tested some anki cards, and the alt text and monochrome functionality both carried over as expected. I'm not at all familiar with this code base so it's possible I may have overlooked something, so please take a careful look.

@stephenmk stephenmk requested a review from a team as a code owner December 12, 2023 03:49
Copy link

github-actions bot commented Dec 12, 2023

✔️ No visual differences introduced by this PR.

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

@stephenmk
Copy link
Author

I just noticed that the description property for images was only just added to dictionary-term-bank-v3-schema.json a couple weeks ago by @toasted-nutbread . I had wrongly assumed that the schema hadn't changed since the yomichan days.

Maybe this description property does already have a purpose that I'm not aware if. It might be better to add a new alt property to the schema instead.

@toasted-nutbread
Copy link

toasted-nutbread commented Dec 15, 2023

I just noticed that the description property for images was only just added to dictionary-term-bank-v3-schema.json a couple weeks ago by @toasted-nutbread . I had wrongly assumed that the schema hadn't changed since the yomichan days.

Maybe this description property does already have a purpose that I'm not aware if. It might be better to add a new alt property to the schema instead.

It was already in the codebase for some reason, just never in the schema, so I left it in and added it to the schema.

4da4827#diff-ce730242c96c30356b0f3b286e9d160ab920e11d878726a31b3d28a531232c98R480-R508

Since the codebase has types now, you can follow references to see how it's used:

description?: string;

Which takes you to here:

https://github.com/themoeway/yomitan/blob/1ae752b9fefb3e64114ebb4cd28f1fa0db9137c6/ext/js/display/display-generator.js#L434C1-L451

@stephenmk
Copy link
Author

If I'm understanding the code correctly, it looks like the purpose of the description property was to contain text to be displayed below or alongside the image. I tried testing it and couldn't get it to actually work, but that's outside the scope of this PR.

I added a new alt property to use for setting the HTML alt attributes of these images. I tested it in Chromium and exported some Anki cards, and it seems to work okay.

It seems the automated tests are failing because I made changes to the test dictionary. I'm not sure how to go about fixing that.

@toasted-nutbread
Copy link

It seems the automated tests are failing because I made changes to the test dictionary. I'm not sure how to go about fixing that.

This relates to comment 2. in #342 (comment). I can try to resurrect this functionality so you don't have to try and manually update those files.

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.

I should comment that the changes here look good, I'm just working towards getting the functionality to rebuild the test data with my recent PRs so we can fix the tests.

@toasted-nutbread
Copy link

Now that #415 is merged, you can rebase this and run npm run test-code-write to fix the tests. Inspect the diffs to make sure they look good, I can do that here also once it's pushed.

@stephenmk
Copy link
Author

you can rebase this and run npm run test-code-write to fix the tests

I'm not quite sure what this command did. I ran it and it didn't seem to change or fix anything.

I looked a bit harder at the test script and noticed that the expected media file counts in test/database.test.js needed to be updated. Changing those seems to have resolved the errors with the test suite.

@toasted-nutbread
Copy link

Oh, I might have made a wrong assumption that this was affecting some of the tests that use the JSON files as input/output. If all you needed to fix CI was to fix those numbers in the test file, then that should be sufficient.

@djahandarie djahandarie added this pull request to the merge queue Dec 23, 2023
Merged via the queue into themoeway:master with commit 93cdbe4 Dec 23, 2023
5 checks passed
@djahandarie djahandarie added the kind/enhancement The issue or PR is a new feature or request label Dec 29, 2023
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