CSS: dimensions workaround for IE11 fullscreen quirk #2401

AVGP · 2015-06-16T21:33:24Z

mgol · 2015-06-16T21:57:25Z

This fix will not work if IE/Edge fixes this bug so I don't think it'll work. We'd need to have a way to feature detect this bug but this would require going to fullscreen which is impossible (and we wouldn't want to do such an invasive test anyway).

Is there any non-intrusive way to detect the issue?

mgol · 2015-06-16T21:59:15Z

I guess it would work if we could get a confirmation from the MS Edge team that this will not get fixed in IE11 as Edge has unprefixed the fullscreen API - so it's enough to test if the API works only with the prefix. We'd workaround it only for IE11 and just hope Edge fixes it soon.

Thoughts, @dmethvin?

AVGP · 2015-06-16T22:02:14Z

@mzgol Valid point. Do you have any info regarding this bug for Edge?
But I'm sure there's gonna be a fix at some point.

There actually is one other possibility as pointed out by http://christophercurrie.github.io/technology/2014/03/20/internet-explorer-11-fullscreen-bug.html which is checking if clientWidth / clientHeight are smaller than offsetWidth / offsetHeight.

Should I try that approach instead?

AVGP · 2015-06-17T06:51:22Z

@mzgol I have now narrowed down the fix to only kick in if dimensions are obviously wrong (clientrect > offset)

mgol · 2015-06-17T10:11:02Z

@AVGP A minor difference in output of these two methods may happen because of fractional values etc. I'd feel safer if you checked that it's much larger, e.g. 99 larger (that's the bug, isn't it?) - maybe not 100x to account for minor differences.

Ideally we'd do it via a support test (see https://github.com/jquery/jquery/blob/master/src/css/support.js and how support.* is used in the code) & cache the result but here the difference exists only in fullscreen mode so I guess we do have to check it every time.

We could cache the result on its first failure but I'm not sure if it's worth it as most people will use the non-fullscreen mode and we couldn't cache the result there.

Unfortunately, we have no way of automatically testing it so it will have to stay without a test for now.

It's starting to look good! 👍

mgol · 2015-06-17T10:11:13Z

src/css.js

