search

themoeway / yomitan

Pop-up dictionary browser extension. Successor to Yomichan.
https://yomitan.wiki
GNU General Public License v3.0
1.2k stars 94 forks source link

`vertical-align` style on structured content hyperlinks causes visual glitches #271

Closed stephenmk closed 11 months ago

stephenmk commented 1 year ago

Hyperlinks in yomitan (both internal links and external links) receive a gloss-link-text class which is styled with a vertical-align property set to middle by default.

This was introduced in this commit last year and merged in this pull request.

Like a lot of the structured content functionality, I don't think this feature had the opportunity to be extensively tested before yomichan reached end-of-life earlier this year. I'm not sure why toasted-nutbread chose to set this attribute to middle; I don't see any discussion about it in the above links. But over the past year I've noticed many instances of this style causing visual glitches in dictionaries which use the feature.

Notably, the style causes some fonts to clip through the hyperlink underline. Take a look at the first image in UMNV's post here and note how the 浮世絵 text in the hyperlink is partially covered by the underline.

I think this behavior only appears with certain choices of fonts, font sizes, and/or zoom levels, which would explain why toasted-nutbread didn't see it.

Today I noticed that the style causes the underline to form unevenly when the hyperlink contains a ruby element.

hyperlinked ruby text with vertical-align set to `middle` ![saba_wo_yomu_vertical_align_middle](https://github.com/themoeway/yomitan/assets/8003332/78712c51-582b-402b-8833-69523a38623b)
hyperlinked ruby text with vertical-align style disabled ![saba_wo_yomu_no_vertical_align](https://github.com/themoeway/yomitan/assets/8003332/94b923ef-4eaf-4952-a167-790fe1698f03)

I've been running yomichan with a style .gloss-link-text {vertical-align: inherit;} in my custom CSS for many months without any issue. I think this issue can probably be resolved simply by removing the middle style in the ext/css/structured-content.css file. I've been procrastinating on putting together a thoroughly tested pull request to that effect for a while now, so I thought I may as well open this issue to get the ball rolling.

djahandarie commented 12 months ago

@stephenmk Given how long you've been running the patch with no issue, I think it's okay to open a PR for it now and merge. If someone finds a bug in the other direction then we can figure out how to fix it for both cases at that time. :pray:

stephenmk commented 11 months ago

We still don't know why toasted-nutbread set it to middle in the first place. Hopefully it wasn't to solve some other issue that we haven't noticed. I tried my best to thoroughly test the change to baseline in both Firefox and Chrome, so it must be a pretty obscure issue if it does exist. I guess we can close this now.