Skip to content
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

theme in a publication does not apply everywhere #68

Closed
panaC opened this issue Jul 31, 2019 · 14 comments
Closed

theme in a publication does not apply everywhere #68

panaC opened this issue Jul 31, 2019 · 14 comments

Comments

@panaC
Copy link
Member

panaC commented Jul 31, 2019

Screenshot 2019-07-31 at 15 36 30
Screenshot 2019-07-31 at 15 38 58

H.+G.+Wells+-+The+Time+Machine.epub.zip

@JayPanoz
Copy link
Collaborator

JayPanoz commented Aug 1, 2019

Ah right so unlike night mode, sepia mode doesn’t override background-color.

So the following is not used in sepia mode:

:root:--night-mode *:not(a) {
background-color: transparent !important;
color: inherit !important;
border-color: currentColor !important;
}

The assumption was that it’s a light mode close to the default enough that asides with background-color could be preserved – e.g. O’Reilly books, but relevant in non-fiction more generally, cf. Kindle implementing a conversion process just for this in their pipeline.

I’m not sure what the best option is though, as by fixing this issue, we will automatically lose this nuance in non-fiction.

Note: I’m also wondering how this renders in night mode in both apps, since background-color is used to hide the border so if you transparentize the background, borders will be displayed.

@panaC
Copy link
Member Author

panaC commented Aug 1, 2019

ok, thanks, to follow

Screenshot 2019-08-01 at 09 55 10

@JayPanoz
Copy link
Collaborator

JayPanoz commented Aug 1, 2019

Ah thanks so I’ll have to fix that in night mode also.

Otherwise, I guess it would help to gather opinions on the following:

The assumption was that it’s a light mode close to the default enough that asides with background-color could be preserved – e.g. O’Reilly books, but relevant in non-fiction more generally, cf. Kindle implementing a conversion process just for this in their pipeline.

I’m not sure what the best option is though, as by fixing this issue, we will automatically lose this nuance in non-fiction.

So @llemeurfr and @danielweck for instance, since they were in the original issue. :-)

@llemeurfr
Copy link
Contributor

Hi @JayPanoz, for sure, the iBooks rendering of this specific book (from Feedbooks) is far better. And we know that many publishers are using iBooks as a reference for finalizing their EPUBs.

So I would be inclined to follow their choice and override background colors when themes are active, which would be better for accessibility purpose IMO.

@JayPanoz
Copy link
Collaborator

JayPanoz commented Aug 1, 2019

Thanks!

It is my understanding too that sepia mode may be used quite a lot in dys so a11y is one of my concerns.

JayPanoz added a commit that referenced this issue Aug 4, 2019
@JayPanoz
Copy link
Collaborator

JayPanoz commented Aug 5, 2019

@panaC I pushed fixes to the reading-mode-improvements branch so ReadiumCSS-after has been updated in dist (for all versions).

See diff.

Looks OK here but can you please confirm it’s also the case on your end before I merge on develop/master?

@danielweck I also updated highlights (re #65) i.e. putting them at the very end of the after stylesheet, adding selectors for sepia/night-mode + !important. Comments/review greatly appreciated, although not urgent by any means.

@danielweck
Copy link
Member

I will create a special / temporary build of r2-navigator-js in order to test the ReadiumCSS branch from r2-testapp-js. Once the sepia mode is confirmed to override the background (which it now should, just based on reading Jiminy's CSS code changes), then ReadiumCSS can be updated/published officially, and r2-navigator-js can reference the correct / merged ReadiumCSS Git revision hash.

@JayPanoz
Copy link
Collaborator

JayPanoz commented Nov 27, 2019

@panaC @danielweck Yikes, sorry for the delay, just emerging now and I can see the last comment was made on August, 5.

Is the fix OK? It’s been sitting in a branch for months now and should probably be merged into production.

@danielweck
Copy link
Member

Good point ... I need to revisit this! (will report back)

@JayPanoz
Copy link
Collaborator

Thanks.

And just to be sure as I edited my previous comment, the fix is in the reading-mode-improvements branch https://github.com/readium/readium-css/tree/reading-mode-improvements (and not staging)

@danielweck
Copy link
Member

Alll good, thanks Jiminy! :)

ReadiumCSS_Sepia

@JayPanoz
Copy link
Collaborator

Thanks for the confirmation, I will merge into develop first, in order to resolve #58 as well, then merge into master as alpha-3.

@danielweck
Copy link
Member

So, I can confirm that fixing (see PR #80 ) the performance issue related to the "blanket" application of background-color (sepia or night modes) on all elements in the document's body ... results in a "small" regression with borders / labelled outlines. I say "small", because in my tests the performance degradation was orders of magnitude more detrimental than the visually-annoying border outline (striked-through label).

Screenshots:

Screenshot 2020-03-22 at 13 18 06

Screenshot 2020-03-22 at 13 18 01

Of course, ideally we would find a solution to the performance issue that would not break publication documents ... but unfortunately I ran out of ideas (and time) so I settled for the workaround of applying a transparent background colour everywhere.

Also, note that my fix works in Chromium 80 (Electron 8) ... but your mileage may vary in other browser engines. I suggest using Children's Literature ( https://idpf.github.io/epub3-samples/30/samples.html#childrens-literature ) in order to test the performance impact of CSS design choices (including triggering the GPU 2D / 3D acceleration, in some cases, like I did in ReadiumCSS PR #80). That is because Children's Literature has a single relatively-large spine item in the reading order ( https://github.com/IDPF/epub3-samples/blob/master/30/childrens-literature/EPUB/s04.xhtml ), so when font-size is cranked-up between 150% or 200% for low-vision users, the CSS column pagination spans a great number of pixels on the horizontal axis (consequently, the browser engine's layout and rendering subsystem must then somehow optimize these textures in memory / on the graphics card, and it seems that some combination of CSS rules trigger a more performant render path)

@JayPanoz
Copy link
Collaborator

Thanks for the recap.

I say "small", because in my tests the performance degradation was orders of magnitude more detrimental than the visually-annoying border outline (striked-through label).

Yeah and it’s only one “CSS hack” we tried to handle here, I’m not sure we want to handle every conceivable CSS hack because they are de facto corner cases. For instance, we would have the same issue with leading dots in an ePub2-compatible way – using background-color.

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

No branches or pull requests

4 participants