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

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

Open
danielweck opened this issue Mar 29, 2022 · 11 comments
Labels
Proposed solution Issues or feature requests with a solution in reach

Comments

@danielweck
Copy link
Member

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" :)

/* Fix bug for older Chrome */
-webkit-perspective: 1;

@danielweck
Copy link
Member Author

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

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

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

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

@JayPanoz
Copy link
Collaborator

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

JayPanoz commented Apr 19, 2024

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

Thanks @JayPanoz

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

@mickael-menu
Copy link
Member

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

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

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

@JayPanoz
Copy link
Collaborator

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

@JayPanoz JayPanoz added the Proposed solution Issues or feature requests with a solution in reach label Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposed solution Issues or feature requests with a solution in reach
Projects
None yet
Development

No branches or pull requests

3 participants