Skip to content

Fix Layout Shifts by managing priorities when enabling styles

Jannik Lehmannrequested to merge
startup-glitches into master

EDIT: After this was merged at 2021-07-22 after some Back & Forth the solution proposed below was not the one that was merged. We found a way to fix the problem without any visual regressions (by not changing the order in which the utility classes are loaded) More details and Demos on this approach here

What does this MR do?

On first load (Clear Cache required) we are having layout shifts. With this MR we are refactoring our basic styles to get rid of this layout shift.

What's wrong here?

First of all, it's worth explaining what is the problem here. The reason we see the elements that are supposed to be hidden is indeed in the fact that application_utilities has not yet been applied to the page. However, the solution to this problem is not only in loading the application_utilities as soon as possible, but there are two conditions:

  • import application BEFORE application_utilities. Otherwise, we will have the situation we're about to fix here permanently: the styles in the application.css will override styles in the application_utilities that ruins the whole point of the utility classes. This is one more reason why StartupCSS is not the right tool here.
  • import both application and application_utilities BEFORE the content is rendered. Due to application_utilties being applied after the content has been output on the screen is the main cause of the issue here.

What's the solution?

After playing with the code and different approaches, we found a very simple yet effective solution. We do see that application.css gets applied before content is rendered on the screen. This is controlled by application.scss itself: by switching the content part of the page into display: block. This assures that application.css is parsed and before the content output.

So, the solution we came to is simply piggybacking this feature and import application_utilities right into application.scss as the very last import. This way we can be sure that both of the conditions above are met:

  • since the import is the last one in the application.scss, the first condition is fulfilled
  • since the application.scss controlls when the content is rendered and makes sure the styles are applied before that, we fulfill the second condition

Wouldn't it harm our LCP?

By reducing the parallelization in the downloading process, indeed we might have harmed the performance a bit. But, most probably by a very tiny margin. First of all, once the application.scss is fetched, it gets cached. So only the first hit on GitLab would be affected. By how much? Well, the result kB are pretty much the same and it's only the difference of 1 vs. 2 requests.

As a bonus 💪

In the course of this change, we have also identified that application_utilities_dark isn't really needed. So the references to it have been removed and we might want to remove the stylesheet itself in the next iteration.

Demo

Note: Visual Changes in the video appear in the top section of the MR:

Visual Changes area Example
Screenshot_2021-06-28_at_13.39.40
before after
before-layout-shift after-layout-shift

Expected Sideeffects

We'll most likely will be dealing with two different sideeffects once this MR will be merged.

pleasant sideeffect

It can be assumed since we are dealing with global stylesheets here, that the Demo Video above is not the only place behaviour like this occurs. Since the application_utilities stylesheet provides the basic foundation for styling our components, other parts of GitLab will benefit from this change and potentially more Layout shifts will be eliminated.

not so pleasant sideeffect

Just as @leipert explained perfectly in this comment !64806 (comment 621228857)

Initially:

The split of application and application utilities was on purpose, because the application utilities are supposed to overwrite the page_specific_styles

Let's imagine you're going to have the following Markup

<div class="foo gl-display-block!">

The loaded styles were coming in as:

// page specific style
.foo {
  display: none !important
}

// utility
.gl-display-block! {
  display: block !important
}

Which had the upside that utility styles were overwriting the page-specific styles, but under certain network conditions, the styles above will cause layout-shifts.

With the changes in this MR the styles will be like

// utility
.gl-display-block! {
  display: block !important
}

// page specific style
.foo {
  display: none !important
}

Which will remove any potential Layout Shifts, but since page-specific styles are no longer overwritten but utility styles, this has the potential to brake existing styles. This is probably a reasonable trade-off and the affected styles will be rather easy to fix, still something to be aware of.

Credit: Thanks to @pslaughter & @dmishunov for pairing up on this MR!

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Jannik Lehmann

Merge request reports