-
Notifications
You must be signed in to change notification settings - Fork 20
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
Improve highlights, especially in night mode #65
Comments
The current prototype in |
So, the current ReadiumCSS draft proposal is to use CSS class names: https://github.com/readium/readium-css/blob/master/css/src/modules/ReadiumCSS-highlights.css This will not be usable in practice with the |
Yeah I’m wondering whether we shouldn’t even get highlights out of ReadiumCSS now, given this also reminds me of the Highlight API doc. And more generally, it will be a lot more complex to handle/customise if it’s in ReadiumCSS. |
Yes, ReadiumCSS is by definition "opinionated" (that is what makes it useful), so at best ReadiumCSS could define highlight colours that "work" with its own predefined themes. That being said, if the styles authored at the source in EPUB content documents deviate from the typical "back text on white background", then all bets are off, and it is not possible to guarantee acceptable contrast levels / readability. |
Sorry had to sleep over this following your remarks. A few thoughts. Overall, I think there is a possibility I align the module with all others, while providing defaults, and extensibility.
It’s really still more of a placeholder than a draft at this point so classes here are more like references making “highlight colours” obvious. That also means this module is completely different than all others in its design right now. For instance, a RGB colour value (triplet) could become 4 CSS variables (one for R, one for G, one for B, and one for alpha), as seen in this article for HSL(a) for instance. But, as you highlighted…
That could be handled via a “type” flag probably, once again as a CSS variable.
Just as a note because I haven’t had the time to read it thoroughly but there are attempts to handle text based on the background-color e.g. https://css-tricks.com/css-variables-calc-rgb-enforcing-high-contrast-colors/ (there are also the
So I think there is a possibility of having opinionated set of triplets while allowing for customisation. I’ll try coming up with a proposal asap so that it’s clearer but, something like (pseudo CSS code)…
Knowing that if you had other values as inline styles on the element itself, those inline values will be used for background-color, etc.
Not a silver bullet, mind you, but at least that would offer defaults + could still be used dynamically if the RS hasn’t implemented the feature yet – I’m really fine with JS implementations for such features, given CSS is lacking for such complex issues. |
Yes, this looks like a step in the right direction. Thanks Jiminy. |
Related notes:
Well at least now I have a clearer idea how the change will turn out, and it’s not massive work – documenting it will be the heavier task. |
Also for reference: styling + SVG handling in r2-navigator-js |
At first glance the Android highlight implementation is not using Readium CSS's highlight classes, and I also wonder if it should be part of Readium CSS or not. To align platforms, we can use a shared specification with a set of default colors, but I would expect reading apps to want to customize these colors. If there was a way to have different color variants per reading mode in Readium CSS, it could make it worthwhile though. |
So let’s use this comment for voting on the removal of this module in ReadiumCSS. in this mobile issue, @mickael-menu said:
We also know the Thorium implementation doesn’t use ReadiumCSS, and have this r2-navigator-js issue re. dev needs, which can inform our decision. What’s become clearer is that ReadiumCSS would be way too constraining implementation-wise as you would have to rely on what it provides as CSS variables for In addition, we shouldn’t forget other formats – PDF as mentioned by Mickaël but I’m assuming web publications could have TTS with sync/overlaid highlights, notes, etc. as differentiating features from web browsers. If you’re in favour of removal, use 👍 . Otherwise, use 👎 |
Further data points to consider - in
Furthermore, CSS Without going into too much detail, you can see that there are several optimization paths to achieve decent performance when dealing with thousands of character-level drawings (e.g. to present search results inside HTML documents). These techniques make it hard to standardize on simple ReadiumCSS styles / rules. Note that we use the same CSS styling techniques for both SVG and HTML render modes. Also note that "highlights" in |
Ah thanks for these details. If ReadiumCSS had to handle all of this, that would turn this module into a labyrinthine system indeed so it’s not worth bringing confusion into an already tricky issue. |
That's smart :) I wonder if a similar "static" strategy could be used for mobile, where we don't have mouse hover. |
So the highlights module has been removed in the develop branch – which means Nobody complained about this change so we’re assuming this shouldn’t be a breaking change. But be aware that if you were relying on these highlights styles, there won’t be provided any longer. |
I guess closing this issue is long overdue. 😅 |
I'm submitting a feature request (raised from a bug report during yesterday’s engineering call).
Short description of the issue/suggestion:
Highlights module has been kind of a placeholder for a long time as we never discussed those properly in the past, so not many people use it, and it’s not even compatible with night mode.
The selector isn’t even customisable like most others in src.
Steps to reproduce the issue/enhancement:
N/A.
What is the expected behaviour?
This module should at least be updated to solve the two issues above, but the opportunity to have a “standard” for all Readium-related apps is perhaps even more important there, as it could offer a good-enough default for a lot of apps.
What is the current behaviour?
See description.
Do you have screenshots, GIFs, demos or samples which demonstrate the problem or enhancement?
N/A
What is the motivation / use case for changing the behaviour?
Provide better defaults for highlights, in a way that makes sense for implementers.
Do you know which CSS modules (stylesheets) are impacted?
Please tell us about your environment:
The text was updated successfully, but these errors were encountered: