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


In ve.ce.TableCellNode#onSetup:

		.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 );

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
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 Freezer on the VisualEditor board.
Danny_B moved this task from Unsorted to Cleanup on the Technical-Debt board.Jan 22 2016, 11:35 PM
Krinkle closed this task as Resolved.Jul 6 2018, 7:13 PM
Krinkle claimed this task.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJul 6 2018, 7:13 PM