@@ -113,6 +113,14 @@ function getWidthOrHeight( elem, name, extra ) {
 		styles = getStyles( elem ),
 		isBorderBox = jQuery.css( elem, "boxSizing", false, styles ) === "border-box";

+	// Fix for edge case in IE 11. See https://github.com/jquery/jquery/issues/1764
+	if ( window.top !== window.self && document.msFullscreenElement &&
+		elem.getBoundingClientRect().width > elem.offsetWidth ) {


Shouldn't this be the other way round?

Hmm, wait a moment. From the description at https://connect.microsoft.com/IE/feedback/details/838286/ie-11-incorrectly-reports-dom-element-sizes-in-fullscreen-mode-when-fullscreened-element-is-within-an-iframe it seems that both values, elem.getBoundingClientRect().width & elem.offsetWidth are 100 times too small. Is this test correct?

Interesting. I have tested against IE 11 on Windows 7 using the VM that Microsoft provides and found getBoundingClientRect to return correct values, while offsetWidth/offsetHeight returned values 100x smaller (but rounded, so I couldn't just multiply those with 100).

I observed this behaviour consistently throughout my tests...

I have a Windows 10 VM around, I'll check if the problem exists in IE11 & Edge and to what extent.

BTW, I'll be working soon on fixing #1724 so there will be more getBoundingClientRect around. It would be cool if it was correct in IE11 because this whole workaround wouldn't be needed then. ;)

Nevermind! I'm an idiot. I mixed up clientWidth / clientHeight with the bounding rect. Dang! So that check is not going to work :/

But that's not a big problem. So I'll check if the ratio clientWidth / offsetWidth is larger than 90. But that's a bit of a danger, as somebody might use a giant border...

AVGP · 2015-06-17T10:50:39Z

@mzgol Alright, I fixed the detection to use the ratio of clientWidth and offsetWidth and the workaround only becomes effective if that's larger than 90 (as there is some rounding error involved).

I don't think using the support.* method and its caching is useful here, it might rather introduce issues me thinks (especially as it's such a nasty edge case).

The code works reliably in my IE 11 / Win7 VM, what does your IE 11 / Edge / Win 10 VM say?

mgol · 2015-06-17T11:11:09Z

@AVGP I see the issue in IE11 with both offsetWidth & getBoundingClientRect:

Edge doesn't experience this bug (\o/):

AVGP · 2015-06-17T11:12:30Z

@mzgol Yup, as I said: I mixed up clientWidth vs. getBoundingRect().width - I fixed that in my latest change.

And hooray for Edge! \o/ :rainbow:

mgol · 2015-06-17T11:22:09Z

Fortunately, Edge not only supports the unprefixed fullscreen API but it also doesn't support the prefixed one. So if document.msFullscreenElement !== undefined (if the browser is not in fullscreen mode this will equal null) then we know we're in IE11 land and this bug exists.

I think comparing with clientWidth is risky as I'm sure there are cases where people create elements with giant borders (some icons are often emulated this way).

So, judging that we cannot test it reliably I think we could construct a support test like:

support.widthReliableInFullscreenMode = 'msFullscreenElement' in document;

with a comment about why we don't have a "true" test and then in the code if this value is true then check if document.msFullscreenElement is truthy and then apply your workaround.

I'll reach out to MS to make sure they don't intend to fix it in IE11, if that's true I think we're good to go.

AVGP · 2015-06-17T11:35:38Z

@mzgol Coool! Alright, that makes things much more predictable!

Now one last question: shouldn't it be

support.widthUnreliableInFullscreenMode = 'msFullscreenElement' in document;

so the inverse (check name) as the width is unreliable in fullscreen if we're having the IE 11?

AVGP · 2015-06-17T11:40:23Z

@mzgol by the way: Thanks for onboarding me so patiently, really appreciate your support 🙇

mgol · 2015-06-17T11:57:40Z

Now one last question: shouldn't it be

support.widthUnreliableInFullscreenMode = 'msFullscreenElement' in document;

I'd prefer to not introduce a negation in the name, it's harder to comprehend what widthUnreliableInFullscreenMode being false means. Most our support tests follow this advice - if they're true, the browser passes the test (this isn't true for all the tests but shhh ;)). it just means that you need to negate the condition as I made the mistake in the test. :)

@mzgol by the way: Thanks for onboarding me so patiently, really appreciate your support 🙇

It's great to have dedicated contributors. :) Pure pleasure.

AVGP · 2015-06-17T12:13:10Z

@mzgol I went with reliableDimensionsInFullscreenMode to follow the convention in support.* and to express that it's actually both width and height that's affected.

mgol · 2015-06-17T12:35:22Z

@mzgol I went with reliableDimensionsInFullscreenMode to follow the convention in support.* and to express that it's actually both width and height that's affected.

Sounds good. 👍

mgol · 2015-06-17T12:36:19Z

src/css/support.js

+		},
+		// Support: IE 11
+		// IE 11 has a bug with offsetWidth / offsetHeight / getClientBoundingRect()
+		// in fullscreen mode inside an iframe


I'd describe what exactly the bug is about and link to the MS Connect bug about it.

mgol · 2015-06-17T18:40:14Z

@mzgol absence of automatization of those tests is dangerous, just like leaving fix without the test

Right; I just having a test in test/integration/ that's not run automatically doesn't really decrease the danger level. ;)

mgol · 2015-06-17T18:40:56Z

src/css.js

@@ -113,6 +113,18 @@ function getWidthOrHeight( elem, name, extra ) {
 		styles = getStyles( elem ),
 		isBorderBox = jQuery.css( elem, "boxSizing", false, styles ) === "border-box";

+	// Support: IE11+


// Support: IE11 only as there is no IE12 and it's fixed in Edge.

markelog · 2015-06-17T19:34:22Z

Right; I just having a test in test/integration/ that's not run automatically doesn't really decrease the danger level. ;)

Well, that is two different issues, one is to follow XP covenants and other is to have test for the specific bug.

For our predecessors, from the seventies, second, was only one thing that mattered, provided that they tested their software before shipping.

markelog · 2015-06-17T19:36:10Z

For our predecessors, from the seventies, second, was only one thing that mattered, provided that they tested their software before shipping.

That was a hint btw :-)

AVGP · 2015-06-17T21:50:38Z

