fix: improve unhighlightable contents check

[tefkah]

This addresses the following issue:

When adding a highlight decoration inside of a parent element which contains any nonhighlightable image, Readium will always fallback to the non-native highlight solution.

I think this is incorrect: it would be better to check whether the actual range we are attempting to highlight contains such elements.

I'm logging this issue to address the following issue in my attempt to add annotations/highlight to Storyteller.

• When highlighting within a paragraph, Readium will use the native Custom Highlight API, and a highlight will look like this usually
• When highlighting multiple paragraph in a chapter which contains any image (not even in the range), Readium will fallback to the default highlighting solution, which imo looks worse, but importantly looks different from the Custom Highlight API version.

This leads to the following:

Before

As you can see in the sidebar, both of these highlights should be green.
But because the second highlight falls back to the old solution, which has mix-blend-mode: exclusion set, it turns pink instead.

(ignore the weird edit button)

After

Correct result

It still works "correctly" when the range itself contains an unhighlightable element, such as the offending image

Other solutions

Provide a way to always use the default solution, ie opt out of using experimentalLayout altogether. This would at least make the highlights look consistent.

I've also thought about adding a MutationObserver to the _cFrames and immediately removing the mix-blend-mode: exclusion !important property, but that also leads to bad results

[JayPanoz]

Will review but as a quick note:

Removing the mix-blend-mode: exclusion !important property

Thorium Desktop went there and did that because it does not play nicely with accessibility and contrast.

ReadiumCSS, on which the navigator relies for applying theming, now let developers apply any arbitrary background color they want in version 2, so we will have to eventually remove and replace it with something that guarantees sufficient contrast, as was done in Thorium Desktop.

I did not have the opportunity to tackle this yet and it should not impact this PR unless this piece of information makes you comfortable attempting another fix you perhaps thought about but it is worth mentioning we will have to remove it anyway.

[tefkah]

ReadiumCSS, on which the navigator relies for applying theming, now let developers apply any arbitrary background color they want in version 2, so we will have to eventually remove and replace it with something that guarantees sufficient contrast, as was done in Thorium Desktop.

That's good to hear! That would ease most of my problems with this way of creating highlights. Especially for users using browsers that aren't up to date where they cannot use the native highlighting API.

[JayPanoz]

I'll double check on the desktop, but it seems reasonable to me we remove it right now, as there is no longer any reason to have it.

If I read things right, it is only checking the original dark mode, that is using the --USER__appearance flag from ReadiumCSS, that actually no longer exists as we removed reading modes recently.

I started creating a set of color helpers at the time I implemented the Preferences API as we discussed this issue with other maintainers, but didn't have time to test nor finalize these. And it doesn't mean we would end up using them directly in the Decorator, or at least you could enable/disable it.

The assumption is that developers know the background and text color they apply, so they can pick a color that is accessible, and we could help with that on their request instead of applying unexpected side-effects like this one, which doesn't even work well, as already demonstrated in another project.

[JayPanoz]

LGTM.

My only theoretical input would revolve around cloning contents of very large ranges, but admittedly that would be overzealous and leaning towards premature optimisation for an edge case we did not even encounter nor measure – I couldn’t find anything about this anyway through a quick search so probably not a practical worry.

@chocolatkey Do you want to give it a review too or should I merge?

On a related note, I will also remove the mix-blend-mode before publishing through another PR, and make the className consistent with the pattern we are using across the codebase – I can see it is using the pattern used in Thorium Desktop with r2- (for readium 2, that is not even a thing anymore).

[danielweck]

the "r2-xxx" naming convention (to minimize the risk of class name collisions) still exists in Thorium Desktop as this was never refactored ... definitely needs better naming in modern Readium incarnations.

[JayPanoz]

@danielweck I can recall that indeed, which is understandable as long as it is consistent in Thorium Desktop – with all its legacy.

I just noticed it may be surprising to users of the TS-Toolkit as we have a pattern of readium-prefixed classnames + dataset when adding things into the DOM. So while I'm at removing the mix-blend-mode, I can leverage this opportunity to make it consistent in Decorators.

[chocolatkey]

@JayPanoz @tefkah Is it not possible to run querySelector directly on the rangeFragment itself?

[chocolatkey]

FYI the r2 convention in the decorator code was copied from the mobile toolkit, in particular the code here: https://github.com/readium/kotlin-toolkit/blob/develop/readium/navigator/src/main/assets/_scripts/src/decorator.js

The code there also has many more advanced decoration options aside from highlights

[JayPanoz]

@JayPanoz @tefkah Is it not possible to run querySelector directly on the rangeFragment itself?

Indeed, considering cloneContents() returns a DocumentFragment, that has method querySelector, it should help spare the creation of a div and appending it.

AFK right now so I cannot check whether there might be issues with a document fragment obtained from range tho.