General codebase issues with multiple patterns being followed for the same functionality

Describe the current behavior

This is the first time I'm using Dawn theme. I need to modify it heavily. I was happy to see that Dawn uses Web Components! Seems like there are many different teams and developers working on Dawn as Shopify has grown really big recently.

It would be really useful to establish certain software development patterns so that they are followed throughout the codebase.

Describe the expected behavior

For example, I've already mentioned in another issue that Dawn uses pub/sub pattern, but only in a handful of places. Would be great if this was used more or if this wasn't used at all as there are other ways to achieve the same / similar functionality.

One issue that I hit today and spent about an hour debugging is that my HeaderDrawer component drawer wasn't being properly closed. There have been many issues with this functionality: https://github.com/Shopify/dawn/issues/2498 https://github.com/Shopify/dawn/pull/2472 https://github.com/Shopify/dawn/pull/1242 https://github.com/Shopify/dawn/pull/1171

I believe that every drawer, popover, accordion, etc. should have "open" and "close" methods that open/close the element no matter what and that contain all the logic for properly opening/closing given element.

I looked into MenuDrawer and HeaderDrawer code and, unfortunately, this is not the case:

class MenuDrawer extends HTMLElement {
  constructor() {
    super();

    this.mainDetailsToggle = this.querySelector('details');

    this.addEventListener('keyup', this.onKeyUp.bind(this));
    this.addEventListener('focusout', this.onFocusOut.bind(this));
    this.bindEvents();
  }

  bindEvents() {
    this.querySelectorAll('summary').forEach((summary) =>
      summary.addEventListener('click', this.onSummaryClick.bind(this))
    );
    this.querySelectorAll('button:not(.localization-selector)').forEach((button) =>
      button.addEventListener('click', this.onCloseButtonClick.bind(this))
    );
  }

  onKeyUp(event) {
    if (event.code.toUpperCase() !== 'ESCAPE') return;

    const openDetailsElement = event.target.closest('details[open]');
    if (!openDetailsElement) return;

    openDetailsElement === this.mainDetailsToggle
      ? this.closeMenuDrawer(event, this.mainDetailsToggle.querySelector('summary'))
      : this.closeSubmenu(openDetailsElement);
  }

  onSummaryClick(event) {
    const summaryElement = event.currentTarget;
    const detailsElement = summaryElement.parentNode;
    const parentMenuElement = detailsElement.closest('.has-submenu');
    const isOpen = detailsElement.hasAttribute('open');
    const reducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)');

    function addTrapFocus() {
      trapFocus(summaryElement.nextElementSibling, detailsElement.querySelector('button'));
      summaryElement.nextElementSibling.removeEventListener('transitionend', addTrapFocus);
    }