Right, in summary: I'm lost. I need a short, actionable summary of what I have to do to make this land.

@mzgol is going to put one together for me, so stay tuned.

P.S.: Data roaming su-ucks!

AVGP · 2015-06-17T22:35:58Z

@christophercurrie Thanks for the nice, isolated test case :-)

mgol · 2015-06-18T11:14:16Z

@AVGP OK, there's one thing needed to finish this PR: remove support.reliableFullscreenBox, i.e. revert all changes in src/css/support.js & test/unit/support.js and change the condition if ( document.msFullscreenElement && window.top !== window ). Sorry for guiding you into this rabbit hole, that was my mistake as the test doesn't buy us anything here, we have to check the element later anyway. Also, note the change from window.self to window as they're equivalent. This is not a major change, though so I can finish it up if you want.

One other thing is the integration test page but there are a couple of things needed to remember there so I can do it myself in a separate commit and not bother you about it if you want. :) But I'll describe: one should create an HTML page similar to https://github.com/jquery/jquery/blob/compat/test/integration/gh-1684-ajax.html (note that this one is present only in the compat branch so you'll need to create a test/integration directory). This comment says how it looks like: #2183 (comment). I wouldn't just copy http://christophercurrie.github.io/technology/2014/03/20/internet-explorer-11-fullscreen-bug.html but create a simple page that will say "Test failed" or "Test passed" as the page I linked to. If a person is not in fullscreen mode instead of "Test passed" it should probably say "Go to fullscreen mode", there should also be a button allowing to go to fullscreen mode & back.

Fixes jquery#1764

AVGP · 2015-06-18T12:39:06Z

@mzgol Alright. I reverted the changes and adjusted the condition as desired.

I'm not sure if I will give the integration test a shot. It does sound interesting, but I simply don't know if I come around doing it.

If you take that upon you, that'd be great - if I have some time, I may give it a go (and will push here for you to see)... whatever gets us this fix landed :-)

mgol · 2015-06-18T21:17:44Z

OK, I'll add the test. The rest looks ready. Thanks @AVGP!

AVGP · 2015-06-18T21:22:22Z

@mzgol Well thanks to you, @markelog, @christophercurrie and @gibson042 for helping me making it this far :)

(cherry-picked from 90d828b) Fixes gh-1764 Closes gh-2401

Refs jquerygh-1764 Refs jquerygh-2401

mgol · 2015-06-22T23:56:45Z

@AVGP Landed on master & compat, thanks! I added the integration test in a separate PR, see #2425 (screenshots attached).

AVGP · 2015-06-22T23:57:59Z

@mzgol Thanks for making it so! I'm looking forward to the next release then :-)

Refs jquerygh-1764 Refs jquerygh-2401

Refs gh-1764 Refs gh-2401 Closes gh-2425

(cherry-picked from 8887106) Refs gh-1764 Refs gh-2401 Closes gh-2425

IE 11 used to have an issue where if an element inside an iframe was put in fullscreen mode, the element dimensions started being 100 times too small; we've added a workaround that would multiply them by 100. However, the IE 11 issue has been unexpectedly fixed and since our detection was really detecting the browser and not a bug, we've started breaking the browser instead of fixing it. Since there's no good way to detect if the bug exists, we have to back the workaround out completely. Fixes gh-3041 Refs gh-1764 Refs gh-2401 Refs 90d828b

IE 11 used to have an issue where if an element inside an iframe was put in fullscreen mode, the element dimensions started being 100 times too small; we've added a workaround that would multiply them by 100. However, the IE 11 issue has been unexpectedly fixed and since our detection was really detecting the browser and not a bug, we've started breaking the browser instead of fixing it. Since there's no good way to detect if the bug exists, we have to back the workaround out completely. Refs ff1a082 Fixes gh-3041 Refs gh-1764 Refs gh-2401 Refs 90d828b

IE 11 used to have an issue where if an element inside an iframe was put in fullscreen mode, the element dimensions started being 100 times too small; we've added a workaround that would multiply them by 100. However, the IE 11 issue has been unexpectedly fixed and since our detection was really detecting the browser and not a bug, we've started breaking the browser instead of fixing it. Since there's no good way to detect if the bug exists, we have to back the workaround out completely. Refs ff1a082 Refs fb9adb9 Fixes gh-3041 Refs gh-1764 Refs gh-2401 Refs 90d828b

