angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.29k stars 6.73k forks source link

uib-tab : deselect callback have wrong index with explicit Tab index set. #6500

Open Rouche opened 7 years ago

Rouche commented 7 years ago

Bug description:

Edit: im unsure for the fix because it breaks some tests. Doc say: "The $selectedIndex can be used to determine which tab was attempted to be opened" If this $selectedIndex is supposed to be the one you assign in the template, then there really is a bug and the tests needs to be fixed too.

Lets say i have explicit tab index set on uib-tabs. I do have ng-if on some of them. Creating "holes" in indexes.

The deselect function sends the wrong index.

Link to minimally-working plunker that reproduces the issue:

None, pretty simple to understand looking at code.

Solution see second post.

Steps to reproduce the issue:

Theres even a test spec that assign index to 1, 3, 5, 7 deselect will then send 0, 1, 2, 3 as $selectedIndex wich is a bit counter intuitive.

<uib-tabset active="ctrl.index">
    <uib-tab index="1" heading="0" deselect="ctrl.selectTab($event, $selectedIndex)">
    </uib-tab>
    <uib-tab index="3" ng-if="false" heading="0" deselect="ctrl.selectTab($event, $selectedIndex)">
    </uib-tab>
    <uib-tab index="5" heading="0" deselect="ctrl.selectTab($event, $selectedIndex)">
    </uib-tab>
</uib-tabset>

Version of Angular, UIBS, and Bootstrap

Angular: 1.6.1

UIBS: 2.5.0

Bootstrap: 3.3.7

Rouche commented 7 years ago

Better fix so that $selectedIndex fits the one you set in template:

There is a workaround for this, but not perfect. So i guess since its a breaking change, i will have to cope with it,

It just does not feel right to receive a different index from the one that is set in the template. Fix:

        previousSelected.tab.onDeselect({
          $event: evt,
          $selectedIndex: typeof index === 'number' ? ctrl.tabs[index].index : undefined <==== Fix
        });

Some tests that verify on deselect needs to be adjusted too:

    it('should call deselect callback on deselect', function() {
      titles().eq(1).find('> a').click();
      expect(scope.deselectFirst).toHaveBeenCalled();
      expect(scope.deselectFirst.calls.argsFor(0)[0].target).toBe(titles().eq(1).find('> a')[0]);
      expect(scope.deselectFirst.calls.argsFor(0)[1]).toBe(2);
      titles().eq(0).find('> a').click();
      expect(scope.deselectSecond).toHaveBeenCalled();
      expect(scope.deselectSecond.calls.argsFor(0)[0].target).toBe(titles().eq(0).find('> a')[0]);
      expect(scope.deselectSecond.calls.argsFor(0)[1]).toBe(1);
      titles().eq(1).find('> a').click();
      expect(scope.deselectFirst.calls.count()).toBe(2);
      expect(scope.deselectFirst.calls.argsFor(1)[0].target).toBe(titles().eq(1).find('> a')[0]);
      expect(scope.deselectFirst.calls.argsFor(1)[1]).toBe(2);
    });
AndrewBoklashko commented 6 years ago

Same problem

Wes-Reid commented 6 years ago

Also encountering this.