edrlab / thorium-reader

A cross platform desktop reading app, based on the Readium Desktop toolkit
https://www.edrlab.org/software/thorium-reader/
BSD 3-Clause "New" or "Revised" License
1.86k stars 157 forks source link

annotations highlight engine: Electron v33 beta 5 introduces Chromium revision upgrade which breaks color calc in CSS Highlights #2586

Closed danielweck closed 1 month ago

danielweck commented 1 month ago

https://github.com/electron/electron/releases/tag/v33.0.0-beta.5

=> Chromium 130.0.6723.19 (jump from 130.0.6723.6)

https://chromium.googlesource.com/chromium/src/+log/130.0.6723.6..130.0.6723.19?n=10000&pretty=fuller

Screenshot 2024-10-03 at 01 16 20 Screenshot 2024-10-03 at 01 12 25

The breaking change in the underlying CSS implementation is in both:

color: oklch(from rgb(R, G, B) clamp(0, (0.7 / l - 1) * infinity, 1) c h);
color: color(from rgb(R, G, B) xyz-d65 clamp(0, (0.36 / y - 1) * infinity, 1) clamp(0, (0.36 / y - 1) * infinity, 1) clamp(0, (0.36 / y - 1) * infinity, 1));

More complete CSS:

@supports (color: oklch(from red l c h)) {

    ::highlight(${cssHighlightID}) {
        background-color: rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue});

        color: oklch(from rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue}) clamp(0, (0.7 / l - 1) * infinity, 1) c h);

        text-shadow: none;
    }
}

@supports (color: oklch(from color-mix(in oklch, red, tan) l c h)) {

    ::highlight(${cssHighlightID}) {
        background-color: rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue});

        color: color(from rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue}) xyz-d65 clamp(0, (0.36 / y - 1) * infinity, 1) clamp(0, (0.36 / y - 1) * infinity, 1) clamp(0, (0.36 / y - 1) * infinity, 1));

        text-shadow: none;
    }
}

The contrast-color directive remains unrecognised so this code isn't activated:

@supports (color: contrast-color(red)) {

    ::highlight(${cssHighlightID}) {
        background-color: rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue});

        color: contrast-color(rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue}));
        text-shadow: none;
    }
}

... consequently, white is the active fallback due to the 2 aforementioned failures:

::highlight(${cssHighlightID}) {
    background-color: rgb(${highlight.color.red}, ${highlight.color.green}, ${highlight.color.blue});

    color: white;
    text-shadow: 0 0 .05em black, 0 0 .05em black, 0 0 .05em black, 0 0 .05em black;
}

I pinned Electron to v33 beta 4 but it is now at beta 6 and I fear this bug will remain ... I will monitor the situation. Bottom line: we cannot release low contrast ratio text foreground/background in our annotations implementation, so if the final stable Electron v33 continues to exhibit this Chromium regression we will need a workaround. Fingers crossed, this will automatically be fixed in a future Chromium release of the v33 Electron beta / RC revisions!

danielweck commented 1 month ago

FYi, I informed the author of the CSS technique about the broken functionality: https://github.com/LeaVerou/lea.verou.me/issues/70

danielweck commented 1 month ago

Fixed via https://github.com/readium/r2-navigator-js/commit/4e426fdb4ee15af28e13f6f183b05651a6f811be

danielweck commented 1 month ago

the issue is now tracked in the Chromium project:

https://issues.chromium.org/issues/371224775