CSS: Fix support test results for initially hidden iframes #5317

ssigwart · 2023-09-12T03:19:17Z

Summary

If the iframe is not initially visible, boxSizingReliableVal gets set to false, which triggers this code to set the wrong dialog width here:

jquery/src/css.js

Lines 418 to 426 in a7cc3a0

    
           if ( isBorderBox && scrollboxSizeBuggy ) { 
        
           	subtract -= Math.ceil( 
        
           		elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ] - 
        
           		parseFloat( styles[ dimension ] ) - 
        
           		boxModelAdjustment( elem, dimension, "border", false, styles ) - 
        
           		0.5 
        
           	); 
        
           }

I think this should solve jquery/jquery-ui#2176.

You can reproduce something similar to the original issue by creating the following two files in the top level of a website. Then go to index.html, click "Show Frame", then "Show Popup". The popup should be 600px, but is not.

`index.html`

<!DOCTYPE html>
<html>
	<body>
		<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.7.1/jquery.js" integrity="sha512-+k1pnlgt4F1H8L7t3z95o3/KO+o78INEcXTbnoJQ/F2VqDVhWoaiVml/OEHv9HsVgxUaVW+IbiZPUJQfF/YxZw==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
		<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.13.2/jquery-ui.js" integrity="sha512-ynDTbjF5rUHsWBjz7nsljrrSWqLTPJaORzSe5aGCFxOigRZRmwM05y+kuCtxaoCSzVGB1Ky3XeRZsDhbSLdzXQ==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
		<div hidden="hidden" id="testFrame">
			<iframe src="/innerIframe.html" style="width: 100%; height: 500px"></iframe>
		</div>
		<button type="button" onclick="$('#testFrame').prop('hidden', false);this.hidden=true;">Show Frame</button>
	</body>
</html>

`innerIframe.html`

<!DOCTYPE html>
<html>
	<head>
		<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.13.2/themes/base/jquery-ui.min.css" integrity="sha512-ELV+xyi8IhEApPS/pSj66+Jiw+sOT1Mqkzlh8ExXihe4zfqbWkxPRi8wptXIO9g73FSlhmquFlUOuMSoXz5IRw==" crossorigin="anonymous" referrerpolicy="no-referrer" />
		<style>
			* {
				box-sizing: border-box;
			}
			.ui-dialog {
				width: 400px;
			}
		</style>
	</head>
	<body>
		<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.7.1/jquery.js" integrity="sha512-+k1pnlgt4F1H8L7t3z95o3/KO+o78INEcXTbnoJQ/F2VqDVhWoaiVml/OEHv9HsVgxUaVW+IbiZPUJQfF/YxZw==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
		<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.13.2/jquery-ui.js" integrity="sha512-ynDTbjF5rUHsWBjz7nsljrrSWqLTPJaORzSe5aGCFxOigRZRmwM05y+kuCtxaoCSzVGB1Ky3XeRZsDhbSLdzXQ==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
		<div id="testPopup" hidden="hidden" style="width: 800px;">
			Popup
		</div>
		<button type="button" onclick="$('#testPopup').dialog({width: 600});">Show Popup</button>
		<script>
			// Calling this triggers `computeStyleTests` to be called before the page is visible
			console.log($('#testPopup').outerHeight());
		</script>
	</body>
</html>

Checklist

New tests have been added to show the fix or feature works
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

mgol

Thanks for the PR. We will need a unit test added for a change like that. This one would go to https://github.com/jquery/jquery/blob/main/test/unit/css.js.

ssigwart · 2023-09-21T01:14:35Z

Thanks, @mgol. I added a test and verified that it failed prior to my change.

mgol · 2023-10-16T16:35:20Z

src/css/support.js

