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

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

Merged
merged 7 commits into from
Mar 23, 2020

Conversation

danielweck
Copy link
Member

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 danielweck changed the title FYI: NPM packages updates, consistent multiline CSS selectors (lint) FYI: NPM packages updates, consistent multiline CSS selectors (lint), backgroundColor fix (performance) Mar 13, 2020
@danielweck
Copy link
Member Author

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
Copy link
Collaborator

JayPanoz commented Mar 13, 2020

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: 79a61fc

Which is tied to this issue: #68

As far as I can tell, using the CSS var was to address this issue: #68 (comment) (unexpected border) but if performance is taking such a massive hit, it’s obviously a bigger issue.

@JayPanoz
Copy link
Collaborator

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
Copy link
Member Author

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

@JayPanoz
Copy link
Collaborator

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.

Switched to puppeteer after backstopJS update and had to cheat a little
bit in html test files to keep some consistency with previous
screenshots.

Note: require express be installed.
@JayPanoz
Copy link
Collaborator

@danielweck visual regression tests should be fixed in this commit: cd484c8

Note express is now required.

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

@danielweck
Copy link
Member Author

awesome. just need to check the back colour regression now

@danielweck
Copy link
Member Author

Sorry, I am cross-posting here (from there: #68 (comment) ), 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 #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!

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 JayPanoz merged commit a21018a into master Mar 23, 2020
@JayPanoz
Copy link
Collaborator

Merged, thanks again!

@danielweck danielweck deleted the npm-update-and-multiline-selector-lint branch March 23, 2020 19:27
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

Successfully merging this pull request may close these issues.

2 participants