Skip to content

luci-base: remove bad Unicode on clone button #7746

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dwfreed
Copy link

@dwfreed dwfreed commented Apr 23, 2025

  • This PR is not from my main or master branch πŸ’©, but a separate branch βœ…
  • Each commit has a valid βœ’οΈ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid πŸ“ <package name>: title first line subject for packages

This Unicode character is intended to be followed by 2 more characters which are combined. It's entirely unnecessary, so just remove it.

@dwfreed
Copy link
Author

dwfreed commented Apr 23, 2025

Note: this should be backported to 24.10 as well

@systemcrash
Copy link
Contributor

There are no bad unicode characters - just the choice of fonts displaying them.

https://www.unicode.org/charts/PDF/U2FF0.pdf
βΏ» - U+2FFB
"These are visibly displayed graphic characters, not invisible composition controls."

@dwfreed
Copy link
Author

dwfreed commented Apr 23, 2025

No useful fonts implement these characters, and they look like garbage to everybody, but sure, show garbage to everybody

@dwfreed
Copy link
Author

dwfreed commented Apr 23, 2025

image

@systemcrash
Copy link
Contributor

What do you run, windows 3.11? https://fonts.google.com/?preview.text=%E2%BF%BB

@dwfreed
Copy link
Author

dwfreed commented Apr 23, 2025

https://www.fontspace.com/unicode/char/2FFB-ideographic-description-character-overlaid lists a total of 13 fonts that implement the character, none of them appear like they'd be default on any standard installation. https://www.fileformat.info/info/unicode/char/2ffb/fontsupport.htm lists 6, though 2 of them are fallback fonts. Your own link only shows the actual character for the Noto fonts for China, Japan, and Korea and related locations, as far as I can tell. All the rest are the same as my screenshot.

@dwfreed
Copy link
Author

dwfreed commented Apr 23, 2025

Using your link, I made one change, restricting the language to English, and then scrolled through 1,599 fonts. This excluded the Noto Chinese, Japanese, and Korean fonts I mentioned. I saw 4 fonts that implemented this character:

  • LXGW WenKai Mono TC
  • LXGW WenKai TC
  • Chocolate Classical Sans
  • Cactus Classical Serif

Copy link
Member

@ynezz ynezz left a comment

Choose a reason for hiding this comment

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

LGTM. That icon button makes the UI inconsistent, breaks the rhythm, there is no spacing between icon and label, the dashed outline is hard to see on dark gray and I believe, that the icon should be before the label. I would add Fixes: #7754 Git trailer.

This Unicode character is intended to be followed by 2 more characters
which are combined. It's entirely unnecessary, so just remove it.

Fixes: openwrt#7754
Signed-off-by: Doug Freed <dwfreed@mtu.edu>
@Ramon00
Copy link
Contributor

Ramon00 commented May 11, 2025

Using your link, I made one change, restricting the language to English, and then scrolled through 1,599 fonts. This excluded the Noto Chinese, Japanese, and Korean fonts I mentioned. I saw 4 fonts that implemented this character:

  • LXGW WenKai Mono TC
  • LXGW WenKai TC
  • Chocolate Classical Sans
  • Cactus Classical Serif

I cannot say in how many it is implemented, but on my screen it just shows as it should:
image
(both on android and Windows using chromium Edge).

I did not install any strange fonts or anything.

On some fonts it may look bad, but lets not over exaggerate the problem. Can we not add a check in the code if the character is defined in the font that is used? In that way everybody is happy

@Ramon00
Copy link
Contributor

Ramon00 commented May 11, 2025

Seems it is implemented in rather default fonts: https://unicodeplus.com/U+2FFB
image

@GeorgeSapkin
Copy link
Contributor

Seems to work here on Firefox 138 on Fedora 42, with no extra fonts installed. Ditto for the latest Firefox on Android 15. Not sure what the issue is. Might look better with an extra space, sure.

@Ramon00
Copy link
Contributor

Ramon00 commented May 11, 2025

Maybe use something like this to detect if the element is defined:

function isUnicodeCharacterSupported(char, element = document.body) {
    const canvas = document.createElement("canvas");
    const ctx = canvas.getContext("2d");

    const font = window.getComputedStyle(element).font;
    ctx.font = font;

    const referenceWidth = ctx.measureText("\uFFFD").width; //fallback character
    const charWidth = ctx.measureText(char).width;

    return charWidth !== referenceWidth;
}

then invoke isUnicodeCharacterSupported("\u2FFB")

This checks if the width is the same as the refenence width. Not idiot proof but should work most of the time, if accidentally the width is the same the only consequence is that the symbol is missing (not the end of the world).

@dwfreed
Copy link
Author

dwfreed commented May 11, 2025

Or, you know, just not use a character that requires special font support and serves no practical purpose... One solution seems massively simpler than the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants