FieldLayout and FieldsetLayout 'help' popups inside a PanelLayout inside a Dialog always appear with a disabled horizontal scrollbar
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	matmarex
	Feb 14 2017, 11:54 PM

Description

In the following super-simple example, the FieldLayout and FieldsetLayout 'help' popups always appear with a disabled horizontal scrollbar. This is unexpected, as there is ample space to display them.

(You can copy-paste the code into browser JS console on https://doc.wikimedia.org/oojs-ui/master/demos/ to test.)

function MyProcessDialog( config ) {
	MyProcessDialog.parent.call( this, config );
}
OO.inheritClass( MyProcessDialog, OO.ui.ProcessDialog );
MyProcessDialog.static.name = 'myProcessDialog';
MyProcessDialog.static.title = 'Process dialog';
MyProcessDialog.prototype.initialize = function () {
	var loremIpsum, fieldset, panel;

	MyProcessDialog.parent.prototype.initialize.apply( this, arguments );

	loremIpsum = 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, ' +
		'sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.\u200E';

	fieldset = new OO.ui.FieldsetLayout( {
		label: 'Fieldset',
		help: loremIpsum,
		items: [
			new OO.ui.FieldLayout( new OO.ui.CheckboxInputWidget(), {
				align: 'inline',
				label: 'Field',
				help: loremIpsum
			} )
		]
	} );

	panel = new OO.ui.PanelLayout( { padded: true, expanded: false } );

	panel.$element.append( fieldset.$element );
	this.$body.append( panel.$element );
};
MyProcessDialog.prototype.getBodyHeight = function () {
	return 500;
}
var windowManager = new OO.ui.WindowManager();
$( 'body' ).append( windowManager.$element );
var dialog = new MyProcessDialog();
windowManager.addWindows( [ dialog ] );
windowManager.openWindow( dialog );

Related Objects
Search...

Status	Assigned	Task
Resolved	Catrope	T158341 ClippableElement horizontal clipping is totally broken
Resolved	Catrope	T158138 FieldLayout and FieldsetLayout 'help' popups inside a PanelLayout inside a Dialog always appear with a disabled horizontal scrollbar
Invalid	matmarex	T158222 Create a diagram describing the unholy calculations going on in ClippableElement#clip
Open	None	T158223 Create an interactive demo for ClippableElement that will scroll and resize things by itself

Event Timeline

matmarex created this task.Feb 14 2017, 11:54 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 14 2017, 11:54 PM

Applying the following patch seems to resolve the issue – but who knows what else it breaks. I honestly don't know if it's more correct before or after. The code in ClippableElement is hopeless.

(Incorrect; see later comments)

diff --git a/src/mixins/ClippableElement.js b/src/mixins/ClippableElement.js
index 2a716fe..560bf95 100644
--- a/src/mixins/ClippableElement.js
+++ b/src/mixins/ClippableElement.js
@@ -233,8 +233,8 @@ OO.ui.mixin.ClippableElement.prototype.clip = function () {
        // It should never be desirable to exceed the dimensions of the browser viewport... right?
        desiredWidth = Math.min( desiredWidth, document.documentElement.clientWidth );
        desiredHeight = Math.min( desiredHeight, document.documentElement.clientHeight );
-       allotedWidth = Math.ceil( desiredWidth - extraWidth );
-       allotedHeight = Math.ceil( desiredHeight - extraHeight );
+       allotedWidth = Math.ceil( desiredWidth );
+       allotedHeight = Math.ceil( desiredHeight );
        naturalWidth = this.$clippable.prop( 'scrollWidth' );
        naturalHeight = this.$clippable.prop( 'scrollHeight' );
        clipWidth = allotedWidth < naturalWidth;
@@ -246,14 +246,14 @@ OO.ui.mixin.ClippableElement.prototype.clip = function () {
                this.$clippable.css( 'overflowX', 'scroll' );
                void this.$clippable[ 0 ].offsetHeight; // Force reflow
                this.$clippable.css( {
-                       width: Math.max( 0, allotedWidth ),
+                       width: Math.max( 0, allotedWidth - extraWidth ),
                        maxWidth: ''
                } );
        } else {
                this.$clippable.css( {
                        overflowX: '',
                        width: this.idealWidth ? this.idealWidth - extraWidth : '',
-                       maxWidth: Math.max( 0, allotedWidth )
+                       maxWidth: Math.max( 0, allotedWidth - extraWidth )
                } );
        }
        if ( clipHeight ) {
@@ -262,14 +262,14 @@ OO.ui.mixin.ClippableElement.prototype.clip = function () {
                this.$clippable.css( 'overflowY', 'scroll' );
                void this.$clippable[ 0 ].offsetHeight; // Force reflow
                this.$clippable.css( {
-                       height: Math.max( 0, allotedHeight ),
+                       height: Math.max( 0, allotedHeight - extraHeight ),
                        maxHeight: ''
                } );
        } else {
                this.$clippable.css( {
                        overflowY: '',
                        height: this.idealHeight ? this.idealHeight - extraHeight : '',
-                       maxHeight: Math.max( 0, allotedHeight )
+                       maxHeight: Math.max( 0, allotedHeight - extraHeight )
                } );
        }

I talked to @Catrope on IRC, the above patch isn't correct and must have only worked accidentally.

[19:00]	<MatmaRex> RoanKattouw: have you seen https://phabricator.wikimedia.org/T158138 and the patch there? does that look even worth submitting for review?
[19:01]	<RoanKattouw> MatmaRex: Interesting patch, do you have any intuition as to why this fixes the bug?
[19:02]	<RoanKattouw> Also we probably need a diagram for that code and other places where we do geometrical computations (e.g. PopupWidget#updateDimensions)
[19:04]	<RoanKattouw> I find it difficult to understand or modify that code without drawing some diagrams on paper
[19:04]	<MatmaRex> RoanKattouw: i've kind of banged on it until it seemed to do something different, really. and yeah, it's terrible
[19:05]	<MatmaRex> RoanKattouw: effectively, i think it fixes the meaning of the 'allotedWidth < naturalWidth' comparison, without affecting the sizing itself
[19:05]	<RoanKattouw> I mean you're just moving where extraHeight is subtracted so I'm convinced that the end result is the same unless it takes a different branch when deciding what to do based on allotedHeight/Width
[19:05]	<MatmaRex> naturalWidth represents the "internal" width, and so does allotedWidth (~= desiredWidth). extraWidth shouldn't be included… i think?
[19:05]	<MatmaRex> yes. it changes based on that comparison
[19:06]	<RoanKattouw> Looking up the definition, it's the difference in width between the container and the clippable
[19:06]	<MatmaRex> the calculations in both cases are the same (except for a ceil() call happening in a different place…). but it changes which case is used.
[19:07]	<RoanKattouw> So if the container is wider/taller than the clippable that value will be positive
[19:12]	<RoanKattouw> It looks like desiredWidth/Height is the distance between the top/left edge of the clippable element and the  right/bottom edge of the scrollable container
[19:12]	<RoanKattouw> Ignoring for the moment the ccOffset.left < 0 ternary which I'm not at all convinced by
[19:13]	<RoanKattouw> So that makes sense to me as an "available width/height" computation
[19:14]	<RoanKattouw> Now I don't understand why extraWidth/Height is involved, which sis the amount by which the scrollable container is bigger than the clppable element
[19:14]	<RoanKattouw> Why is that subtracted at all... in most cases that would be a high value
[19:17]	<MatmaRex> RoanKattouw: not the scrollable container, the clippable container
[19:17]	<RoanKattouw> 		( scOffset.left + scrollLeft + scWidth ) - ccOffset.left;
[19:17]	<MatmaRex> extraWidth/Height effectively represents a "border" around the clippable element that should be kept visible, i think
[19:18]	<RoanKattouw> scOffset.left + scWidth  is the right edge of the scrollable container, and ccOffset.left is the left edge of the clippable container
[19:18]	<RoanKattouw> However, I think these offsets might be relative, not absolute?
[19:18]	<RoanKattouw> Oh, no, offset is absolute
[19:18]	<RoanKattouw> What I don't understand is why the current code works so well, because these extraWidth/Height values must typically be huge
[19:19]	<MatmaRex> they aren't
[19:19]	<MatmaRex> ok, we need a diagram :D hold on
[19:19]	<RoanKattouw> Oooh, I see
[19:19]	<RoanKattouw> extraWidth + Height refers to the CLIPPABLE container vs the clippable element, gotcha
[19:19]	<RoanKattouw> So if $clippableContainer is not specified, that'll be 0
[19:21]	<MatmaRex> https://i.imgur.com/9NIQC9e.png big red rectangle is $clippableContainer, small red rectangle is $clippable, extraWidth is the sum of the green lines
[19:21]	<RoanKattouw> Right
[19:22]	<RoanKattouw> That's dodgy though
[19:22]	<RoanKattouw> You'd expect only the first green line to be used
[19:23]	<RoanKattouw> OK, I think I'm starting to get it
[19:23]	<RoanKattouw> The inner rectangle is the one that's scrollable
[19:24]	<RoanKattouw> And the outer rectangle is not, so the width of the "padding" around it has to be reserved
[19:24]	<RoanKattouw> So I think the old code makes sense
[19:25]	<RoanKattouw> naturalWidth/Height are computed on the scrollable inner rectangle, so that does not include extraWidth/Height
[19:25]	<RoanKattouw> But desiredWidth/Height does include it because it's how wide/tall the outer rectangle wants to be
[19:26]	<RoanKattouw> So we have to subtract it out before we compare, so that it's apples to apples
[19:27]	<MatmaRex> hmm.

The ClippableElement code where this bug is hiding is very difficult to reason about, and we're definitely going to need a diagram to make sense of it. And I think we're also going to need some interactive demo that will scroll and resize things by itself, changes to this are currently a major pain to test. Either of these is probably going to be a few hours of work, and right now all this messy nasty code is somehow magically working mostly okay in most cases, so I'd say this is pretty low priority.

matmarex added a comment.Feb 15 2017, 6:42 PM

This comment was removed by matmarex.

matmarex created subtask T158222: Create a diagram describing the unholy calculations going on in ClippableElement#clip.Feb 15 2017, 6:47 PM

matmarex triaged this task as Low priority.Feb 15 2017, 6:50 PM

matmarex created subtask T158223: Create an interactive demo for ClippableElement that will scroll and resize things by itself.

This is probably a specific case of T158341.

matmarex removed matmarex as the assignee of this task.Feb 16 2017, 7:13 PM

This looks fixed, probably by https://gerrit.wikimedia.org/r/#/c/347658/.

matmarex closed this task as Resolved.Apr 12 2017, 10:23 PM

matmarex closed subtask T158222: Create a diagram describing the unholy calculations going on in ClippableElement#clip as Invalid.Nov 9 2017, 11:23 PM