fix: Fixed jank on primary detail when expanding on compact layout (#1754)

* CPFED-4318: Fixing jank in pfe-primary-detail when expanding section in compact mode

* Adding changelog info

* Fixing attribute filter

* Update elements/pfe-primary-detail/src/pfe-primary-detail.js

Co-authored-by: Michael Potter <[email protected]>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michael Potter <[email protected]>

Author: 3 people

CHANGELOG-1.x.md

@@ -1,3 +1,5 @@
+- [](https://github.com/patternfly/patternfly-elements/commit/) fix: Pfe-Primary-Detail - Fixing animation jank when expanding a section in compact mode
+
 # 1.11.0 (2021-08-18)
 
 - [489c561](https://github.com/patternfly/patternfly-elements/commit/489c561fd825bd7f84ebd3c1addbdeac28304a0e) feat: Pfe-Primary-Detail - Added much improved a11y and keyboard navigation, added mobile optimized layout behavior, and added thorough tests using new framework.

elements/pfe-primary-detail/src/pfe-primary-detail.html

@@ -1,13 +1,15 @@
-<div id="details-nav" data-pristine>
-  <slot name="details-nav--header"></slot>
-  <slot name="details-nav"></slot>
-</div>
-<div id="details-wrapper">
-  <div id="details-wrapper__header">
-    <button id="details-wrapper__back"><div id="details-wrapper__heading"></div></button>
+<div id="wrapper" data-pristine>
+  <div id="details-nav">
+    <slot name="details-nav--header"></slot>
+    <slot name="details-nav"></slot>
+  </div>
+  <div id="details-wrapper">
+    <div id="details-wrapper__header">
+      <button id="details-wrapper__back"><div id="details-wrapper__heading"></div></button>
+    </div>
+    <slot name="details"></slot>
+  </div>
+  <div id="details-nav__footer">
+    <slot name="details-nav--footer"></slot>
   </div>
-  <slot name="details"></slot>
-</div>
-<div id="details-nav__footer">
-  <slot name="details-nav--footer"></slot>
 </div>

elements/pfe-primary-detail/src/pfe-primary-detail.js

@@ -1,10 +1,5 @@
 import PFElement from "../../pfelement/dist/pfelement.js";
 
-// Config for mutation observer to see if things change inside of the component
-const lightDomObserverConfig = {
-  childList: true,
-};
-
 /**
  * Debounce helper function
  * @see https://davidwalsh.name/javascript-debounce-function
@@ -135,6 +130,7 @@ class PfePrimaryDetail extends PFElement {
     this._setBreakpoint = this._setBreakpoint.bind(this);
     this._setDetailsNavVisibility = this._setDetailsNavVisibility.bind(this);
     this._updateBackButtonState = this._updateBackButtonState.bind(this);
+    this._manageWrapperAttributes = this._manageWrapperAttributes.bind(this);
 
     // Place to store references to the slotted elements
     this._slots = {
@@ -145,9 +141,11 @@ class PfePrimaryDetail extends PFElement {
     };
 
     // Setup mutation observer to watch for content changes
-    this._observer = new MutationObserver(this._processLightDom);
+    this._childObserver = new MutationObserver(this._processLightDom);
+    this._attributesObserver = new MutationObserver(this._manageWrapperAttributes);
 
     // Commonly used shadow DOM elements
+    this._wrapper = this.shadowRoot.getElementById("wrapper");
     this._detailsNav = this.shadowRoot.getElementById("details-nav");
     this._detailsWrapper = this.shadowRoot.getElementById("details-wrapper");
     this._detailsWrapperHeader = this.shadowRoot.getElementById("details-wrapper__header");
@@ -176,7 +174,8 @@ class PfePrimaryDetail extends PFElement {
     window.addEventListener("resize", this._debouncedSetBreakpoint);
 
     // Process the light DOM on any update
-    this._observer.observe(this, lightDomObserverConfig);
+    this._childObserver.observe(this, { childList: true });
+    this._attributesObserver.observe(this, { attributes: true, attributeFilter: ["active"] });
 
     this._detailsBackButton.addEventListener("click", this.closeAll);
 
@@ -185,7 +184,8 @@ class PfePrimaryDetail extends PFElement {
   }
 
   disconnectedCallback() {
-    this._observer.disconnect();
+    this._childObserver.disconnect();
+    this._attributesObserver.disconnect();
 
     window.removeEventListener("resize", this._debouncedSetBreakpoint);
 
@@ -199,6 +199,14 @@ class PfePrimaryDetail extends PFElement {
     this.removeEventListener("keydown", this._keyboardControls);
   }
 
+  _manageWrapperAttributes() {
+    if (this.hasAttribute("active")) {
+      this._wrapper.classList.add("active");
+    } else {
+      this._wrapper.classList.remove("active");
+    }
+  }
+
   /**
    * Updates markup of details-nav elements to be toggle buttons
    * @param {object} toggle Slotted element (probably a headline, unless it's been initialized already)
@@ -295,7 +303,7 @@ class PfePrimaryDetail extends PFElement {
 
     // If nothing has been touched and we move to mobile, the details nav should be shown,
     // not the item that was opened by default so the desktop design would work
-    if (this._detailsNav.hasAttribute("data-pristine") && breakpointIs === "compact") {
+    if (this._wrapper.hasAttribute("data-pristine") && breakpointIs === "compact") {
       const activeToggle = this.active ? document.getElementById(this.active) : false;
       if (activeToggle) {
         this._addCloseAttributes(activeToggle);
@@ -486,7 +494,7 @@ class PfePrimaryDetail extends PFElement {
     // Detect if handleHideShow was called by an event listener or manually in code
     if (typeof e === "object" && Array.isArray(e.path)) {
       // If the user has interacted with the component remove the pristine attribute
-      this._detailsNav.removeAttribute("data-pristine");
+      this._wrapper.removeAttribute("data-pristine");
     }
 
     if (typeof nextToggle === "undefined") {
@@ -502,7 +510,7 @@ class PfePrimaryDetail extends PFElement {
     const currentToggle = document.getElementById(this.active);
 
     // Update attribute to show which toggle is active
-    this.setAttribute("active", nextToggle.id);
+    this.active = nextToggle.id;
 
     // Create the appropriate heading for the top of the content column
     let newHeading = null;

elements/pfe-primary-detail/src/pfe-primary-detail.scss

@@ -26,43 +26,46 @@ $LOCAL-VARIABLES: (
 }
 
 :host {
-  display: grid;
-  grid-template-columns: pfe-local(GridTemplateColumns);
+  display: block;
   border: pfe-local(Border);
-  align-items: stretch;
-  justify-content: stretch;
   // For compact styling and transitioning from compact to normal styling
   overflow: hidden;
 }
 
-:host([breakpoint='compact']) {
-  grid-template-columns: 1fr;
-}
-
 :host([hidden]) {
   display: none;
 }
 
-#details-nav,
-#details-wrapper,
-#details-nav__footer {
-  ///
-  // @todo [WR] Animation is janky nav item is selected,
-  // disabling overflow: hidden on parent is only fix
-  // I can find but we need that.
-  //
-  // Tried: delaying _setDetailsNavVisibility, changing to flexbox layout,
-  // transitioning position, adding extra wrappers, will-change,
-  // backface-visibility, and 4ish ways of laying this out and animating it
-  ///
+#wrapper {
+  display: grid;
+  grid-template-columns: pfe-local(GridTemplateColumns);
+  align-items: stretch;
+  justify-content: stretch;
+  overflow: hidden;
   transition: transform 0.25s ease-in-out;
   will-change: transform;
   backface-visibility: visible;
 
   @media (prefers-reduced-motion) {
-    transform: none;
+    transition: none;
+    will-change: auto;
   }
 
+  :host([breakpoint='compact']) & {
+    display: grid;
+    grid-template-columns: 50% 50%;
+    gap: 0;
+    width: 200%;
+  }
+
+  :host([breakpoint='compact']) &.active {
+    // When active this column should be off the left side
+    transform: translateX(-50%);
+  }
+}
+
+#details-nav,
+#details-nav__footer {
   :host([breakpoint='compact']) & {
     align-self: stretch;
     justify-self: stretch;
@@ -73,17 +76,9 @@ $LOCAL-VARIABLES: (
   }
 }
 
-// Left column elements
-#details-nav,
 #details-nav__footer {
-
   :host([breakpoint='compact']) & {
-    transform: none;
-  }
-
-  :host([breakpoint='compact'][active]) & {
-    // When active this column should be off the left side
-    transform: translateX(-100%);
+    grid-row: 2;
   }
 }
 
@@ -152,16 +147,11 @@ $LOCAL-VARIABLES: (
   background: pfe-local(Background, $region: details);
 
   :host([breakpoint='compact']) & {
-    // When inactive this should be shoved off the right side of the screen at mobile
-    transform: translateX(100%);
     grid-row: 1 / span 2;
+    grid-column: 2;
     padding-top: 0;
   }
 
-  :host([breakpoint='compact'][active]) & {
-    transform: none;
-  }
-
   // Consistent height is enforced with grid, which will respect the max-height of the tallest thing in the grid area
   :host([consistent-height]) & {
     display: flex;
@@ -205,7 +195,7 @@ $LOCAL-VARIABLES: (
 
 #details-wrapper__back {
   $focus-width: 2px;
-  display: none;
+  display: none; // Prevent from effecting layout at desktop
   position: relative;
   align-self: stretch;
   border: 0;
@@ -222,8 +212,15 @@ $LOCAL-VARIABLES: (
     outline-width: $focus-width !important;
   }
 
-  :host([active]) & {
-    display: inline-block;
+  :host([breakpoint='compact']) & {
+    // Make sure it effects layout at compact
+    // But shouldn't be selectable until it's active
+    display: block;
+    visibility: hidden;
+  }
+
+  :host([breakpoint='compact']) .active & {
+    visibility: visible;
     width: 100%;
     padding-left: 16px;
   }