@@ -18,6 +18,11 @@ define( [
 			return;
 		}

+		// Don't run until window is visible (https://github.com/jquery/jquery-ui/issues/2176)
+		if ( documentElement.offsetHeight === 0 ) {


This will be true if height: 0 is set on the root element in CSS. We may need a better check.

Do you have any suggestions? I'm not sure what the rational for setting height: 0 on the root element would be.

Maybe I can have it check if the width is zero too. I didn't test that yet. I wanted to get your thoughts on that first.

Thanks.

I would rather check against div directly than against its containing document and assuming accuracy and correspondence. Specifically, I think div.offsetWidth would be a good target: https://jsfiddle.net/tjnvs51o/

I updated to div.offsetWidth. It think it works, though I'm struggling to figure out how to run all the tests.

There are a couple ways to run tests:

With npm scripts: npm run browser. This will run tests on the command line using karma and headless browsers. Set the browser you want with the BROWSERS env var. See Gruntfile.cjs.

Open the QUnit test page directly and run the test using the QUnit UI. Run node test/server.js and visit http://localhost:3000/test/. Your test is in the support module so select that from the module list to filter the tests.

The test in this PR is failing in Safari. Also, we want to remove the test container element before the early return.

@timmywil we should document this in README.

I'm planning on documenting all of the upcoming test changes (to drop grunt, karma, etc). node test/server.js will likely be added to an npm script.

gibson042 · 2023-10-30T16:30:58Z

test/unit/css.js

+	"css/cssComputeStyleTests.html",
+	function( assert, jQuery, window, document, initialHeight ) {
+		assert.expect( 2 );
+		assert.strictEqual( initialHeight, 0, "initial height should be undefined" );


Looks like Safari actually is reporting offsetHeight for the div in the hidden iframe: https://github.com/jquery/jquery/actions/runs/6634909917/job/18189746921?pr=5317#step:9:161

Suggested change

assert.strictEqual( initialHeight, 0, "initial height should be undefined" );

assert.strictEqual( initialHeight === 0 ? 20 : initialHeight, 20,

"hidden-frame content sizes should be zero or accurate" );

src/css/support.js

ssigwart · 2023-11-01T03:07:01Z

I made the suggested updates. I haven't really been able to get the tests running well. I might be running them wrong. The way I'm running them, it's failing on 3.x-stable too. Also, npm run browser failed with Missing script: "browser" and I don't see a test/server.js file.

mgol · 2023-11-01T13:30:15Z

@ssigwart you need to rebase over latest 3.x-stable. And the command should actually be npm run test:browser.

ssigwart · 2023-11-01T13:41:55Z

Thanks, @mgol. I'll try to do that tonight. I try not to rebase after people have started reviewing so it doesn't mess up their review.

mgol · 2023-11-01T13:52:50Z

Also, there’s a bug in Chrome 117 & 118 that causes one support test failure. The issue is fixed in Chrome 119 so we’re just waiting for GitHub Actions images to be updated with Chrome 119 to resolve this one failure.

If the iframe is not initially visible, `boxSizingReliableVal` gets set to false, which triggers this code to set the wrong dialog width here: https://github.com/jquery/jquery/blob/a7cc3a0f0418d168ff3818576161c5af24d0ca78/src/css.js#L418-L426 I think this should solve jquery/jquery-ui#2176.

ssigwart · 2023-11-02T01:36:35Z

Thanks. I rebased and force pushed that. Running npm run test:browser worked. There was one failure, but that was on Chrome 118. Is there anything else you need me to do on this PR?

mgol

Thanks for the updates! LGTM.

ssigwart · 2023-11-02T12:33:10Z

Thanks for your help and merging this.

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Ref jquerygh-5317

Ref jquerygh-5317

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Ref jquerygh-5317

mgol · 2023-11-03T10:23:46Z

I submitted two followups to this PR, to apply the fix to the reliableTrDimensions support test as well.

main version: CSS: Fix reliableTrDimensions support test for initially hidden iframes #5358
3.x-stable version: CSS: Fix reliableTrDimensions test for initially hidden iframes (3.x) #5359

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Closes gh-5359 Ref gh-5317 Ref gh-5358

Closes gh-5358 Ref gh-5317 Ref gh-5359

Old iOS (<10) always rounds widths down, account for this in tests. Closes gh-5361 Ref gh-5317 Ref gh-5359

ssigwart mentioned this pull request Sep 12, 2023

UI Dialog inside non-visible iFrame opens alternatingly with width=0 jquery/jquery-ui#2176

Open

hexiaokang approved these changes Sep 15, 2023

View reviewed changes

mgol added Needs info CSS labels Sep 20, 2023

mgol requested changes Sep 20, 2023

View reviewed changes

mgol added 3.x-only and removed Needs info labels Sep 20, 2023

mgol added Needs review Discuss in Meeting labels Sep 21, 2023

mgol added this to the 3.7.2 milestone Sep 21, 2023

mgol reviewed Oct 16, 2023

View reviewed changes

gibson042 reviewed Oct 30, 2023

View reviewed changes

timmywil removed the Discuss in Meeting label Oct 30, 2023

mgol reviewed Oct 30, 2023

View reviewed changes

src/css/support.js Show resolved Hide resolved

ssigwart and others added 6 commits November 1, 2023 21:30

Fix issue link

a17fd73

CSS: Add test

474be4a

CSS: Use div width instead of document

333ee33

CSS: Remove container

602caa4

CSS: Update test

8edeb51

ssigwart force-pushed the narrowDialog branch from 5104a29 to 8edeb51 Compare November 2, 2023 01:30

mgol approved these changes Nov 2, 2023

View reviewed changes

mgol changed the title ~~Fix UI Dialog Width for Initially Hidden Dialog~~ CSS: Fix support test results for initially hidden iframes Nov 2, 2023

mgol merged commit ac68b21 into jquery:3.x-stable Nov 2, 2023

mgol removed the Needs review label Nov 2, 2023

ssigwart deleted the narrowDialog branch November 2, 2023 12:32

mgol added a commit to mgol/jquery that referenced this pull request Nov 3, 2023

CSS: Fix reliableTrDimensions support test for initially hidden iframes

5eae3e0

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Ref jquerygh-5317

mgol added a commit to mgol/jquery that referenced this pull request Nov 3, 2023

CSS: Fix reliableTrDimensions support test for initially hidden iframes

0aac9ad

Ref jquerygh-5317

mgol added a commit to mgol/jquery that referenced this pull request Nov 3, 2023

CSS: Fix reliableTrDimensions test for initially hidden iframes (3.x)

49ffd4c

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Ref jquerygh-5317

This was referenced Nov 3, 2023

CSS: Fix reliableTrDimensions support test for initially hidden iframes #5358

Merged

CSS: Fix reliableTrDimensions test for initially hidden iframes (3.x) #5359

Merged

mgol added a commit that referenced this pull request Nov 6, 2023

CSS: Fix reliableTrDimensions test for initially hidden iframes (3.x)

4e98980

Also, account for the fact old Firefox (<61) has `null` computed style for elements in such iframes. Closes gh-5359 Ref gh-5317 Ref gh-5358

mgol added a commit that referenced this pull request Nov 6, 2023

CSS: Fix reliableTrDimensions support test for initially hidden iframes

b1e66a5

Closes gh-5358 Ref gh-5317 Ref gh-5359

mgol added a commit that referenced this pull request Nov 13, 2023

Tests: Workaround a tr width test issue in old iOS

b5b43ad

Old iOS (<10) always rounds widths down, account for this in tests. Closes gh-5361 Ref gh-5317 Ref gh-5359

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS: Fix support test results for initially hidden iframes #5317

CSS: Fix support test results for initially hidden iframes #5317

ssigwart commented Sep 12, 2023 •

edited

Loading

mgol left a comment

ssigwart commented Sep 21, 2023

mgol Oct 16, 2023

ssigwart Oct 17, 2023

gibson042 Oct 23, 2023

ssigwart Oct 25, 2023

timmywil Oct 30, 2023

timmywil Oct 30, 2023

mgol Oct 30, 2023

timmywil Oct 30, 2023

gibson042 Oct 30, 2023

ssigwart commented Nov 1, 2023

mgol commented Nov 1, 2023 •

edited

Loading

ssigwart commented Nov 1, 2023

mgol commented Nov 1, 2023 •

edited

Loading

ssigwart commented Nov 2, 2023

mgol left a comment

ssigwart commented Nov 2, 2023

mgol commented Nov 3, 2023

	if ( isBorderBox && scrollboxSizeBuggy ) {
	subtract -= Math.ceil(
	elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ] -
	parseFloat( styles[ dimension ] ) -
	boxModelAdjustment( elem, dimension, "border", false, styles ) -
	0.5
	);
	}

	assert.strictEqual( initialHeight, 0, "initial height should be undefined" );
	assert.strictEqual( initialHeight === 0 ? 20 : initialHeight, 20,
	"hidden-frame content sizes should be zero or accurate" );

CSS: Fix support test results for initially hidden iframes #5317

CSS: Fix support test results for initially hidden iframes #5317

Conversation

ssigwart commented Sep 12, 2023 • edited Loading

Summary

index.html

innerIframe.html

Checklist

mgol left a comment

Choose a reason for hiding this comment

ssigwart commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssigwart commented Nov 1, 2023

mgol commented Nov 1, 2023 • edited Loading

ssigwart commented Nov 1, 2023

mgol commented Nov 1, 2023 • edited Loading

ssigwart commented Nov 2, 2023

mgol left a comment

Choose a reason for hiding this comment

ssigwart commented Nov 2, 2023

mgol commented Nov 3, 2023

ssigwart commented Sep 12, 2023 •

edited

Loading

`index.html`

`innerIframe.html`

mgol commented Nov 1, 2023 •

edited

Loading

mgol commented Nov 1, 2023 •

edited

Loading