jquerybot added the CLA: Error label Jun 16, 2015

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from b9513bb to 25c134d Compare June 16, 2015 21:47

jquerybot added CLA: Valid and removed CLA: Error labels Jun 16, 2015

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from 25c134d to a6d1fa4 Compare June 16, 2015 21:54

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from a6d1fa4 to f9f2891 Compare June 17, 2015 06:50

mgol reviewed Jun 17, 2015
View reviewed changes

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from f9f2891 to 266fd38 Compare June 17, 2015 10:48

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from 266fd38 to fc69b37 Compare June 17, 2015 11:57

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from fc69b37 to adcdab0 Compare June 17, 2015 12:12

mgol reviewed Jun 17, 2015
View reviewed changes

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from adcdab0 to c8dde6e Compare June 17, 2015 13:21

mgol reviewed Jun 17, 2015
View reviewed changes

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from 1034428 to fcb4b3f Compare June 17, 2015 22:34

AVGP force-pushed the 1764-ie-fullscreen-dimensions-workaround branch from fcb4b3f to be824b9 Compare June 18, 2015 12:37

CSS: dimensions workaround for IE11 fullscreen quirk

be824b9

Fixes jquery#1764

mgol self-assigned this Jun 18, 2015

mgol closed this in 90d828b Jun 22, 2015

mgol pushed a commit that referenced this pull request Jun 22, 2015

CSS: Work around an IE11 fullscreen dimensions bug

6df1bf9

(cherry-picked from 90d828b) Fixes gh-1764 Closes gh-2401

mgol added a commit to mgol/jquery that referenced this pull request Jun 22, 2015

CSS: Add an integration test for issue jquerygh-1764

25d1eca

Refs jquerygh-1764 Refs jquerygh-2401

mgol mentioned this pull request Jun 22, 2015

CSS: Add an integration test for issue gh-1764 #2425

Closed

mgol added a commit to mgol/jquery that referenced this pull request Jun 25, 2015

CSS: Add an integration test for issue jquerygh-1764

61a27b6

Refs jquerygh-1764 Refs jquerygh-2401

mgol added a commit to mgol/jquery that referenced this pull request Jul 7, 2015

CSS: Add an integration test for issue jquerygh-1764

3ad86bb

Refs jquerygh-1764 Refs jquerygh-2401

mgol added a commit that referenced this pull request Jul 8, 2015

CSS: Add an integration test for issue gh-1764

8887106

Refs gh-1764 Refs gh-2401 Closes gh-2425

mgol added a commit that referenced this pull request Jul 8, 2015

CSS: Add an integration test for issue gh-1764

7ee0fea

(cherry-picked from 8887106) Refs gh-1764 Refs gh-2401 Closes gh-2425

dmethvin mentioned this pull request Apr 5, 2016

.width() in fullscreen mode in iframe is too large in IE 11 #3041

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

CSS: dimensions workaround for IE11 fullscreen quirk #2401

CSS: dimensions workaround for IE11 fullscreen quirk #2401

Conversation

AVGP commented Jun 16, 2015

mgol commented Jun 16, 2015

mgol commented Jun 16, 2015

AVGP commented Jun 16, 2015

AVGP commented Jun 17, 2015

mgol commented Jun 17, 2015

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

AVGP commented Jun 17, 2015

mgol commented Jun 17, 2015

AVGP commented Jun 17, 2015

mgol commented Jun 17, 2015

AVGP commented Jun 17, 2015

AVGP commented Jun 17, 2015

mgol commented Jun 17, 2015

AVGP commented Jun 17, 2015

mgol commented Jun 17, 2015

Choose a reason for hiding this comment

mgol commented Jun 17, 2015

Choose a reason for hiding this comment

markelog commented Jun 17, 2015

markelog commented Jun 17, 2015

AVGP commented Jun 17, 2015

AVGP commented Jun 17, 2015

mgol commented Jun 18, 2015

AVGP commented Jun 18, 2015

mgol commented Jun 18, 2015

AVGP commented Jun 18, 2015

mgol commented Jun 22, 2015

AVGP commented Jun 22, 2015