Ticket #2528595 (closed defect)

Reporter


Satyam
Opened: 10/30/09
Last modified: 05/5/10
Status: closed
Type: defect
Resolution: fixed

Owner


Eric Gelinas
Target Release: 2.8.1
Priority: P3 (normal)
Summary: Row expansion example has bad manners
Description:

The example at:

http://developer.yahoo.com/yui/examples/datatable/dt_rowexp_basic.html

is augmenting the prototype of DataTable with a series of methods and properties instead of creating its own class inheriting from DataTable.

This is a bad example because it is showing a bad practice, which becomes painfully evident in method initAttributes.

When it calls its superclass version, what it is actually calling is Element's own initAttributes. This means DataTable's initAttributes is not called at all. The resulting DataTable has no
configuration attributes except those set by Element and by this extension. It is easy to see by looking at myDatatable._configs, it is almost empty.

I haven't checked for other methods it might be redefining, but from initAttributes on, any function that might require getting or setting a particular configuration attribute will fail badly.

Type: defect Observed in Version: 2.8.0
Component: DataTable Severity: S3 (normal)
Assigned To: Eric Gelinas Target Release: 2.8.1
Location: Example Priority: P3 (normal)
Tags: Relates To:
Browsers: IE 7.x
URL: http://gist.github.com/225100
Test Information:

Change History

Christian Tiberg

Posted: 11/3/09
  • testurl changed to http://gist.github.com/225100

Jim Simpson

Posted: 11/3/09
  • browser changed from N/A to IE 7.x

I am attempting to use conditional row coloring http://developer.yahoo.com/yui/examples/datatable/dt_row_coloring.html with the http://developer.yahoo.com/yui/examples/datatable/dt_rowexp_basic.html example and am discovering that the 'mark' class which is added via the row coloring example, never adds the class. I believe this to be due to the fact that the 'Row expansion example has bad manners'.

Satyam

YUI Contributor

Posted: 11/3/09

The test URL that Christian entered is actually the corrected code for this example.

http://gist.github.com/225100

Jim Simpson

Posted: 11/3/09

If {scrollable:true} is passed to the constructor for ExpandableDataTable a javascript error [ Error: 'this._events' is null or not an object. ] will be realized.
<pre>
// fails
this.myDataTable = new YAHOO.widget.ExpandableDataTable('dtcontainer',
myColumnDefs,
this.myDataSource,
{initialRequest:"",
formatRow: myRowFormatter,
scrollable: true,
width: '100%',
rowExpansionTemplate: '<img src="http://aegis/jsimpson/st3b.old/images/tim.jpg" />'
}
);
</pre>

if {scrollable:true} is commented out, the function is successful

<pre>
//passes
this.myDataTable = new YAHOO.widget.ExpandableDataTable('dtcontainer',
myColumnDefs,
this.myDataSource,
{initialRequest:"",
formatRow: myRowFormatter,
// scrollable: true,
width: '100%',
rowExpansionTemplate: '<img src="http://aegis/jsimpson/st3b.old/images/tim.jpg" />'
}
);
</pre>

Jenny Donnelly

YUI Developer

Posted: 11/12/09

Eric Gelinas

YUI Contributor

Posted: 11/19/09
  • location changed from Example to Website
  • milestone changed to 2.NEXT
  • status changed from assigned to accepted

It looks like this issue is caused by the way that my specific example is hooking this behavior into the DataTable. Some users have suggested other solutions for connecting this behavior like, for instance, wrapping the DataTable. I think this is a good solution. I am going to remove the code that actually augments the DataTable and let implementers decide which approach works best for them. Look for a update to the example soon.

Luke Smith

YUI Contributor

Posted: 12/3/09
  • status changed from accepted to checkedin

The example fix has been applied. It will show on the YDN site at the latest with the next release.

Daniel Kirkdorffer

Posted: 12/4/09

The Javascript is still problematic because it doesn't seem to support paginated datatables very well, which have their own indexing per page that my tests appear to not be taken into account.

Daniel Kirkdorffer

Posted: 12/4/09

Let me better explain my last comment:

I'm trying to implement a variation of row expansion which auto-collapses previously expanded rows when a new row is expanded. I'd want this to work across pages.

I think the problem is with the way a_rowExpansions is managed. With pagination you need to consider record offsets and rows per page when setting state. This may just be my own problem, so when I get back to work Friday morning I'll validate the changes in http://gist.github.com/225100 work without my adjustments to try and collapseAllRows prior to a new row expansion.

Daniel Kirkdorffer

Posted: 12/4/09

Ok. Just using the changes as listed at http://gist.github.com/225100 but introducing pagination and behavior when expanding rows on different pages is bizarre. Some expansions are retained, some aren't, and state flags fall out of sync. Is this something that a row expansion example can code for regardless of whether pagination is in use, or would the introduction of pagination necessarily require a pagination specific example?

Thanks in advance.

Luke Smith

YUI Contributor

