readium-css icon indicating copy to clipboard operation
readium-css copied to clipboard

`-webkit-perspective: 1` kills performance in CSS column paginated large HTML documents

Open danielweck opened this issue 3 years ago • 11 comments

Some large HTML documents are totally unusable in Thorium, the jank / lag during user interaction is between 500ms and 1 whole second sometimes. Not just when "turning" pages (which is just horizontal scroll), but also just making text selections with the mouse in the current spread. The performance profiler (Chromium, Electron) shows costly layout recalc but mostly paint operations. After much trial and error, I discovered that -webkit-perspective: 1; on the root HTML element is the culprit, and funnily-enough this is marked as "Fix bug for older Chrome" :) https://github.com/readium/readium-css/blob/583011453612e6f695056ab6c086a2c4f4cac9c0/css/src/modules/ReadiumCSS-pagination.css#L45-L46

danielweck avatar Mar 29 '22 11:03 danielweck

-webkit-perspective: unset; or -webkit-perspective: none; solves this (no need for !important if on the html@style attribute). However I am experiencing mouse hit-testing and x/y coordinate issues when tweaking via Chromium's web inspector, so I will build this in Thorium / r2-navigator-js properly to make sure no regressions are caused.

danielweck avatar Mar 29 '22 11:03 danielweck

Furthermore, as with all alterations of the CSS rendering "stacking context", I must make sure this doesn't break position: fixed; used in some cases (from the top of my head: annotations / highlights, overlay modal dialog positioning)

danielweck avatar Mar 29 '22 11:03 danielweck

So as an educated guess, I should be able to remove this considering how old the version of Chrome targeted by this fix is now.

But I’d like to double-check just in case.

@mickael-menu could you update me on the situation re. Android support? Thx.

JayPanoz avatar Apr 18 '24 08:04 JayPanoz

We support down to Chrome 68 on Android, but I think we might raise this if needed. What's the problematic version?

mickael-menu avatar Apr 18 '24 17:04 mickael-menu

@mickael-menu So that’s an interesting question and a rabbit hole I’ll have to go down into.

The issue is described in CanIUse’s ā€œknown issuesā€ for CSS Multicolumn:

Chrome is reported to incorrectly calculate the container height, often breaks on margins and padding, and can display one pixel of the next column at the bottom of the previous column. Some of these issues can be solved by adding -webkit-perspective: 1; to the column container. This creates a new stacking context for the container, and apparently causes Chrome to (re)calculate column layout.

But is missing a link/source. So I’ll have to dig deeper.

What we can assume is that either the version @ which Daniel started removing the CSS hack is safe, or something else has been forcing Chrome to recalculate the column layout all along and it wasn’t really needed.

JayPanoz avatar Apr 19 '24 06:04 JayPanoz

Alright some background, this issue popped up a couple of weeks after chromium switched to LayoutNG – specifically LayoutNGBlockFragmentation. Did my best to find clues in Chromium bug tracker re. multicolum and that’s all I could find at the moment, via this issue in which Thorium is mentioned.

But it sounds to me that’s more like coincidence than causation at the moment.

Also, as a sidenote:

if a multicol container has flex, grid, or table inside, the entire multicol container will fall back to legacy layout for now.

And it looks like it took them until October 2022 to complete that.

I remember Chromium switched to their initial implementation of fragmentation when they removed CSS regions, so I’ll have to confirm whether it was what was used until LayoutNGBlockFragmentation in early 2022. I’ll also check performance issues/regressions after that.

In any case, sounds a bit too much to ask to bump from v.68 to v.10x

JayPanoz avatar Apr 19 '24 09:04 JayPanoz

Thanks @JayPanoz

Probably related ... I filed this bug report some time ago: https://issues.chromium.org/issues/329478330

danielweck avatar Apr 19 '24 09:04 danielweck

By the way an option could be to add the -webkit-perspective: 1; property directly in the Kotlin toolkit, for older Chrome versions (if we know which one).

mickael-menu avatar Apr 19 '24 18:04 mickael-menu

@danielweck Yeah I can’t say this is surprising to me as I saw quite a significant number of regressions after the switch while scanning all the issues. I’ll probably make another pass to collect unfixed issues that may be impactful.

@mickael-menu as far as I can tell, the complete switch to LayoutNG for columns happened with LayoutNGTableFragmentation, which was v.106.0.5245.0, according to Chromium Dash

JayPanoz avatar Apr 22 '24 08:04 JayPanoz

Thanks, I'll update the Kotlin code if you remove -webkit-perspective!

mickael-menu avatar Apr 22 '24 11:04 mickael-menu

Proposed solution: remove from ReadiumCSS and update the Kotlin code, and document the change.

JayPanoz avatar Apr 22 '24 15:04 JayPanoz