Clean up mixed use of getters, attributes and properties around rowspan/colspan in TableCellNode
Open, LowPublic

Description

In ve.ce.TableCellNode#onSetup:

	this.$element
..
		.attr( {
			rowspan: this.model.getRowspan(),
			colspan: this.model.getColspan()
		} );

This should be changed to prop( { rowSpan: .., colSpan: .. } ) since we don't care about updating the DOM attribute values for serialisation, but the live values as used in rendering (the browser usually syncs them for you, however).

I tried changing this, but that would create a mismatch with the logic in ve.ce.TableCellNode#onAttributeChange:

	switch ( key ) {
		case 'colspan':
		case 'rowspan':
			if ( to > 1 ) {
				this.$element.attr( key, to );
			} else {
				this.$element.removeAttr( key );
			}
			break;

As it assumes the data model attribute to be the same as the view attribute. And one place uses getRowspan, and the other just access the attribute directly. I guess the getters are not needed?

In the DOM, attributes only exist when created by the user. properties, however, as being an interface always exist and have relevant defaults.

A newly created empty table cell has a property of rowSpan === 1:

node = document.createElement( 'td' );
node.rowSpan; // 1
"<td>"
node.getAttribute( 'rowspan' ); // null

So we may not need the defaulting to 1. And that logic itself also seems done more defensively and repeatedly then needed. There's about half a dozen places where we consolidate the rowspan/colspan and have logic for defaulting to 1. We should only have to do that in one place (data model consumption). And I suspect even that one isn't needed.

Krinkle created this task.Jan 28 2015, 9:13 PM
Krinkle updated the task description. (Show Details)
Krinkle raised the priority of this task from to Needs Triage.
Krinkle added a subscriber: Krinkle.
Restricted Application added a project: VisualEditor. · View Herald TranscriptJan 28 2015, 9:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdforrester-WMF triaged this task as Low priority.Jan 28 2015, 11:46 PM
Jdforrester-WMF set Security to None.
Jdforrester-WMF moved this task from To Triage to Backlog on the VisualEditor board.
Danny_B moved this task from Unsorted to Cleanup on the Technical-Debt board.Jan 22 2016, 11:35 PM