Trying to address paper-tabs issues (#1048)

* remove unused properties

* only shows inkBar if we have selected tab position

Doing this avoid, on render where the selected tab is NOT the first tab, to have the inkBar quickly starting from offset 0 to the actual selected tab.

* ensure newOffset is always set

* only have width/left for tab

* reverse shouldPaginate condition

By defaults, assume this is true.

* the newOffset fallback should be its current position

* remove 'didRender' hook in paper-tab

This is unnecessary because we already have the hook
in paper-tabs that:

- is always called after one of the tab didRender
- calls 'updateDimensions' on all of its children

* make it possible for shouldPaginate to toggle value

* it appears to be unnecessary to do the work in didInsert

As it will be done directly after in didRender

* add comment

* remove unnecessary fixOffset in selectedTab observer

Because didRender will be called and will call this method too.

* correct movingRight that may be undefined

Because when navigating there is a render without
any selected tab

* add test for md-left/right on ink bar

* remove unused import

* do not use computed property for _selectedTab

Because we pass 'selected' to each of them, updating
the selected tab will invalidate all the 'isSelected'
property which will trigger an validation of
_selectedTab (thus observer callback) for each nested tab

* correct indentation

* remove unnecessary observer

* use computed property for paginationStyle

* do not try to correct offset for each didRender event

Only do it in two occasions:

- first render
- selected tab was updated

Otherwise it will prevent user's pagination as the selected tab will
become partially hidden.

* ensure 'fixOffsetIfNeeded' is not called against destroyed component

* execute resize event in current runloop

* fix nextPage pagination when the current tab's width is greater than canvas

* fix previousPage when some tabs have bigger width than canvas size

addon/components/paper-tab.js

@@ -41,26 +41,14 @@ export default Component.extend(ChildMixin, RippleMixin, FocusableMixin, {
     }
   },
 
-  didInsertElement() {
-    this._super(...arguments);
-    let width = this.element.offsetWidth;
-    // this is the initial tab width
-    // it is used to calculate if we need pagination or not
-    this.set('width', width);
-  },
-
-  didRender() {
-    this._super(...arguments);
-    this.updateDimensions();
-  },
-
-  // this method is also called by the parent
+  // this method is called by the parent
   updateDimensions() {
-    let left = this.element.offsetLeft;
     // this is the true current width
-    // it is used to calculate the ink bar position
-    let currentWidth = this.element.offsetWidth;
-    this.setProperties({ left, currentWidth });
+    // it is used to calculate the ink bar position & pagination offset
+    this.setProperties({
+      left: this.element.offsetLeft,
+      width: this.element.offsetWidth
+    });
   },
 
   click() {

addon/components/paper-tabs.js

@@ -1,9 +1,9 @@
 import { inject as service } from '@ember/service';
 import { gt } from '@ember/object/computed';
-import { computed, observer } from '@ember/object';
+import { computed } from '@ember/object';
 import Component from '@ember/component';
 import { htmlSafe } from '@ember/string';
-import { scheduleOnce, next } from '@ember/runloop';
+import { scheduleOnce, join } from '@ember/runloop';
 import layout from '../templates/components/paper-tabs';
 import { ParentMixin } from 'ember-composability-tools';
 import ColorMixin from 'ember-paper/mixins/color-mixin';
@@ -20,45 +20,33 @@ export default Component.extend(ParentMixin, ColorMixin, {
 
   selected: 0, // select first tab by default
 
-  _selectedTab: computed('childComponents.@each.isSelected', function() {
-    return this.get('childComponents').findBy('isSelected');
-  }),
-
-  _selectedTabDidChange: observer('_selectedTab', function() {
-    let selectedTab = this.get('_selectedTab');
-
-    let previousSelectedTab = this.get('_previousSelectedTab');
-
-    if (selectedTab === previousSelectedTab) {
-      return;
-    }
-
-    this.setMovingRight();
-
-    this.fixOffsetIfNeeded();
-
-    this.set('_previousSelectedTab', selectedTab);
-  }),
-
   noInkBar: false,
   noInk: false,
   ariaLabel: null,
-  previousInkBarPosition: 0,
   stretch: 'sm',
+  movingRight: true,
 
-  inkBarLeft: computed('_selectedTab.left', function() {
-    return this.get('_selectedTab.left') || 0;
-  }),
+  inkBar: computed('noInkBar', '_selectedTab.{width,left}', 'wrapperWidth', function() {
+    if (this.get('noInkBar')) {
+      return null;
+    }
 
-  inkBarRight: computed('wrapperWidth', '_selectedTab.currentWidth', 'inkBarLeft', function() {
-    return this.get('wrapperWidth') - this.get('inkBarLeft') - (this.get('_selectedTab.currentWidth') || 0);
+    let selectedTab = this.get('_selectedTab');
+    if (!selectedTab || selectedTab.get('left') === undefined) {
+      return null;
+    }
+
+    return {
+      left: selectedTab.get('left'),
+      right: this.get('wrapperWidth') - selectedTab.get('left') - selectedTab.get('width')
+    };
   }),
 
-  tabsWidth: computed('childComponents.@each.width', function() {
-    return this.get('childComponents').reduce((prev, t) => prev + t.get('width'), 0);
+  paginationStyle: computed('currentOffset', function() {
+    return htmlSafe(`transform: translate3d(-${this.get('currentOffset')}px, 0px, 0px);`);
   }),
 
-  shouldPaginate: false,
+  shouldPaginate: true,
 
   shouldCenter: computed('shouldPaginate', 'center', function() {
     return !this.get('shouldPaginate') && this.get('center');
@@ -71,36 +59,48 @@ export default Component.extend(ParentMixin, ColorMixin, {
   didInsertElement() {
     this._super(...arguments);
 
-    let updateCanvasWidth = () => {
-      this.updateDimensions();
-      this.updateStretchTabs();
-      this.fixOffsetIfNeeded();
+    this.updateCanvasWidth = () => {
+      join(() => {
+        this.updateDimensions();
+        this.updateStretchTabs();
+      });
     };
 
-    window.addEventListener('resize', updateCanvasWidth);
-    window.addEventListener('orientationchange', updateCanvasWidth);
-    this.updateCanvasWidth = updateCanvasWidth;
-
-    // trigger updateDimensions to calculate shouldPaginate early on
-    this.updateDimensions();
-    scheduleOnce('afterRender', () => {
-      next(() => {
-        // here the previous and next buttons should already be renderd
-        // and hence the offsets are correctly calculated
-        if (!this.isDestroyed && !this.isDestroying) {
-          this.updateDimensions();
-          this.fixOffsetIfNeeded();
-        }
-      });
-    });
+    window.addEventListener('resize', this.updateCanvasWidth);
+    window.addEventListener('orientationchange', this.updateCanvasWidth);
+
+    scheduleOnce('afterRender', this, this.fixOffsetIfNeeded);
   },
 
   didRender() {
     this._super(...arguments);
     // this makes sure that the tabs react to stretch and center changes
+    // this method is also called whenever one of the tab is re-rendered (content changes)
+    this.updateSelectedTab();
     this.updateCanvasWidth();
   },
 
+  /**
+   * Updates the currently selected tab only once all the <paper-tab> has rendered.
+   *
+   * If we were to use a computed property the observer would get triggered once per
+   * nested <paper-tab> because we pass the 'selected' property to them that will
+   * invalidate their 'isSelected' property.
+   */
+  updateSelectedTab() {
+    let selectedTab = this.get('childComponents').findBy('isSelected');
+    let previousSelectedTab = this.get('_selectedTab');
+
+    if (selectedTab === previousSelectedTab) {
+      return;
+    }
+
+    this.set('movingRight', !selectedTab || !previousSelectedTab || previousSelectedTab.get('left') < selectedTab.get('left'));
+    this.set('_selectedTab', selectedTab);
+
+    scheduleOnce('afterRender', this, this.fixOffsetIfNeeded);
+  },
+
   willDestroyElement() {
     this._super(...arguments);
     window.removeEventListener('resize', this.updateCanvasWidth);
@@ -116,12 +116,11 @@ export default Component.extend(ParentMixin, ColorMixin, {
     }
   },
 
-  setMovingRight() {
-    let movingRight = this.get('_previousSelectedTab.left') < this.get('_selectedTab.left');
-    this.set('movingRight', movingRight);
-  },
-
   fixOffsetIfNeeded() {
+    if (this.isDestroying || this.isDestroyed) {
+      return;
+    }
+
     let canvasWidth = this.get('canvasWidth');
     let currentOffset = this.get('currentOffset');
 
@@ -138,14 +137,15 @@ export default Component.extend(ParentMixin, ColorMixin, {
     } else if (tabLeftOffset < currentOffset) {
       // ensure selectedTab is not partially hidden on the left side
       newOffset = tabLeftOffset;
+    } else {
+      newOffset = currentOffset;
     }
 
     if (newOffset === currentOffset) {
       return;
     }
 
     this.set('currentOffset', newOffset);
-    this.set('paginationStyle', htmlSafe(`transform: translate3d(-${newOffset}px, 0px, 0px);`));
   },
 
   updateDimensions() {
@@ -154,10 +154,7 @@ export default Component.extend(ParentMixin, ColorMixin, {
     this.get('childComponents').invoke('updateDimensions');
     this.set('canvasWidth', canvasWidth);
     this.set('wrapperWidth', wrapperWidth);
-
-    if (wrapperWidth > canvasWidth) {
-      this.set('shouldPaginate', true);
-    }
+    this.set('shouldPaginate', wrapperWidth > canvasWidth);
   },
 
   updateStretchTabs() {
@@ -185,22 +182,25 @@ export default Component.extend(ParentMixin, ColorMixin, {
   actions: {
     previousPage() {
       let tab = this.get('childComponents').find((t) => {
-        return t.get('left') >= this.get('currentOffset');
+        // ensure we are no stuck because of a tab with a width > canvasWidth
+        return (t.get('left') + t.get('width')) >= this.get('currentOffset');
       });
       if (tab) {
         let left = Math.max(0, tab.get('left') - this.get('canvasWidth'));
         this.set('currentOffset', left);
-        this.set('paginationStyle', htmlSafe(`transform: translate3d(-${left}px, 0px, 0px);`));
       }
     },
 
     nextPage() {
       let tab = this.get('childComponents').find((t) => {
-        return t.get('left') + t.get('width') - this.get('currentOffset') > this.get('canvasWidth');
+        // ensure tab's offset is greater than current
+        // otherwise if the tab's width is greater than canvas we cannot paginate through it
+        return t.get('left') > this.get('currentOffset')
+          // paginate until the first partially hidden tab
+          && t.get('left') + t.get('width') - this.get('currentOffset') > this.get('canvasWidth');
       });
       if (tab) {
         this.set('currentOffset', tab.get('left'));
-        this.set('paginationStyle', htmlSafe(`transform: translate3d(-${tab.get('left')}px, 0px, 0px);`));
       }
     },
 

addon/templates/components/paper-tabs.hbs

@@ -20,9 +20,9 @@
         )
       )}}
 
-      {{#unless noInkBar}}
-        {{paper-ink-bar movingRight=movingRight left=inkBarLeft right=inkBarRight}}
-      {{/unless}}
+      {{#if inkBar}}
+        {{paper-ink-bar movingRight=movingRight left=inkBar.left right=inkBar.right}}
+      {{/if}}
 
     </md-pagination-wrapper>
   </md-tabs-canvas>

tests/integration/components/paper-tabs-test.js

@@ -160,6 +160,27 @@ module('Integration | Component | paper tabs', function(hooks) {
     assert.notOk(find('md-ink-bar'));
   });
 
+  test('ink bar has md-left or md-right class', async function(assert) {
+    await render(hbs`
+        {{#paper-tabs as |tabs|}}
+          {{tabs.tab name="Tab one"}}
+          {{tabs.tab name="Tab two"}}
+          {{tabs.tab name="Tab three"}}
+        {{/paper-tabs}}
+      `);
+
+    assert.dom('md-ink-bar').hasClass('md-right');
+
+    await click('.md-tab:nth-child(2)');
+
+    assert.dom('md-ink-bar').hasClass('md-right');
+
+    await click('.md-tab:nth-child(1)');
+
+    assert.dom('md-ink-bar').hasClass('md-left');
+
+  });
+
   test('borderBottom true adds border', async function(assert) {
     await render(hbs`
       {{#paper-tabs borderBottom=true as |tabs|}}