Page MenuHomePhabricator

Consider aligning sections by offsetTop directly, rather than trying to equalize cumulative height
Closed, ResolvedPublic

Description

It seems something like the following strategy might work more reliably:

function keepAlignment( sourceNode, targetNode ) {
    var offsetTop = Math.max( sourceNode.offsetTop, targetNode.offsetTop );
    sourceNode.style.paddingTop = ( offsetTop - sourceNode.offsetTop ) + 'px';
    targetNode.style.paddingTop = ( offsetTop - targetNode.offsetTop ) + 'px';
}

It doesn't require all previous sections to be in alignment, and it doesn't change the height of the content (and therefore hopefully shouldn't change the word wrapping nor cause much browser rerendering effort).

Event Timeline

dchan created this task.Feb 17 2017, 5:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 17 2017, 5:37 PM

Even though tt doesn't require all previous sections to be in alignment, a section content change handler need to update the offset top of all existing sections one after another.

santhosh closed this task as Resolved.Apr 9 2018, 5:25 AM
santhosh claimed this task.

The current version of CX uses this approach. See https://github.com/wikimedia/mediawiki-extensions-ContentTranslation/blob/master/modules/ui/mw.cx.ui.TranslationView.js#L85
A small difference is, instead of paddingTop, marginTop is used.

mw.cx.ui.TranslationView.static.alignSectionPair = function ( sourceOffsetTop, targetOffsetTop, sectionNumber ) {
	var offsetTop, viewNode,
		sourceNode = document.getElementById( 'cxSourceSection' + sectionNumber ),
		targetNode = document.getElementById( 'cxTargetSection' + sectionNumber );

	function isSubclass( x, y ) {
		return x && ( x.constructor === y || x.constructor instanceof y );
	}
	if ( !sourceNode || !targetNode ) {
		return;
	}
	viewNode = $.data( targetNode, 'view' );
	sourceNode.style.marginTop = '';
	targetNode.style.marginTop = '';
	targetNode.style.height = '';
	offsetTop = Math.max(
		sourceOffsetTop + sourceNode.offsetTop,
		targetOffsetTop + targetNode.offsetTop
	);
	sourceNode.style.marginTop = ( offsetTop - sourceOffsetTop - sourceNode.offsetTop ) + 'px';
	targetNode.style.marginTop = ( offsetTop - targetOffsetTop - targetNode.offsetTop ) + 'px';
	if ( isSubclass( viewNode, ve.ce.CXPlaceholderNode ) || isSubclass( viewNode, ve.ce.CXSectionNode ) ) {
		if ( sourceNode.offsetHeight > targetNode.offsetHeight ) {
			targetNode.style.height = sourceNode.offsetHeight + 'px';
		}
	}
};

It's not a small between offsetTop and paddingTop/marginTop and I am puzzled why you marked this as resolved instead of declined. Do you have ideas how to fix T190917 while also taking care of T190907#4111696?

It is not declined. @dchan reported it and later he implemented this alignment without using the height, but using offset top.

I see, we are *reading* from offsetTop and *writing* to marginTop. For some reason I was thinking of writing to offset.