Problem/Motivation

The Olivero mobile nav hamburger animation is less performant than it needs to be. This is most noticeable on Safari, in my testing.

Steps to reproduce

  1. Enable the Olivero theme
  2. Open a page on a small screen or mobile browser, where the hamburger nav menu is visibile
  3. Click/touch to toggle the hamburger menu

Proposed resolution

Change the transition animation property from all to transform. The transform property is the only one animated for the hamburger menu.

Comments

bronzehedwick created an issue. See original summary.

bronzehedwick’s picture

Here's my patch to address.

cilefen’s picture

Category: Bug report » Task
Status: Active » Needs review

Thanks for the patch. As part of a triage I am marking this a task because performance bugs imply backing data, which is not shown, and even then, must meet certain benchmarks.

_utsavsharma’s picture

StatusFileSize
new691 bytes
new1.39 KB

Fixed CCF for #2.
Please review.

andy-blum’s picture

Status: Needs review » Needs work
StatusFileSize
new441.92 KB

If we slow down the animation, we can see that animating only the transform property results in the menu icon jumping up and down.

screen recording of animation

Looking at the code, we're also changing the inset-block-start property, so we need to either allow the transition to apply to that property as well, or account for the vertical movement in the transformation (which would be more performant).

&:before {
    inset-block-start: 0;
    transform: rotate(-45deg);
  }

  &:after {
    inset-block-start: 0;
    transform: rotate(45deg);
  }
bronzehedwick’s picture

Thanks @andy-blum, I missed that!

My second pass at the patch does the following:

  1. Removes inset from the animation, using the performant transform: translate instead to position the bars.
  2. Combines the duplicate before and after rules into a single selector, to reduce code and to make it easier to see what the unique properties are for before and after.
  3. Uses double colon syntax for before and after to indicate they are pseudo elements, as distinct from pseudo selectors, per the W3C standard. See MDN.
  4. Compiles the PCSS file into the corresponding CSS file, per Drupal CSS guidelines.

I've also attached a GIF showing the slowed-down animation after the above is applied.

andy-blum’s picture

StatusFileSize
new150.85 KB

Not to be super nitpicky, but we still have some jumpiness in here. I think this is from the center bar being toggled between border-top: 3px solid var(...) and border-top: 0. We should override just the color of that border so that it doesn't cause layout shift. It might be easier to see if you apply overflow: hidden to the HTML element to prevent the scrollbar from shifting the right-aligned elements.

screen caputre of menu button animation

andy-blum’s picture

StatusFileSize
new79.13 KB

One other thing I hadn't considered until right now is the hamburger icon in the sticky desktop header. We should make sure that one works as identically as possible.

bronzehedwick’s picture

Thanks @andy-blum!

Good call on removing the layout thrashing. I updated the patch to toggle the mobile menu border color instead of removing it when open.

I also adapted the desktop sticky nav to the same transform over inset approach as the mobile nav. The main difference now in the animation is that the desktop sticky menu transitions opacity for the middle line, where the mobile menu does not, due to it being implemented with pseudo-elements.

andy-blum’s picture

Status: Needs work » Needs review
bronzehedwick’s picture

StatusFileSize
new2.75 KB
new1.34 KB

Adding interdiffs. cc @andy-blum.

andy-blum’s picture

StatusFileSize
new7.97 KB
new6.17 KB

I was still seeing some issues in #9. I think I've addressed them now in this patch.

andy-blum’s picture

Assigned: bronzehedwick » Unassigned
_utsavsharma’s picture

StatusFileSize
new870 bytes
new7.97 KB

Fixed CCF for #12.
Please review.

bronzehedwick’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested #14 and I find it's working well. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3329863-14.patch, failed testing. View results

andy-blum’s picture

Patch #14 is passing tests. It may periodically fail unrelated CKEditor5 tests. RTBC per #15.

andy-blum’s picture

Status: Needs work » Reviewed & tested by the community

  • bnjmnm committed 8c8f8b24 on 10.1.x
    Issue #3329863 by bronzehedwick, andy-blum, _utsavsharma: Improve...
bnjmnm’s picture

Version: 10.1.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Fixed
  • I tried several modifications of this approach just to see if I liked it better or it saved LOC or overhead. Nothing I changed was an improvement, so I'm pleased with what has been provided.
  • I ran some performance analysis tools comparing HEAD vs what is in the patch, and this is ~40% less demanding, so a really nice clear improvement. I tried a comparison replacing the use of translateY here with translate3d and there was not enough of a difference to suggest a switch would be benefifical. I ran a few profiles and if anything translate3d was a little more demanding, but even then it was by a microtrivial amount

Tthanks @andy-blum for hopping on a Zoom to walk me through this. This seemed pretty complex but a brief explanation revealed this to be a pretty straghtforward and thoughtful fix.

I'm going to run tests on 10.0.x just in case there are any build tool differences before backporting.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.