Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10727 closed defect (fixed)

ScrollingTabContainer: tab click calls selectChild twice

Reported by: Adam Peller Owned by: bill
Priority: high Milestone: 1.4.2
Component: Dijit Version: 1.4.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

In test_TabContainer, click on any tab to select a new tab. If you put a log in selectChild, you'll see it gets called twice. While it should not be catastrophic, some apps may not be expecting this condition and it can cause regressions. Thanks to Phil Berkland @ IBM for spotting this.

StackController.onButtonClick calls container.selectChild. That publishes an event which triggers onButtonClick a second time via ScrollingTabController.onSelectChild:280

tab.onClick(null);

Change History (7)

comment:1 Changed 10 years ago by bill

Description: modified (diff)
Owner: set to Shane O'Sullivan
Summary: ScrollingTabContainer tab click calls selectChild twiceScrollingTabContainer: tab click calls selectChild twice

Shane, do we need the tab.onClick() calls in ScrollingTabController.onSelectChild()? They don't seem appropriate.

I tried commenting them out and then running:

dijit.byId("mainTabContainer").selectChild(dijit.byId("tab2"))
dijit.byId("mainTabContainer").selectChild(dijit.byId("tab1"))

from firebug, it seems to work.

Obviously the checked (aka selected) state of the TabButton's needs to change, so that the selected tab is white, but that's handled in StackController.onSelectChild().

comment:2 Changed 10 years ago by Shane O'Sullivan

Did you test where one of the tabs was not currently visible, and had to be scrolled to? The reason for that approach is to not show the change in the tab until it is visible.

However, I can see that this is a problem. If it works just fine with those lines commented out, then it should be ok to make that change.

comment:3 Changed 10 years ago by Adam Peller

Milestone: tbd1.4.2

put this in for consideration for 1.4.2, since we've got a couple of other Tab-related things in there already

comment:4 in reply to:  2 Changed 10 years ago by bill

Owner: changed from Shane O'Sullivan to bill
Status: newassigned

Replying to shaneosullivan:

Did you test where one of the tabs was not currently visible, and had to be scrolled to? The reason for that approach is to not show the change in the tab until it is visible.

However, I can see that this is a problem. If it works just fine with those lines commented out, then it should be ok to make that change.

Yes, I tested that, selecting "tab 3" manually and then doing dijit.byId("mainTabContainer").selectChild(dijit.byId("embedded")) but it's hard to see what's happening because it moves so fast.

I see what you are saying about delaying the CSS change until the tab is visible, but actually that isn't working currently because StackController.onSelectChild() changes the CSS of the two tabs instantly, before the animation even starts.

So, I'll remove the onClick calls.

comment:5 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [21355]) Remove tab.onClick() call at end of slide animation, since it's generating an extra selectChild() call (which confuses apps that connect to selectChild(). Fixes #10727 !strict.

comment:6 Changed 10 years ago by bill

(In [21395]) Test file updates to match template changes for ScrollingTabControllerButton widget. Refs #10527.

Also, selecting a pane from the menu no longer focuses on the tab label. I think this is OK since keyboard users will never use the menu anyway, but I needed to remove menu --> tab label focus test. Refs #10727.

comment:7 Changed 10 years ago by bill

(In [21592]) Fix problem after [21355] where selecting a tab from the menu wouldn't actually select a tab. Fixes #10846, refs #10727 !strict.

Note: See TracTickets for help on using tickets.