[TIMOB-26211] (7_3_X) iOS: Round layout percentage down instead of up (parity)

[hansemannn]

JIRA: https://jira.appcelerator.org/browse/TIMOB-26211

no_tests since UI-related change.

⚠️Master PR #10194 merged already. This one can be merged once 7_3_X is 7.3.1

[build]

	Messages
📖	💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

[jquick-axway]

CR: Pass

[sgtcoolguy]

Not to be too much of a stickler, but I'm 100% certain a test case could be written to expose this. We have a large number of layout tests that verify a number of cases. I'm guessing one of the existing ones may get fixed by this? Or that another one could be written to expose the issue and confirm the fix.

[sgtcoolguy]

So, the ticket itself actually gave a couple test files that could easily be turned into unit tests.

I did so for one of them:

it('TIMOB-26211 - layout should round down fractional units to avoid overflowing total', function (finish) {
	var leftLabel = Ti.UI.createLabel({
			text: '\nLeft (50%)\n',
			textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
			color: 'black',
			backgroundColor: 'green',
			width: '50%',
		}),
		rightLabel = Ti.UI.createLabel({
			text: '\nRight (50%)\n',
			textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
			color: 'black',
			backgroundColor: 'yellow',
			width: '50%',
		});
	win = Ti.UI.createWindow({
		layout: 'horizontal',
		width: 201,
		backgroundColor: 'black',
	});
	win.add(leftLabel);
	win.add(rightLabel);
	win.addEventListener('postlayout', function () {
		if (didPostlayout) {
			return;
		}
		didPostlayout = true;

		try {
			// height doesn't matter, we're checking that
			// we round down on fractional units
			should(leftLabel.size.width).eql(100);
			should(leftLabel.rect.x).eql(0);
			should(leftLabel.rect.y).eql(0);

			should(rightLabel.size.width).eql(100);
			should(rightLabel.rect.x).eql(100);
			should(rightLabel.rect.y).eql(0);

			finish();
		} catch (e) {
			finish(e);
		}
	});
	win.open();
});

This test passes, but looking at the actual visual results on iOS it does not look right. It leaves a black 1dip gap to the far right of the right label here. Given that the total adds up to 100% of the containing window, it should not leave a gap there. I understand that this fix avoids a much bigger issue of wrapping to the next "line", but having a gap is not desirable either. I'm guessing the fix for that is significantly harder, though.

Perhaps moving from using dips to pixels as the underlying unit in our dimension objects would fix that? Or at least shrink the inaccuracy to a single pixel rather than 1dip. The real fix is more likely to involve needing to look at multiple children to help lay them out in cases like this (so say one child gets to round up and the other down to fully fill).

[sgtcoolguy]

Just ran this on Android to compare - and lo and behold it too leaves a gap. So, I suppose this at least fixes the big wrapping issues and achieves parity.

But man oh man, if I were laying out UIs and ran into this I'd not be too happy to be leaving all these slivers around. I suppose you need to make judicious use of FILL and try and ensure you always use even numbers (though it sounds like devices make have odd widths/heights anyhow - like the iPhone 7?)

[jquick-axway]

On Android, we round-down and lay-out the views in pixels, not dips. So, there is no gaps between the views onscreen other than on the edge of the screen for the fraction pixel case. And it's better to "round-down" than to "round" or "round-up" to avoid wrapping/clipping issues. Especially for the fraction pixel case since there is no such thing as a fraction screen pixel. The coordinates must be converted to an integer in the end.

In your test case, let's pretend the scale factor is 1x, which makes the container width 201 pixels. A 50% width would be 100.5 pixels, but since screen pixels are integers, we have to make a choice. You can turn 100.5 pixels to 100 pixels or 101 pixels. If you choose 101 pixels wide for both views, then you'll have a wrapping/clipping issue. If you choose to do 101 pixels wide for the 1st view and 100 pixels wide for the 2nd view to "fill" the container, then both views will have a disproportionate width which would look off-putting on low resolution screens. Especially if the container's width resizes dynamically because the 2nd view would fluctuate in size. I would argue that this would look worse. If you "round-down", then all views would be sized proportionally... even when the container size changes dynamically, making it the best solution in my opinion.

I used to code my own view layouts in OpenGL. So, this isn't my 1st rodeo. :)
But I'm always happy to discuss things like this.

[hansemannn]

@sgtcoolguy @jquick-axway So should this be taken for a 7.3.1? Although I feel it's a useful fix, it still is no regression so it should be moved to 7.4.0 only.