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
89 stars 20 forks source link

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

Open danielweck opened 2 years ago

danielweck commented 2 years ago

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 commented 2 years ago

-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 commented 2 years ago

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)

JayPanoz commented 2 months ago

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.

mickael-menu commented 2 months ago

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

JayPanoz commented 2 months ago

@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 commented 2 months ago

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

danielweck commented 2 months ago

Thanks @JayPanoz

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

mickael-menu commented 2 months ago

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).

JayPanoz commented 2 months ago

@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

mickael-menu commented 2 months ago

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

JayPanoz commented 2 months ago

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