    if (detailsElement === this.mainDetailsToggle) {
      if (isOpen) event.preventDefault();
      isOpen ? this.closeMenuDrawer(event, summaryElement) : this.openMenuDrawer(summaryElement);

      if (window.matchMedia('(max-width: 990px)')) {
        document.documentElement.style.setProperty('--viewport-height', `${window.innerHeight}px`);
      }
    } else {
      setTimeout(() => {
        detailsElement.classList.add('menu-opening');
        summaryElement.setAttribute('aria-expanded', true);
        parentMenuElement && parentMenuElement.classList.add('submenu-open');
        !reducedMotion || reducedMotion.matches
          ? addTrapFocus()
          : summaryElement.nextElementSibling.addEventListener('transitionend', addTrapFocus);
      }, 100);
    }
  }

  openMenuDrawer(summaryElement) {
    setTimeout(() => {
      this.mainDetailsToggle.classList.add('menu-opening');
    });
    summaryElement.setAttribute('aria-expanded', true);
    trapFocus(this.mainDetailsToggle, summaryElement);
    document.body.classList.add(`overflow-hidden-${this.dataset.breakpoint}`);
  }

  closeMenuDrawer(event, elementToFocus = false) {
    if (event === undefined) return;

    this.mainDetailsToggle.classList.remove('menu-opening');
    this.mainDetailsToggle.querySelectorAll('details').forEach((details) => {
      details.removeAttribute('open');
      details.classList.remove('menu-opening');
    });
    this.mainDetailsToggle.querySelectorAll('.submenu-open').forEach((submenu) => {
      submenu.classList.remove('submenu-open');
    });
    document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);
    removeTrapFocus(elementToFocus);
    this.closeAnimation(this.mainDetailsToggle);

    if (event instanceof KeyboardEvent) elementToFocus?.setAttribute('aria-expanded', false);
  }

  onFocusOut() {
    setTimeout(() => {
      if (this.mainDetailsToggle.hasAttribute('open') && !this.mainDetailsToggle.contains(document.activeElement))
        this.closeMenuDrawer();
    });
  }

  onCloseButtonClick(event) {
    const detailsElement = event.currentTarget.closest('details');
    this.closeSubmenu(detailsElement);
  }

  closeSubmenu(detailsElement) {
    const parentMenuElement = detailsElement.closest('.submenu-open');
    parentMenuElement && parentMenuElement.classList.remove('submenu-open');
    detailsElement.classList.remove('menu-opening');
    detailsElement.querySelector('summary').setAttribute('aria-expanded', false);
    removeTrapFocus(detailsElement.querySelector('summary'));
    this.closeAnimation(detailsElement);
  }

  closeAnimation(detailsElement) {
    let animationStart;

    const handleAnimation = (time) => {
      if (animationStart === undefined) {
        animationStart = time;
      }

      const elapsedTime = time - animationStart;

      if (elapsedTime < 400) {
        window.requestAnimationFrame(handleAnimation);
      } else {
        detailsElement.removeAttribute('open');
        if (detailsElement.closest('details[open]')) {
          trapFocus(detailsElement.closest('details[open]'), detailsElement.querySelector('summary'));
        }
      }
    };

    window.requestAnimationFrame(handleAnimation);
  }
}

customElements.define('menu-drawer', MenuDrawer);

class HeaderDrawer extends MenuDrawer {
  constructor() {
    super();
  }

  openMenuDrawer(summaryElement) {
    this.header = this.header || document.querySelector('.section-header');
    this.borderOffset =
      this.borderOffset || this.closest('.header-wrapper').classList.contains('header-wrapper--border-bottom') ? 1 : 0;
    document.documentElement.style.setProperty(
      '--header-bottom-position',
      `${parseInt(this.header.getBoundingClientRect().bottom - this.borderOffset)}px`
    );
    this.header.classList.add('menu-open');

    setTimeout(() => {
      this.mainDetailsToggle.classList.add('menu-opening');
    });

    summaryElement.setAttribute('aria-expanded', true);
    window.addEventListener('resize', this.onResize);
    trapFocus(this.mainDetailsToggle, summaryElement);
    document.body.classList.add(`overflow-hidden-${this.dataset.breakpoint}`);
  }

  closeMenuDrawer(event, elementToFocus) {
    if (!elementToFocus) return;
    super.closeMenuDrawer(event, elementToFocus);
    this.header.classList.remove('menu-open');
    window.removeEventListener('resize', this.onResize);
  }

  onResize = () => {
    this.header &&
      document.documentElement.style.setProperty(
        '--header-bottom-position',
        `${parseInt(this.header.getBoundingClientRect().bottom - this.borderOffset)}px`
      );
    document.documentElement.style.setProperty('--viewport-height', `${window.innerHeight}px`);
  };
}

customElements.define('header-drawer', HeaderDrawer);

openMenuDrawer(summaryElement) requires summary element to be passed-in. I don't think that some other object, calling openMenuDrawer from the "outside" of MenuDrawer class, should have to pass summaryElement in order to open the drawer. Even worse, closeMenuDrawer(event, elementToFocus = false) requires event element to be passed in as otherwise the function will immediately return.

Here's a relevant bit from MenuDrawer:

class MenuDrawer extends HTMLElement {
  // ...

  closeMenuDrawer(event, elementToFocus = false) {
    if (event === undefined) return;

    // ...
  }

  // ...
}