Posted: 12/4/09

Daniel,
You're right that the code doesn't properly handle pagination. This might be better tracked as a separate bug or feature request.

Eric, it seems like the code is structured in a way that suggests that it is a viable extension to DataTable (which it nearly is) rather than setting up a local solution for a specific use case. Adding pagination support might be non-trivial. How do you want to handle this?

Daniel Kirkdorffer

Posted: 12/7/09

Luke, you're right in saying this is non-trivial, which is why hopefully you guys can present a solution. I'm relatively new to working with YUI components, but I have found that with most uses of a DataTable I need to paginate results to limit scrolling, so I'm hopefully there is a way to make a tasty sandwich of this peanut butter and jelly.

Daniel Kirkdorffer

Posted: 12/9/09

I think I may have come up with an acceptable fix that deals with pagination and sorting complications. The following two methods simply gives a hook to reset the state of the row expansions prior to changing a page or sorting the table:

{ { {
/**
* Overridden method that gives us the ability to reset the state
* of the display prior to changing the displayed page if pagination
* has been implemented.
* @method doBeforePaginatorChange
* @param {Object} oPaginatorState An object literal describing the proposed pagination state
* @return {Boolean} Return true to continue changing Paginator value.
**/
doBeforePaginatorChange : function( oPaginatorState ){
this.collapseAllRows();
return true;
},

/**
* Overridden method that gives us the ability to reset the state
* of the display prior to sorting.
* @method doBeforeSortColumn
* @param {YAHOO.widget.Column} oColumn Column instance.
* @param {String} sSortDir YAHOO.widget.DataTable.CLASS_ASC or YAHOO.widget.DataTable.CLASS_DESC.
* @return {Boolean} Return true to continue sorting Column.
**/
doBeforeSortColumn : function( oColumn , sSortDir ){
this.collapseAllRows();
return true;
}

In my suggested implementation, I'm just using those hooks to call the collapseAllRows() method which will ensure we have no expanded rows and the a_rowExpansions array is correctly reset.

I think this an acceptable side effect to a user for having changed a page or resorted the display. At the very least it eliminates bizarre behavior that would otherwise come of doing either action on an ExpandableDataTable with expanded rows.

Another enhancement would be to support the expansion of one row at a time (i.e. collapse expanded rows before expanding the next).

To get that to work I created a new method:


/**
* Toggles the expansion state of a row, collapsing any
* other expanded row if expanding a new row.
* @method toggleSingleRowExpansion
* @param {Mixed} record_id Record / Row / or Index id
* @type mixed
**/
toggleSingleRowExpansion : function( record_id ){
var row_data = this.getRecord( record_id );

// row_data is null if row is expanded and not last
// row, and last row is selected to be expanded
if (row_data != null) {
var state = this._getRecordState( record_id );

if( state &amp;&amp; state.expanded ){
this.collapseRow( record_id );
} else {
// This ensures only one row is
// expanded at a time
this.collapseAllRows();
this.expandRow( record_id );
}
} else {
// This ensures only one row is
// expanded at a time
this.collapseAllRows();
this.expandRow( record_id );
}
},

/**
* Abstract method which toggles row expansion for subscribing to
* the DataTable cellClickEvent.
* @method onEventToggleRowExpansion
* @param {Object} oArgs context of a subscribed event
* @type mixed
**/
onEventToggleSingleRowExpansion : function( oArgs ){
if(YAHOO.util.Dom.hasClass(oArgs.target,
'yui-dt-expandablerow-trigger')){
this.toggleSingleRowExpansion( oArgs.target );
}
},

I think this behavior is useful.

Hope that's helpful.

John Lindal

YUI Contributor

Posted: 03/19/10

I've integrated Daniel's patches for pagination and sorting into the code. I also added a fix for column resizing in IE. Hopefully, the updates will be pushed to YDN soon.

Satyam

YUI Contributor

Posted: 03/21/10

There is a very basic problem with this example and it is that you cannot add rows without corresponding records. DataTable assumes there is a one to one correspondence in between rows and records and uses that to go from one to the other.

There are 17 occurrences of table row attribute sectionRowIndex in datatable.js and they all go wrong if rows are added. It is generally assumed that the sectionRowIndex of a row can be used directly as the index into the arrays or records in the recordset.

The reverse relationship is also true, though it is harder to quantify but it is certainly less of a problem. To go from a record to the row, the id of the record can be used with getElementById so that makes it easier, however, there are a few instances where that implicit one-to-one relation in between indexes is assumed.

This approach cannot work. I know, I tried and gave up when I did a count on those sectionRowIndex. I kept fixing one, then another as I kept finding them until I took a step back and counted how many remained, then I said, no way, I'll never manage to fix them all.

There are two possible solutions to this: Either the inserted elements are made to float on top of a gap made in the actual rows, without being part of them or extra records are added for each row. Such records need a special field so that they will be detected by the row formatter to treat them differently and so the cell formatters are canceled and also by the column sort so they are kept always after their master record. I didn't try this second approach so I can't say what else needs to be done. I've done the first one, float the stuff on top of an enlarged row. It works with client-side sorting and paging (I didn't try server-side but I assume it should): http://satyam.com.ar/yui/2.8.0/nested1.html

Eric Gelinas

YUI Contributor

Posted: 03/21/10

This example was originally designed to always assume that the expanded rows would be wiped out and restored when the datatable paginates or sorts. This is why we keep the current open state. If the restore method is called and the state shows it is open the row will be re-created. The implementer can call this restore method with an event after the table is rendered. This was our way of keeping this code as a simple accessory for the YUI2 datatable widget until something could be added under the hood. The mistake was trying to prototype the methods into the datatable widget in the example, I think that confused a lot of people. This is not an addition to the widget, it is an accessory. I completely agree that this can not work along with pagination. We also can not expect the rows to be available through the datatable API. In my conversations with Jenny, she agreed that there would need to be a larger change in how the rows are rendered by the datatable to have row expansion as a proper feature which would be supported by the API.

John Lindal

YUI Contributor

Posted: 03/22/10

Satyam,

First, the code on YDN (http://developer.yahoo.com/yui/examples/datatable/dt_rowexp_basic.html) is very old, so there is no point in complaining about that :)

Second, you have not provided any concrete example of how the widget fails. My patches (which will hopefully be up on YDN soon), fix pagination, sorting, IE6 errors, etc. Once YDN has been updated, please provide concrete examples of how the widget fails, so we can try to address real issues. Vague discussions about sectionRowIndex do not help us improve the example. We need to hear: "I did W,X,Y and I got the result Z, but I think the correct result should be A."

John Lindal

YUI Contributor

Posted: 03/22/10

P.S. It's certainly possible that some particular failure case may convince us to abandon this extension to YUI DataTable, but I haven't heard anything yet that makes me want to give up.

Satyam

YUI Contributor

Posted: 03/24/10

I searched for sectionRowIndex and picked the easiest DataTable method I could test:

http://www.satyam.com.ar/yui/2.8.0/rowExpandNextTRtest.html

I copied expandtable.js from the gist above. I assume that to be the latest, otherwise, you can run the test locally as long as you copy YQLDataSource.js and expandtable.js or a newer version in the same directory.

I am not saying this is the only function that fails. You asked for a concrete example. Try getPreviousTrEl. Just search for sectionRowIndex and try those functions out. All those occurrences assume the same number of rows as records.

Jenny Donnelly

YUI Developer

Posted: 03/25/10

The sectionRowIndex issue is being tracked separately here: #2528233. Let's keep this discussion to the original initAttributes issue.

intp

Posted: 03/30/10

I have tried to replace the code in this example http://developer.yahoo.com/yui/examples/datatable/dt_rowexp_basic.html with the code from http://gist.github.com/225100

But it will not work. There is no error message, it simply does not expand the row. Is there anything else that needs to be done? Does the new code for sure work?

Thanks.

John Lindal

YUI Contributor

Posted: 04/16/10

I have posted the updated code here:

http://jafl.github.com/yui2/rowexpansion/

This will allow us to make progress while YUI 2.9 is under development.

Daniel Kirkdorffer

Posted: 04/16/10

The version posted is buggy. Row expansion states still seem to get out of sync with a row's visual expansion state. I simply expanded and collapses various rows as I bounced around different pages to see this. What I seem to be seeing is that collapsing a row on one page sometimes has a side effect of collapsing a row on a different page visually, but the state is not correctly set and you cannot expand that row again.

Repro steps:
1) Load example: http://jafl.github.com/yui2/rowexpansion/
2) Expand row 1 of page 1
3) Select page 2 and expand row 2 on page 2
4) Go back to page 1 and collapse the expanded row 1

Go back to page 2 and you'll find the previously expanded row 2 on page 2 is no longer expanded, and when you try to expand it nothing happens.

John Lindal

YUI Contributor

Posted: 04/16/10

Excellent. This is why I posted it -- so we can iron out the issues before YUI 2.9 comes out. I've uploaded a fix.

Daniel Kirkdorffer

Posted: 04/16/10

From my cursory verification it looks good. Nice.

Jenny Donnelly

YUI Developer

Posted: 04/20/10
  • location changed from Website to Example
  • milestone changed from 2.NEXT to 2.8.1

Pulling into 2.8.1 branch.

created f3a0051: "[fixes #2528650] Row exp example has bad manners."
3 files changed, 1000 insertions(+), 926 deletions(-)
create mode 100644 templates/examples/datatable/assets/js/rowexpansion.js
rewrite templates/examples/datatable/dt_rowexp_basic_description.php (75%)
rewrite templates/examples/datatable/dt_rowexp_basic_source.php (98%)

Please open new tickets to further discuss related issues.

George

YUI Developer

Posted: 05/5/10
  • resolution changed to fixed
  • status changed from checkedin to closed