fix: pfe-tabs set default index to 0 if null provided (#1275)

* fix: Fix patch

* fix: Add more info to the changelog

* fix: Update modal script

* fix: If tabset gets null selectedIndex, use 0

* fix: Revert documentation changes

* fix: Revert modal updates

* fix: Remove package-lock

* fix: Use more explicit null and undefined values

* fix: Add warning message if no tab provided

* fix: Bug fix content-set and tabs

* fix: Fix content set failure

* fix: pfelement has a this.template so the template getter in the accordion and tabs is conflicting

* fix: Remove default 0 on selectedIndex

* fix: Stronger comment

Author: castastrophe

CHANGELOG-1.x.md

@@ -2,7 +2,8 @@
 
 - [f876664](https://github.com/patternfly/patternfly-elements/commit/f876664655894cbd29d610c20b3bdbde31aaed7a) fix: Sass maps missing from compiled assets
 - [e3fd841](https://github.com/patternfly/patternfly-elements/commit/e3fd8414cf380a45a89f4166ad2f9d9125cf8760) fix: Storybook updates required change to knobs add-on import
-- [](https://github.com/patternfly/patternfly-elements/commit/) fix: Remove deprecated method call in pfe-modal, _mapSchemaToSlots
+- [7cbd6fb](https://github.com/patternfly/patternfly-elements/commit/7cbd6fb4f879986dcf3677947ae29fe49268884f) fix: Remove deprecated method call in pfe-modal, _mapSchemaToSlots
+- [](https://github.com/patternfly/patternfly-elements/commit/) fix: Tabset should default to 0 if selectedIndex is set to null
 
 ## 1.1.0 ( 2020-12-22 )
 

elements/pfe-accordion/src/pfe-accordion.js

@@ -78,7 +78,7 @@ class PfeAccordion extends PFElement {
   }
 
   // Each set contains a header and a panel
-  static get template() {
+  static get contentTemplate() {
     return `
     <pfe-accordion-header content-type="header"></pfe-accordion-header>
     <pfe-accordion-panel content-type="panel"></pfe-accordion-panel>

elements/pfe-content-set/src/pfe-content-set.js

@@ -111,25 +111,14 @@ class PfeContentSet extends PFElement {
     return this.parentNode ? this.parentNode.offsetWidth > breakpointValue : window.outerWidth > breakpointValue;
   }
 
-  get tab() {
-    // Check if the appropriate tag exists already
+  get tabs() {
     return this.shadowRoot.querySelector(`${PfeTabs.tag}[visible-at="large"]`);
   }
 
   get accordion() {
-    // Check if the appropriate tag exists already
     return this.shadowRoot.querySelector(`${PfeAccordion.tag}[visible-at="small"]`);
   }
 
-  get displayTemplate() {
-    const template = document.createElement("template");
-
-    // // Set up the template for the sets of content
-    template.innerHTML = this.isTab ? PfeTabs.template : PfeAccordion.template;
-
-    return template;
-  }
-
   constructor() {
     super(PfeContentSet);
 
@@ -203,21 +192,21 @@ class PfeContentSet extends PFElement {
 
   _toggleVisible() {
     if (this.isTab) {
-      if (this.tab) this.tab.removeAttribute("hidden");
+      if (this.tabs) this.tabs.removeAttribute("hidden");
       if (this.accordion) this.accordion.setAttribute("hidden", "");
     } else {
       if (this.accordion) this.accordion.removeAttribute("hidden");
-      if (this.tab) this.tab.setAttribute("hidden", "");
+      if (this.tabs) this.tabs.setAttribute("hidden", "");
     }
   }
 
   _removeNodes(list) {
-    let host = this.isTab ? this.tab : this.accordion;
-
     list.forEach(item => this._removeNode(item));
 
     // Check if the container is empty
-    if (!host.hasChildNodes()) host.setAttribute("hidden", "");
+    [this.tabs, this.accordion].forEach(host => {
+      if (!host.hasChildNodes()) host.setAttribute("hidden", "");
+    });
   }
 
   _findConnection(node, host) {
@@ -240,7 +229,7 @@ class PfeContentSet extends PFElement {
   }
 
   _removeNode(node) {
-    [this.tab, this.accordion].forEach(host => {
+    [this.tabs, this.accordion].forEach(host => {
       const connection = _findConnection(node, host);
       if (connection) host.removeChild(connection);
       // Fire a full rebuild if it can't determine the mapped element
@@ -249,37 +238,41 @@ class PfeContentSet extends PFElement {
   }
 
   _updateNode(node, textContent) {
-    [this.tab, this.accordion].forEach(host => {
+    [this.tabs, this.accordion].forEach(host => {
       const connection = _findConnection(node, host);
       if (connection) connection.textContent = textContent;
       // Fire a full rebuild if it can't determine the mapped element
       else this._build();
     });
   }
 
-  _buildSets(sets) {
+  _buildSets(sets, template) {
     let fragment = document.createDocumentFragment();
 
     for (let i = 0; i < sets.length; i = i + 2) {
       let header = sets[i];
       let panel = sets[i + 1];
-      const template = this.displayTemplate.content.cloneNode(true);
+
+      // Set up the template for the sets of content
+      const wrapper = document.createElement("template");
+      wrapper.innerHTML = template.trim();
+      const templateMarkup = wrapper.content.cloneNode(true);
 
       if (!header) this.warn(`no element found at position ${i} of the light DOM input.`);
       if (!panel) this.warn(`no element found at position ${i + 1} of the light DOM input.`);
 
       if (header && this._isHeader(header) && panel && this._isPanel(panel)) {
         // Capture the line-item from the template
         [header, panel].forEach((region, idx) => {
-          const type = idx === 0 ? "header" : "panel";
+          const section = idx === 0 ? "header" : "panel";
 
-          let piece = template.querySelector(`[content-type="${type}"]`).cloneNode(true);
+          let piece = templateMarkup.querySelector(`[content-type="${section}"]`).cloneNode(true);
 
           const id = region.id || region.getAttribute("pfe-id") || this.randomId;
           const clone = region.cloneNode(true);
 
           // Remove the flag from the clone
-          clone.removeAttribute(`${this.tag}--${type}`);
+          clone.removeAttribute(`${this.tag}--${section}`);
 
           // Append a clone of the region to the template item
           piece.appendChild(clone);
@@ -299,7 +292,8 @@ class PfeContentSet extends PFElement {
 
   _build(addedNodes) {
     // Check if the appropriate tag exists already
-    [this.tab, this.accordion].forEach(host => {
+    [this.tabs, this.accordion].forEach(host => {
+      const template = host.tag === "pfe-tabs" ? PfeTabs.contentTemplate : PfeAccordion.contentTemplate;
       // If no id is present, give it the id from the wrapper
       if (!host.id) host.id = this.id || this.pfeId || this.randomId;
 
@@ -308,12 +302,10 @@ class PfeContentSet extends PFElement {
       // Clear out the content of the host if we're using the full child list
       if (!addedNodes && rawSets) host.innerHTML = "";
 
-      // If sets is not null, build them usin gthe template
+      // If sets is not null, build them using the template
       if (rawSets) {
-        let sets = this._buildSets(rawSets);
-        if (sets) {
-          host.appendChild(sets);
-        }
+        let sets = this._buildSets(rawSets, template);
+        if (sets) host.appendChild(sets);
       }
 
       this._toggleVisible();
@@ -324,8 +316,9 @@ class PfeContentSet extends PFElement {
       // pass the selectedIndex property down from pfe-content-set
       // to pfe-tabs if there is a selectedIndex value that's not 0
       if (this.isTab) {
+        // Pass the selectedIndex down to the tabset
         if (this.selectedIndex) {
-          this.tab.selectedIndex = this.selectedIndex;
+          this.tabs.selectedIndex = this.selectedIndex;
         }
       }
     });

elements/pfe-tabs/src/pfe-tabs.js

@@ -44,7 +44,7 @@ class PfeTabs extends PFElement {
   }
 
   // Each set contains a header and a panel
-  static get template() {
+  static get contentTemplate() {
     return `
       <pfe-tab content-type="header" slot="tab"></pfe-tab>
       <pfe-tab-panel content-type="panel" slot="panel"></pfe-tab-panel>
@@ -68,7 +68,8 @@ class PfeTabs extends PFElement {
         values: ["horizontal", "vertical"]
       },
       // Do not set a default of 0, it causes a the URL history to
-      // be updated on load for every tab
+      // be updated on load for every tab; infinite looping goodness
+      // Seriously, don't set a default here unless you do a rewrite
       selectedIndex: {
         title: "Index of the selected tab",
         type: Number,
@@ -167,15 +168,15 @@ class PfeTabs extends PFElement {
   }
 
   connectedCallback() {
-    super.connectedCallback();
-
-    this.addEventListener("keydown", this._onKeyDown);
-    this.addEventListener("click", this._onClick);
-
     Promise.all([customElements.whenDefined(PfeTab.tag), customElements.whenDefined(PfeTabPanel.tag)]).then(() => {
+      super.connectedCallback();
+
       if (this.hasLightDOM()) this._init();
 
       this._observer.observe(this, TABS_MUTATION_CONFIG);
+
+      this.addEventListener("keydown", this._onKeyDown);
+      this.addEventListener("click", this._onClick);
     });
   }
 
@@ -195,6 +196,7 @@ class PfeTabs extends PFElement {
   }
 
   _selectedIndexHandler(oldVal, newVal) {
+    // Wait until the tab and panels are loaded
     Promise.all([customElements.whenDefined(PfeTab.tag), customElements.whenDefined(PfeTabPanel.tag)]).then(() => {
       this._linkPanels();
       this.selectIndex(newVal);
@@ -212,9 +214,7 @@ class PfeTabs extends PFElement {
   }
 
   select(newTab) {
-    if (!newTab) {
-      return;
-    }
+    if (!newTab) return;
 
     if (newTab.tagName.toLowerCase() !== PfeTab.tag) {
       this.warn(`the tab must be a ${PfeTab.tag} element`);
@@ -231,14 +231,16 @@ class PfeTabs extends PFElement {
     const tabs = this._allTabs();
     const tab = tabs[index];
 
-    if (!tab) {
+    if (tabs.length > 0 && !tab) {
       this.warn(`tab ${_index} does not exist`);
       return;
+    } else if (!tabs && !tab) {
+      // Wait for upgrade?
+      return;
     }
-
-    // @IE11 doesn't support URLSearchParams
-    // https://caniuse.com/#search=urlsearchparams
     if (this.selected && this.tabHistory && this._updateHistory && CAN_USE_URLSEARCHPARAMS) {
+      // @IE11 doesn't support URLSearchParams
+      // https://caniuse.com/#search=urlsearchparams
       // rebuild the url
       const pathname = window.location.pathname;
       const urlParams = new URLSearchParams(window.location.search);
@@ -255,19 +257,22 @@ class PfeTabs extends PFElement {
 
   _init() {
     const tabIndexFromURL = this._getTabIndexFromURL();
+    this._linked = false;
+    this._linkPanels();
+
+    // Force role to be set to tablist
+    if (window.ShadyCSS) this._observer.disconnect();
+
+    this.role = "tablist";
 
     if (tabIndexFromURL > -1) {
       this._setFocus = true;
       this.selectedIndex = tabIndexFromURL;
-    } else if (this.selectedIndex === null) {
-      this.selectedIndex = 0;
     }
 
-    // Force role to be set to tablist
-    this.role = "tablist";
+    if (!this.selectedIndex) this.selectedIndex = 0;
 
-    this._linked = false;
-    this._linkPanels();
+    if (window.ShadyCSS) this._observer.observe(this, TABS_MUTATION_CONFIG);
   }
 
   _linkPanels() {
@@ -303,6 +308,8 @@ class PfeTabs extends PFElement {
   }
 
   _panelForTab(tab) {
+    if (!tab || !tab.controls) return;
+
     return this.querySelector(`#${tab.controls}`);
   }
 
@@ -329,9 +336,13 @@ class PfeTabs extends PFElement {
   }
 
   _getTabIndex(_tab) {
-    const tabs = this._allTabs();
-    const index = tabs.findIndex(tab => tab.id === _tab.id);
-    return index;
+    if (_tab) {
+      const tabs = this._allTabs();
+      return tabs.findIndex(tab => tab.id === _tab.id);
+    } else {
+      this.warn(`No tab was provided to _getTabIndex; required to return the index value.`);
+      return 0;
+    }
   }
 
   reset() {
@@ -343,6 +354,8 @@ class PfeTabs extends PFElement {
   }
 
   _selectTab(newTab) {
+    if (!newTab) return;
+
     this.reset();
 
     const newPanel = this._panelForTab(newTab);
@@ -419,8 +432,12 @@ class PfeTabs extends PFElement {
 
     event.preventDefault();
 
-    this.selectedIndex = this._getTabIndex(newTab) || 0;
-    this._setFocus = true;
+    if (newTab) {
+      this.selectedIndex = this._getTabIndex(newTab);
+      this._setFocus = true;
+    } else {
+      this.warn(`No new tab could be found.`);
+    }
   }
 
   _onClick(event) {
@@ -431,12 +448,11 @@ class PfeTabs extends PFElement {
     if (!foundTab) return;
 
     // Update the selected index to the clicked tab
-    this.selectedIndex = this._getTabIndex(event.currentTarget) || 0;
+    this.selectedIndex = this._getTabIndex(event.currentTarget);
   }
 
   _getTabIndexFromURL() {
     let urlParams;
-    let tabIndex = -1;
 
     // @IE11 doesn't support URLSearchParams
     // https://caniuse.com/#search=urlsearchparams
@@ -453,11 +469,11 @@ class PfeTabs extends PFElement {
 
       if (urlParams && tabsetInUrl) {
         let id = urlParams.get(`${this.id}`) || urlParams.get(`pfe-${this.id}`); // remove this condition when it's no longer used in production
-        tabIndex = this._allTabs().findIndex(tab => tab.id === id);
+        return this._allTabs().findIndex(tab => tab.id === id);
       }
     }
 
-    return tabIndex;
+    return -1;
   }
 
   _popstateEventHandler() {