I wanted to add "X" close icon and this is how I had to add it:

class HeaderDrawer extends MenuDrawer {
  constructor() {
    super();

    this.querySelector("#menu-drawer .icon-close").addEventListener("click", () => this.querySelector("summary").click());
  }

  // ...
}

This doesn't seem like the way to go.

This doesn't work:

this.querySelector("#menu-drawer .icon-close").addEventListener("click", this.closeMenuDrawer.bind(this));

Why? Because of this line:

https://github.com/Shopify/dawn/blob/main/assets/global.js#L498

class HeaderDrawer extends MenuDrawer {
  // ...
  closeMenuDrawer(event, elementToFocus) {
    if (!elementToFocus) return;
    // ...
  }
  // ...
}

It was added in this commit by @eugenekasimov: https://github.com/Shopify/dawn/commit/60b86d37a2201045fa9b19cb3ab50ae21a47af6a

It kinda makes sense, but perhaps we shouldn't return. We could wrap elementToFocus.classList.remove(drawer-open); in an if-statement.

We could wrap/rename/refactor closeMenuDrawer in both MenuDrawer and HeaderDrawer into onCloseMenuDrawer (since they both require an event, hence the on prefix), which calls closeMenuDrawer which always closes the drawer and can be called programmatically like this: menuDrawer.closeMenuDrawer() 😍

It would allow us to call it exactly like this: https://github.com/Shopify/dawn/blob/main/assets/global.js#L428

class MenuDrawer extends HTMLElement {
  // ...

  onFocusOut() {
    setTimeout(() => {
      if (this.mainDetailsToggle.hasAttribute('open') && !this.mainDetailsToggle.contains(document.activeElement))
        this.closeMenuDrawer();
    });
  }

  // ...
}

BTW This is a bug, this line of code does nothing as closeMenuDrawer(), when called with no arguments, returns immediately. This whole function does nothing. It can either be removed or someone should figure out why it was needed in the first place. It seems to have been added by "Chris Berthe" in the initial git commit. So it must be somewhere in the original/private Dawn git repo history.

Why does this bug happen: https://github.com/Shopify/dawn/issues/2498

Menu drawer overlay does not disappear until "Save" action in the editor

I'd bet it's because we do opening/closing logic outside of drawer classes here: https://github.com/Shopify/dawn/blob/main/assets/global.js#L9

document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => {
  summary.setAttribute('role', 'button');
  summary.setAttribute('aria-expanded', summary.parentNode.hasAttribute('open'));

  if (summary.nextElementSibling.getAttribute('id')) {
    summary.setAttribute('aria-controls', summary.nextElementSibling.id);
  }

  summary.addEventListener('click', (event) => {
    event.currentTarget.setAttribute('aria-expanded', !event.currentTarget.closest('details').hasAttribute('open'));
  });

  if (summary.closest('header-drawer, menu-drawer')) return;
  summary.parentElement.addEventListener('keyup', onKeyUpEscape);
});

The bug happens because in Theme Editor, if the menu type loaded is not drawer, but merchant switches to drawer, it will load drawer menu, but it won't query again: document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => { and therefore it doesn't properly close the drawer.

Here's the bug I'm facing, reproduced on Dawn 9.0.0 installed from the Theme Store (every time the drawer closes when I'm not clicking, it's closing because I press the ESC key - that's where the bug happens, drawer doesn't close properly): https://user-images.githubusercontent.com/67437/232163253-7707ab9b-83f0-45f1-865d-72895d42e8dc.mov

Version information (Dawn, browsers and operating systems)

Dawn Version: 9.0.0
Chrome Version 108.0.5359.124
macOS Version 13.2.1

Possible solution

I'd strongly suggest document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => { to be removed/refactored ASAP. I suggest creating Details or ExpandableDetails component and then MenuDrawer could extend it.

I'd also suggest moving any non globally needed function definitions inside an anonymous function or perhaps global Shopify or Dawn object. Currently all functions defined at the root of global.js (and any other .js file) are globally available and can conflict if apps or anything else adding functions to global scope.

Shopify / dawn