1355494 - Consider using eslint to report synchronous layout flushes

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Lint and Formatting, enhancement, P4)

Description

[Florian Quèze [:florian]]

Calls to getComputedStyle or getBoundingRect dramatically affect performance as they force a synchronous layout flush.

I wonder if we should make eslint reject new calls to these methods, unless a specific annotation has been placed on the line.

Comment 1

[Dão Gottwald [:dao]]

Tests that count layout flushes during performance-critical operations seem like a better idea to me.

It often comes down to being smarter about when to flush layout and cache the results rather than completely eliminating the call. There are perfectly valid reasons for using getComputedStyle and friends, and imho people shouldn't have to learn some meta language to annotate their valid code. (We have started this anti-pattern with global-scope imports, but last I heard this was going to be temporary.)

Comment 2

[:Gijs (he/him)]

Given that we now have the reflow tests for common operations, and it is relatively straightforward to add new ones, I am tempted to suggest we should wontfix this. I think Dão's reasoning in comment #1 is correct. Florian?

Comment 3

[Florian Quèze [:florian]]

I agree with wontfix because I don't see this going anywhere, I don't think eslint is going to be the right tool here.

Reflow tests only cover the operations we know we care about. The reason why I suggested eslint here is that it would be nice to have something giving a heads up at review time (or even earlier if possible) that some code is going to perform poorly.

I don't fully agree with comment 1, in that I don't think we have valid reasons left to trigger sync flushes in new code. We mostly have bugs we should fix, and some of them take a large amount of effort compared to the expected benefit so I don't seem everything getting fixed soon.

Detecting with eslint when code using getComputedStyle is valid or not would be hard, and I agree that putting annotations everywhere would be a pain. At the time I filed this, I was still expecting that most getBoundingRect calls could be easily changed to getBoundsWithoutFlushing, and that would have been easy to detect with eslint. Now we are moving in a direction where getBoundingRect is OK, but the code should wait for a window.promiseDocumentFlushed callback. This isn't trivial to detect with eslint.