PDA

View Full Version : [OPEN-575] roweditor: column-drag-drop, resize problems



jeffcrilly
15 Feb 2010, 2:31 PM
http://www.extjs.com/deploy/dev/examples/grid/row-editor.html

I've been integrating the very nice roweditor ux into an app I'm working on, and in the process of "testing for bulletproofness" noticed some issues with the roweditor ux.

Issue A : data-out-of-sync-when-grid-columns-are-reordered

One major (imo) issue is the reordering of columns while the editor is active.
1) go to the demo page - i.e. the official extjs samples (link is above).
2) double click to start editing a row.
3) drag a column, e.g. put salary in front of name.

Notice that the fields in the editor are not updated.

Reproducibility: This appears to be reproducible on the extjs demo page which (afaict) is running 3.1.1.

[edit] a workaround is to disable column drag/drop. Though this behavior is then inconsistent with other places in the app.

[edit] Ideally, the editor (imo) should "cancel" the changes, even if a cell is dirty.
Alternatively, the editor ~could~ prompt the user, but this seems tedious.
My expectation is the user gave up on editing once the reorder task started.

[edit] attached screenshot from extjs demo page


Issue B - row-editor-controls-do-not-refresh-on-resize

Another issue I noticed is when the editor is used in a "resizeable" window.
On resize the editor buttons are not updated.
I worked around this problem by adding a doing a "refreshFields" on the "resize" event.

Reproducibility: I am currently using 3.0 and have not verified this issue w/ 3.1.1.
However a brief scan of the 3.1.1 roweditor code leads me to believe this is an issue w/ 3.1.1 also.

(apologies if this has been covered. I did a search but nothing relevant seemed to pop up.)

jeff

raluca
23 Feb 2010, 8:23 AM
I'm having the same issue. Any chance of getting these issues fixed in the near future?

Jamie Avins
23 Feb 2010, 8:52 AM
It's an open issue, we'll get to them as soon as we can. Note that UX components are typically considered a lower priority than other components.

JimmyInMD
30 Mar 2010, 7:08 AM
Any updates or workarounds for this?

Thanks!

raluca
30 Mar 2010, 7:21 AM
I just disabled the column move option on the grids that use an ux editor. I didn't really need the column move option anyway. I did not looked for another solution.

Bdfy
6 Apr 2010, 2:06 AM
I just disabled the column move option on the grids that use an ux editor. I didn't really need the column move option anyway. I did not looked for another solution.
In 3.1.1 all OK - I dont use 3.2 because this bug.

Dipish
5 May 2010, 2:36 AM
Bdfy (http://www.extjs.com/forum/member.php?85252-Bdfy), Just checked 3.1.1, same thing. Could figure out what's wrong with that...

dead_man
6 May 2010, 6:18 PM
I changed the script and corrected problem with column resize. Tested on Firefox 3.6.3, IE6, Opera 10, Google Chrome 4.1. Please test and inform me.

There is still column movement bug.

Dipish
7 May 2010, 1:04 AM
I don't experience column resize problem, this is working fine. But the fields layout crashes on column move (i.e. reordering columns)

mdmitry
7 May 2010, 6:07 AM
I've done a little investigation on this bug.

In function initFields there's a line: this.removeAll(false); - remove all editors from RowEditor, and it removes all extraCls = 'x-box-item' from all editors (@ ContainerLayout::onRemove)

So extraCls is attached to editors only once during rendering (ContainerLayout::configureItem), but removed on first detach from container.

So my simple & dirty fix would be


initFields: function(){
...
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
...
this.insert(i, ed);
if (typeof(this.layout) == 'object') {
var wrap = ed.wrap ? ed.wrap : ed;
wrap.addClass(this.layout.extraCls);
this.afterRender();
}
}
this.initialized = true;
},


edit: my bug is actually that after reordering columns a couple of times (or doing grid reconfigure()) editor fields mess up.

20415

Dipish
7 May 2010, 6:26 AM
Unfortunately this fix didn't work for me, I have modified RowEditor have form layout (i.e. when I doubleclick on a row, a small box with form and fields with labels appear) and this code didn't change anything.

But thanks for the idea, I'll investigate in that direction!
I feel that it is connected to the fact that something is definitely done only once at render and isn't done on re-layout, but just couldn't figure out what to grab to...

mdmitry
10 May 2010, 6:44 AM
I've updated the code in my previous post, maybe it'll work now.

dead_man
10 May 2010, 9:44 PM
I made an event-method columnMove. It replaces items without removing.


