readium / readium-css

šŸŒˆ A set of reference stylesheets for EPUB Reading Systems, starting with Readium Mobile
https://readium.org/readium-css/
BSD 3-Clause "New" or "Revised" License
92 stars 20 forks source link

FYI: NPM packages updates, consistent multiline CSS selectors (lint), backgroundColor fix (performance) #80

Closed danielweck closed 4 years ago

danielweck commented 4 years ago

I was in the process of updating r2-navigator-js with ReadiumCSS from the master branch, but I realised that the dist/ folder was out of date, so I attempted to npm install && npm run build in order to update dist/ myself. Unfortunately this failed on MacOS when gyp-compiling fsevents, and as I realised that the NPM packages were out of date, I decided to update them all (rm -rf node_modules && rm package-lock.json && npm install). Whilst I was there, I also decided to fix linting inconsistencies in multiline CSS selectors.

Note that npm run test fails (50 non-pass), but I am not sure whether this is specific to this PR (I haven't tried in the regular master branch, which I am not able to install/build)

So, I'm not sure whether this PR is worth merging, your call @JayPanoz :)

danielweck commented 4 years ago

Well, that's "interesting": in r2-testapp-js / r2-navigator-js I was experiencing a massive performance hit in sepia and night modes (i.e. laggy / janky rendering during page turn transitions / animated translations, in particular). After some trial+error tweaking CSS properties using the Chromium web inspector, I discovered that setting transparent for all elements (but the root HTML, obviously) is a reliable workaround. So I included this fix in the PR, as this is critical for the "Readium desktop" integration context. I think this is a safe change for other platforms too, but: to be verified.

PS: I use the large "children's litterature" single-chapter publication for my CSS bottleneck tests. I've now fixed a number of significant performance issues related to web browser painting / update tree / compositing, using a combination of will-update and translate: translate3D() and translate: translateX() (2D). The bugs tend to manifest themselves in paginated mode ... I guess the regular scroll view is much more optimized, whereas CSS columns tend to break "things"? Side note: for annotations / highlights, it seems HTML divs render quicker than SVG (shape combinations of semi-opaque rectangles, overlaid on top of text) once the appropriate CSS tricks are used to trigger the webview's performance path.

JayPanoz commented 4 years ago

Thanks!

A few notes before diving into this:

Note that npm run test fails (50 non-pass)

I believe there was a breaking change in BackstopJS at some point, because Iā€˜ve used it for visual regression testing in other projects and noticed some differences in the config so Iā€™ll have to give it an extended look (master then the PR branch).

As a sidenote for the longer term, BackstopJS has lost one of its main advantages over say puppeteer + chromium: supporting Webkit and Firefox rendering. I understand why, especially Webkit which was relying on the now unmaintained PhantomJS but the option of testing different rendering engines was definitely a plus. That said last time I checked, it wasnā€™t trivial to switch to Puppeteer, although it looks like people have been working on that.

I discovered that setting transparent for all elements (but the root HTML, obviously) is a reliable workaround.

Ah so I can see I changed transparent to CSS var in this commit: https://github.com/readium/readium-css/commit/79a61fc3b03dc5c86d2a2489d693439e43d6d44a

Which is tied to this issue: https://github.com/readium/readium-css/issues/68

As far as I can tell, using the CSS var was to address this issue: https://github.com/readium/readium-css/issues/68#issuecomment-517169618 (unexpected border) but if performance is taking such a massive hit, itā€™s obviously a bigger issue.

JayPanoz commented 4 years ago

OK I have Chromy errors in master too but w/o any detail/message. My understanding is that a version of Chrome broke something, and I will have to fix those anyway so I will attempt to do it on the PR branch.

danielweck commented 4 years ago

thanks for remembering issue 68 :) ok, i will run a regression test on this epub.

JayPanoz commented 4 years ago

So Chromy as an engine fails to run some scenarios for undefined reasons ā€“ and well error messages are probably overrated so it doesnā€™t throw them.

Iā€™ll switch to puppeteer, which will be better anyway but also means Iā€™ll add expressJS to run a local server and get around local file protocol ā€“ otherwise I have to rewrite tests from scratch.

JayPanoz commented 4 years ago

@danielweck visual regression tests should be fixed in this commit: https://github.com/readium/readium-css/commit/cd484c8324e02d0fdbf9d564d6aa0e98808df715

Note express is now required.

On a related note, stopping server.js is subpar right now.

danielweck commented 4 years ago

awesome. just need to check the back colour regression now

danielweck commented 4 years ago

Sorry, I am cross-posting here (from there: https://github.com/readium/readium-css/issues/68#issuecomment-602201607 ), just in case my screenshots / analysis gets lost in the meandering issue tracker hyperlinks. So here is a copy:


So, I can confirm that fixing (see PR https://github.com/readium/readium-css/pull/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 commented 4 years ago

Thanks!

Opened #81 as a new issue so that we can discuss the handling of ā€œCSS hacksā€ in general.

Will merge this PR later today.

JayPanoz commented 4 years ago

Merged, thanks again!