atom / archive-view

Open compressed files in Atom
MIT License
31 stars 32 forks source link

Focus file before modifying DOM to avoid forced reflows #58

Closed 50Wliu closed 6 years ago

50Wliu commented 6 years ago

Requirements

Description of the Change

Focusing an element is considered a "read" operation. Since we were mutating the DOM before selecting the element, the focus call was causing a double forced reflow. By waiting until after the focus call to mutate the DOM, we avoid those forced reflows, which were taking ~220ms combined. While reflows still have to occur, they seem to be more efficient than the forced reflows as seen below. I've also set contain: strict on the Archive View to improve layout performance.

Before After
1484ms forced-reflows 1200ms regular-reflow

Alternate Designs

Don't focus a file on creation. That however is not very user-friendly.

Benefits

Removes a source of forced reflows and minorly improves the time it takes until the Archive View is usable.

Possible Drawbacks

If an exception occurs while creating the tree entries or updating the summary, the loading view will not be removed. Similarly with selecting the first file and actually adding the entries to the DOM.

Applicable Issues

Refs #19

50Wliu commented 6 years ago

@simurai do you know anything about the contain property? Reading MDN it seems like I can use contain: strict here and everything seems to work but I'd like to be sure that it won't horribly crash and burn in some edge cases.

simurai commented 6 years ago

I haven't experimented with contain yet. But since there is an overflow: auto; I think it's ok to use. There shouldn't be anything that can affect something outside of the overflow.

/cc @as-cii that has used contain for the editor.

as-cii commented 6 years ago

I haven't tested this but it would seem okay to use this approach. A couple of questions/comments:

Thanks, @50Wliu!

50Wliu commented 6 years ago

@as-cii yeah I agree that the forced reflows aren't really bad in this case because they happen anyways later on, but even before applying the updated containment rules the profiling was consistently showing 100-200ms improvements after fixing the reflows.

Could an exception occur while creating tree entries?

I'm highly skeptical that this could occur (as mostly all it does is append elements to the DOM), but I forgot to mention that even if it does happen a Notification will be generated. There was no special handling beforehand either, but now we hide the loading message after the tree entries have finished being generated rather than before (which also makes more sense logically).

as-cii commented 6 years ago

Okay this seems good to go. Thanks for your patience on this, @50Wliu! 🚢 at will.