columnMove: function(oldIndex, newIndex) {
if (this.initialized) {
this.stopEditing(false);
var cm = this.grid.getColumnModel(), pm = Ext.layout.ContainerLayout.prototype.parseMargins;
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
var c = cm.getColumnAt(i),
ed = c.getEditor();
if(!ed){
ed = c.displayEditor || new Ext.form.DisplayField();
}
ed.column = c;
if(i == 0){
ed.margins = pm('0 1 2 1');
} else if(i == len - 1){
ed.margins = pm('0 1 2 1');
} else{
ed.margins = pm('0 1 2');
}
if(ed.ownerCt !== this){
ed.on('focus', this.ensureVisible, this);
ed.on('specialkey', this.onKey, this);
}
this.items.items[i] = ed;
}
this.verifyLayout(true);
}
},
But, when RowEditor is in editing mode, some items are render wrong. After grid refreshing, it's looking good.
When RowEditor is not in editing mode, all works good.
There is a full version of modified script in attachment.

dead_man
10 May 2010, 10:18 PM
But, when RowEditor is in editing mode, some items are render wrong. After grid refreshing, it's looking good.
I fixed this bug already. I replace in stopEditing method string

if(saveChanges === false || !this.isValid()){on

if(!saveChanges || saveChanges === false || !this.isValid()){

Dipish
11 May 2010, 3:23 AM
mdmitry, sorry but your code still doesn't fix the issue in my case.

dead_man, thanks, your solution works like charm! But I don't get the trick - what makes is behave properly? Your event code is almost the same as initFields() that is called by refreshFields() method but instead of removing and re-inserting items

this.removeAll(false);
...
this.insert(i, ed); you just do

this.items.items[i] = ed;
Just wanted to make a little suggestment: I think you don't need these lines here:

if(!ed){
ed = c.displayEditor || new Ext.form.DisplayField();
}Because at that moment all the editors have already been created by initFields() on first show.


Here is my modified version of your columnMove(), maybe it will be useful for someone:

columnMove: function(oldIndex, newIndex) {
if (this.initialized) {
this.stopEditing(false);
var cm = this.grid.getColumnModel(), pm = Ext.layout.ContainerLayout.prototype.parseMargins;
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
var c = cm.getColumnAt(i),
ed = c.getEditor();
var dindex = cm.getDataIndex(i);
if(!Ext.isEmpty(dindex) && !this.in_array(dindex, this.hideIndexes) && ed) {
ed.column = c;
if(i == 0){
ed.margins = pm('0 1 2 1');
} else if(i == len - 1){
ed.margins = pm('0 0 2 1');
} else{
ed.margins = pm('0 1 2');
}
if(ed.ownerCt !== this){
ed.on('focus', this.ensureVisible, this);
ed.on('specialkey', this.onKey, this);
}
this.items.items[i] = ed;
}
else
this.items.items[i] = new Ext.form.Hidden(); // just to keep proper indexes
}
this.verifyLayout(true);
}
}A few comments on that. In my column model I got several columns that I do not want to display and/or edit (these may be hidden columns for custom grouping, display-only columns that display a conjunction of several real Store fields etc.) so this is why I perform this check:

if(!Ext.isEmpty(dindex) && !this.in_array(dindex, this.hideIndexes) && ed)But to keep proper accordance between ColumnModel column indexes and editor form fields indexes I insert Hidden form fields for non-editable columns. There may be no editor assigned such column so there is ' && ed' clause at the end of the if statement given above. hideIndexes is just an array of strings representing underlying Store indexes I do not want to show, I added this parameter to RowEditor config. in_array() is just a simple function that checks if a given item exists in an array. It works like the one in PHP.


I also wonder why the fields on my row editor form aren't really reordered after this code is executed. I do not need this behaviour though, but I wanted to figure out how it works.

Thanks again for the solution!

mdmitry
13 May 2010, 4:57 AM
I've made a list of things RowEditor doesn't support:

* moving columns while editing
* moving columns while not editing, if there are editors that need wrapping (like checkbox)
* resizing columns
* changing grid ColumnModel dynamically with reconfigure()
* expanding/collapsing groups, when using GroupingView

The first three can be seen in official example (http://www.extjs.com/deploy/dev/examples/grid/row-editor.html).

Guess I'll have to ditch this fubar component and somehow use EditorGridPanel instead.

Dipish
14 May 2010, 5:00 AM
* moving columns while not editing, if there are editors that need wrapping (like checkbox)
I didn't understand, what do you mean by that?


* resizing columns
My version of RowEditor with layout: form reacts perfectly on column resize, even if RowEditor is active.


* expanding/collapsing groups, when using GroupingView
A use GroupingView with RowEditor and don't seem to have problems with it. What does go wrong in your case?

tigerfoot
14 May 2010, 5:58 AM
There's already a big diff between dead_man & svn version.

With dead_man patch, resize are working, but in the two version : if you put the RowGridEditor inside a window and you resize the window, and after that you close the window, reopen it you finish with an error.

( Try to use a Editor Grid inside the demo desktop in place of standard grid to have the demo)

Tested with 3.2.1 & svn version

mdmitry
14 May 2010, 7:21 AM
* moving columns while not editing, if there are editors that need wrapping (like checkbox)

1. open official example (http://www.extjs.com/deploy/dev/examples/grid/row-editor.html)
2. start editing a row
3. click Cancel
4. move "Active" column somewhere
5. start editing a row again

20513

this only happens with editors that have a wrapping DIV around them (like checkboxes or date/time editors)


* resizing columns

1. open official example (http://www.extjs.com/deploy/dev/examples/grid/row-editor.html)
2. start editing a row
3. resize any column

20512


* expanding/collapsing groups, when using GroupingView

RowEditor doesn't react to expanding/collapsing groups. IMO if a group that contains current row is collapsed, editor should cancel editing (row hides -> editor hides), and if a group above is expanded/collapsed, roweditor should move with the row, not just stay on its current position. I guess this can be fixed externally using events, but it would be much better if it was supported out-of-the-box.

Dipish
15 May 2010, 7:41 AM
RowEditor doesn't react to expanding/collapsing groups. IMO if a group that contains current row is collapsed, editor should cancel editing (row hides -> editor hides), and if a group above is expanded/collapsed, roweditor should move with the row, not just stay on its current position. I guess this can be fixed externally using events, but it would be much better if it was supported out-of-the-box.

Ok I got your point. Since my editor isn't 'attached' to the row, this behaviour is normal for me.

yurc
27 May 2010, 12:02 AM
thank you !!!!!!!!!!!!!

mrusinak
21 Jun 2010, 12:05 PM
I've made a list of things RowEditor doesn't support:
* resizing columns


I've been doing some investigation on this, with 3.2.2, and the problem appears to be a disconnect between (some) BoxComponents and HBoxLayout (and probably VBox too, but I didnt look).

* In HBoxLayout.calculateChildBoxes, it gets each item's width and height by their member value, i.e. "child.width" and "child.height".
* In BoxComponent.setSize, it updates the size on the value of getResizeEl() - which if different than Box itself means that the "width" and "height" members values aren't changed.

This explains the behavior I see when resizing columns with the editor open - the actual column editor components change size correctly, but their left-hand offsets do not move. I've tried overriding HBoxLayout.calculateChildBoxes to use "getHeight()" and "getWidth()":


Ext.override(Ext.layout.HBoxLayout, {
calculateChildBoxes: function(visibleItems, targetSize) {
...
//gather the total flex of all flexed items and the width taken up by fixed width items
for (i = 0; i < visibleCount; i++) {
child = visibleItems[i];
childHeight = child.getHeight();
childWidth = child.getWidth();
...
}
});
Or alternativly overriding BoxComponent's setSize to also update the member values:

Ext.override(Ext.BoxComponent, {
setSize : function(w, h){
...
if(aw !== undefined || ah !== undefined){ // this code is nasty but performs better with floaters
rz = this.getResizeEl();
if(!this.deferHeight && aw !== undefined && ah !== undefined){
rz.setSize(aw, ah);
this.width = aw;
this.height = ah;
}else if(!this.deferHeight && ah !== undefined){
rz.setHeight(ah);
this.height = ah;
}else if(aw !== undefined){
rz.setWidth(aw);
this.width = aw;
}
this.onResize(aw, ah, w, h);
this.fireEvent('resize', this, aw, ah, w, h);
}
}
});
However, while either of those will fix the rowEditor, it winds up breaking some other panels I have. Hope this might help give someone else some insight however.

Edit: I do have a workaround that appears to fix the RowEditor without breaking my other layout - YMMV:

Ext.override(Ext.form.Field, {
setSize : function(w, h){
var ret = Ext.form.Field.superclass.setSize.call(this, w, h);
if (!this.flex) {
if (undefined !== w) { this.width = w; }
if (undefined !== h) { this.height = h; }
}
return ret;
}
});

Edit Edit: Don't break flexing

Dipish
21 Jun 2010, 10:32 PM
mrusinak, thanks for doing the research! I'll take that into account. Though I am feeling to rather cancel using that rowedior for now :)

biggena
21 Jun 2010, 11:49 PM
mrusinak, your solution works like a charm.
Thank you.
Now "resizing columns" issue is fixed.

biggena
22 Jun 2010, 12:07 AM
dead_man, your solution for "columns move" works like a charm.
There is a small non-critical issue, which I found during work with the RowEditor after column move.
Tab key doesn't work as expected.

For example, initially Grid has three columns: "A", "B" and "C"

When you enter into editing of the row, you can start typing text in the first column ("A") and click Tab key to switch focus to the next column ("B", then "C").

Now, let's assume we moved column "A" to the new position between "B" and "C".
So, Grid shows now columns in the following order: "B", "A", "C"

I enter into editing mode and focus is inside column "B".
I click Tab key and expect focus to move to the next column on the right (it is "A" currently).
But instead of that focus is moved to column "C".

This problem is caused by the fact that Tab order is based on how underlying INPUT fields are rendered inside DOM.
Order of INPUT fields is not changed inside columnMove. Only position of them is changed.

It would be great if underlying INPUT fields are reordered as well.

boriss
1 Jul 2010, 5:16 PM
I'm also trying to add support for column moving and resizing to RowEditor. After debugging the code I can confirm what has been found out in this thread so far. I can propose another patch to add support for column moving though. The following two lines have to be added to make the layout manager add extra CSS classes again (like x-box-item) which are removed by the call to removeAll() (as found out by mdmitry):


initFields: function(){
var cm = this.grid.getColumnModel(), pm = Ext.layout.ContainerLayout.prototype.parseMargins;
this.removeAll(false);
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
var c = cm.getColumnAt(i),
ed = c.getEditor();
if(!ed){
ed = c.displayEditor || new Ext.form.DisplayField();
} else if(ed.rendered){
this.getLayout().configureItem(ed);
}
if(i == 0){
ed.margins = pm('0 1 2 1');
} else if(i == len - 1){
ed.margins = pm('0 0 2 1');
} else{
if (Ext.isIE) {
ed.margins = pm('0 0 2 0');
}
else {
ed.margins = pm('0 1 2 0');
}
}
ed.setWidth(cm.getColumnWidth(i));
ed.column = c;
if(ed.ownerCt !== this){
ed.on('focus', this.ensureVisible, this);
ed.on('specialkey', this.onKey, this);
}
this.insert(i, ed);
}
this.initialized = true;
},


Regarding column resizing it seems like that HBoxLayout is the wrong layout manager. As pointed out by mrusinak HBoxLayout checks the BoxComponent's width and height which are read-only values. The width and height of the underlying element can change but the BoxComponent's width and height never do. As I'm reluctant to change the width and height of BoxComponent and don't want to change HBoxLayout either I tried another layout manager: As it turns out resizing columns works reasonably well when ColumnLayout is used. Of course there is another problem then: Moving columns doesn't work anymore. But this needs to be implemented differently anyway if we want to support navigating with the Tab key (as requested by biggena).

If ColumnLayout is used I think the DOM must only be updated in initFields(). The items collection is already updated but the change must be reflected in the DOM, too. How difficult this will be to implement I'll find out after a break. ;)

boriss
2 Jul 2010, 5:39 AM
I managed now to add support for resizing and removing columns. The following changes are required in RowEditor.js:

layout: 'column'
When ColumnLayout is used resizing works automatically. Not only the fields' widths are updated correctly but also their positions.

grid.on({
scope: this,
keydown: this.onGridKey,
columnresize: this.verifyLayout,
columnmove: this.columnMove,
reconfigure: this.refreshFields,
beforedestroy : this.beforedestroy,
destroy : this.destroy,
bodyscroll: {
buffer: 250,
fn: this.positionButtons
}
});
...
columnMove: function(oldIndex, newIndex) {
if(this.initialized){
this.items.insert(newIndex, this.items.removeAt(oldIndex));
var layout = this.getLayout();
var targetEl = layout.innerCt ? layout.innerCt : this.getLayoutTarget();
var node = targetEl.dom.childNodes.item(oldIndex);
var refNode = targetEl.dom.childNodes.item(newIndex);
targetEl.dom.insertBefore(node, refNode);
}
},
This new function does not only rearrange fields in the items collection. It also updates the DOM (which helps to support navigating with the Tab key, too).


initFields: function(){
var cm = this.grid.getColumnModel(), pm = Ext.layout.ContainerLayout.prototype.parseMargins;
this.removeAll(false);
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
var c = cm.getColumnAt(i),
ed = c.getEditor();
if(!ed){
ed = c.displayEditor || new Ext.form.DisplayField({html:'&nbsp;'});
} else if(ed.rendered){
this.getLayout().configureItem(ed);
}
....
As a new layout manager is used the display field must contain something. Otherwise it doesn't have a height and doesn't fill any space in Opera.


verifyLayout: function(force){
if(this.el && (this.isVisible() || force === true)){
var row = this.grid.getView().getRow(this.rowIndex);
this.setSize(Ext.fly(row).getWidth(), Ext.isIE ? Ext.fly(row).getHeight() + 9 : undefined);
var cm = this.grid.colModel, fields = this.items.items;
for(var i = 0, len = cm.getColumnCount(); i < len; i++){
if(!cm.isHidden(i)){
var adjust = 0;
if(i === (len - 1)){
adjust += 3; // outer padding
} else{
adjust += 1;
}
fields[i].show();
fields[i].setWidth(cm.getColumnWidth(i) - adjust);
....
The code above should be removed as ColumnLayout doesn't support padding. If the width is adjusted the fields are not properly aligned.

TODO:
Test in more browsers (I tested IE8 and Opera 10.54)
Test the reconfigure event (I think there is at least a problem with DisplayFields created in initFields(); as false is passed to items.removeAll() no HTML elements are removed; but then as there are no other references to DisplayField objects the HTML elements are never removed)
ColumnLayout does not support margins (it ignores ed.margins set in initFields()); I wonder if margins can be supported differently
When in edit mode and a column is moved the row editor is automatically hidden; I didn't check why (but this is the reason why verifyLayout() isn't called in columnMove() as the row editor is not visible anyway)

boriss
4 Jul 2010, 10:12 AM
Here's a new version of RowEditor with supports column resizing and moving in IE, Firefox, Opera, Chrome and Safari (the test case was http://www.sencha.com/deploy/dev/examples/grid/row-editor.html). If you are interested in the changes I recommend to use a program like Windiff to compare this new version with the one of Ext JS 3.2.1.

meroy
6 Jul 2010, 1:51 PM
Great work on this.

@boriss: I tested your attachment.

The only thing I'm seeing is that there's no spacing between the fields inside the row editor. The spacing was there previously.

This is amazing.

boriss
9 Jul 2010, 3:45 PM
The only thing I'm seeing is that there's no spacing between the fields inside the row editor. The spacing was there previously.
I've updated the RowEditor to include spacing. The problem was that ColumnLayout ignores margins (which are not ignored by BoxLayout). The new layout manager ColumnWithMarginsLayout (defined in ColumnWithMarginsLayout.js in the ZIP file) overrides onLayout() of ColumnLayout and updates margins after fields have been rendered. While this is a small change I'm not sure whether it's a good idea to override onLayout() in ColumnLayout (as there could be side effects I created a new layout manager).

The row editor looks now the very same as before - except in IE. Currently I've no idea why it doesn't work in IE as the development tools indicate that margins have been set for all fields in IE, too. While I can see the correct inline style settings in the debugger the browser seems to ignore them. I also tried Ext.Element.repaint() but it doesn't work. There is no problem in Firefox, Opera, Chrome and Safari. I'm curious if anyone can find out what's the problem here (yes, this is IE8).

By the way, I had actually replaced BoxLayout with ColumnLayout to avoid changing the layout manager. As I had to adapt ColumnLayout now we could of course also adapt BoxLayout. The reason why both layout managers don't work by default is that BoxLayout reads a box width and height (these are constant while the element within the box can have a different width and height - which is the case when columns are resized) and that ColumnLayout ignores margins. It seems to be a smaller and less risky change though to add support for margins to ColumnLayout instead of making the size of boxes variable for BoxLayout (but I'm guessing here).

sumit.madan
21 Aug 2010, 4:04 AM
@boriss

Pretty good, you're close to fixing the RowEditor completely!

Maybe its possible to revert back to BoxLayout to get the margins working in IE?
mrusinak posted a workaround for supporting resizable columns for BoxLayout.

sergiogragera
1 Oct 2010, 5:50 AM
I've created a modified version of RowEditor that solves the problems with column resize and row editor.

To do this, I modified the verifyLayout method RowEditor.js changing this line:

fields[i].setWidth(cm.getColumnWidth(i) - adjust);

By this others:

fields[i].width = cm.getColumnWidth(i) - adjust;
var inputField = document.getElementById(this.items.keys[i]);
if (inputField != null && inputField.style.width)
inputField.style.width = cm.getColumnWidth(i) - adjust;

beshy
26 Mar 2011, 6:56 PM
@boriss
:D