PDA

View Full Version : [INFOREQ]Menu.remove



James Goddard
25 Feb 2011, 5:38 AM
The remove function on a menu can cause exceptions depending on when it is called. The following code, while obviously not a real world example, demonstrates the issue.


var sub = 1;
Ext.onReady(function() {
var panel = new Ext.create ( 'Ext.panel.Panel', {
renderTo: Ext.getBody(),
width: 600,
height: 350,
frame: true,
dockedItems: {
xtype: 'toolbar',
items: {
text: 'Test Menu',
menu: {
items: {
text: 'Test Item 1',
menu: {
items: [{
text: 'Sub Item 1'
}, {
id: 'remove1',
text: 'Sub Item Remove'
}]
},
listeners: {
activate: function () {
this.menu.remove ('remove' + sub);
this.menu.add ({
id: 'remove' + ++sub,
text: 'Sub Item ' + sub
});
this.menu.doLayout ();
}
}
}
}
}
},
});
});


If you run this and repeatedly open the menu, you will see:

ext-core-debug.js (line 10369) me.dom is undefined

You won't get it every time bug it's not rare.

I've tracked this down and the issue is that the removed item is still in the layoutOnShow of the menu. I'd recommend modifying remove to also remove the item from layoutOnShow.

Animal
25 Feb 2011, 11:16 AM
After fixing it up a bit (4 spaces are better than tabs for readability), and running the below, I get no errors.



var sub = 1;
Ext.onReady(function () {
var panel = new Ext.create('Ext.panel.Panel', {
renderTo: Ext.getBody(),
width: 600,
height: 350,
frame: true,
dockedItems: {
xtype: 'toolbar',
items: {
text: 'Test Menu',
menu: {
items: {
text: 'Test Item 1',
menu: {
items: [{
text: 'Sub Item 1'
}, {
id: 'remove1',
text: 'Sub Item Remove'
}]
},
listeners: {
activate: function () {
this.menu.remove('remove' + sub);
this.menu.add({
id: 'remove' + (sub += 1),
text: 'Sub Item ' + sub
});
}
}
}
}
}
}
});
});

James Goddard
28 Feb 2011, 8:44 AM
After fixing it up a bit (4 spaces are better than tabs for readability), and running the below, I get no errors.

I appears to be "better" now with pr2. However it is still occurring. In one test it took over 100 iterations of showing the menu to occur, in another it was 3 or 4. It seems like you may have to close the menu entirely (by clicking outside) in pr3.

In either case adding:


this.menu.layoutOnShow.removeByKey ('remove' + sub);

after the remove makes it go away, but I think this should happen within ext, as it's really just a workaround to do it outside of the library.

evant
28 Feb 2011, 7:21 PM
I tried for quite a while to reproduce this but was unable to. Can you please post a test case where the error is more apparent?

James Goddard
1 Mar 2011, 5:10 AM
Honestly, I can't understand why it would be so difficult to reproduce. Even using Animals reformatting of the code, both of the shots below were done in less than 20 seconds. With pr1 it occurs probably 4 out of 10 times showing the menu. With pr2 it seems to average about 1 in 40.

At any rate, the code is pretty clear. There are 3 places where a hidden item is added to layoutOnShow (the menu being hidden before the hover). They are only removed in performDeferredLayouts. If the activate event is fired while the item is in layoutOnShow it will fail. As I see it, there are 2 possible fixes:

1. Add a check to performDeferredLayouts so that it doesn't layout the item if item.isDestroyed is true.
2. Add code to remove to additionally remove the item from layoutOnShow.

Personally I think both should be done as it's just common sense that you shouldn't layout a destroyed component and remove should really mean remove, not partially remove.

mattgoldspink
16 Aug 2011, 3:07 AM
I just want to add that we've hit this bug recently in a project (it was using Ext 4.0.1, I'll get them to try with Ext 4.0.5 shortly but we're close to going live). As James says it takes a few tries to hit it, but I seem to be able to get it happen in FF and Chrome after between 3-15 clicks. Our code does:



//addtionalTasksMenu is the Ext.menu.Menu item
addtionalTasksMenu.removeAll();
addtionalTasksMenu.add(addtionalTasksMenuItems);


Note we don't use id's on any of the menu items. The error we get in Chrome is:


Uncaught TypeError: Cannot read property 'style' of undefinedExt.override.setWidth ext-all.js:266
Ext.define.setElementSize ext-all.js:470
Ext.define.setTargetSize ext-all.js:470
Ext.define.onLayout ext-all.js:525
Ext.define.layout ext-all.js:463
Ext.define.doComponentLayout ext-all.js:577
Ext.define.performDeferredLayouts ext-all.js:822
Ext.define.show ext-all.js:821
Ext.define.showBy ext-all.js:2535
Ext.define.showMenu ext-all.js:844
Ext.define.onClick ext-all.js:845

In the debugger I can clearly see there are old menu items in the layoutOnShow collection in the menu:



Ext.getCmp("addtionalTasksMenuItems").items.length3
Ext.getCmp("addtionalTasksMenuItems").layoutOnShow.length
18


Once we try with 4.0.5 I'll update the thread to confirm if it's fixed or not.