New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
heporap opened this Issue Apr 5, 2016 · 18 comments

Comments

Projects
None yet
7 participants
@heporap

heporap commented Apr 5, 2016

.width() is incorrect value in fullscreen mode of iframe.

I got 12800 on IE11. IE Edge is OK.

<iframe src="iframe.html" allowFullscreen webkitAllowFullscreen></iframe>

iframe.html, html

<div id="fs" style="width:100%;height:100px;background-color:green;color:black;">
    <p><input type="button" value="fullscreen" id="run"></p>
    <p><input type="button" value="test" id="test"></p>
    <p id="output"></p>
</div>

iframe.html, javascript

jQuery(function($){
    $('#test').click(function(){
        $('#output').text( $('#fs').width() );
    });
    $('#run').click(function(){
        pageEnterFullScreen( $('#fs')[0] );
    });

});

function pageEnterFullScreen(element){

    if( element.requestFullscreen ){
        element.requestFullscreen();

    }else if( element.msRequestFullscreen ) {
        element.msRequestFullscreen();

    }else if(element.mozRequestFullScreen ){
        element.mozRequestFullScreen();

    }else if(element.webkitRequestFullscreen ){
        element.webkitRequestFullscreen();

    }

}
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 5, 2016

Member

Could you create a test case on jsfiddle.net or jsbin.com? Also, I assume you mean it works fine in Edge and not the edge mode of IE? (There's no such thing as IE Edge)

Member

mgol commented Apr 5, 2016

Could you create a test case on jsfiddle.net or jsbin.com? Also, I assume you mean it works fine in Edge and not the edge mode of IE? (There's no such thing as IE Edge)

@heporap

This comment has been minimized.

Show comment
Hide comment
@heporap

heporap Apr 5, 2016

I could not create iframe page with allowfullscreen attribute on jsfiddle.net. I made a sample in github pages instead.
http://heporap.github.io/testcase/iframe_fullscreen.html

IE 11 of Windows 8 outputs 128000, and IE Edge of Windows 10 outputs 1920.

heporap commented Apr 5, 2016

I could not create iframe page with allowfullscreen attribute on jsfiddle.net. I made a sample in github pages instead.
http://heporap.github.io/testcase/iframe_fullscreen.html

IE 11 of Windows 8 outputs 128000, and IE Edge of Windows 10 outputs 1920.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 5, 2016

Member

Ref gh-1764 and its PR gh-2401. There is no feature check (it would be pretty much impossible I think) so we're just blindly multiplying by 100 if we see fullscreen mode. Maybe IE11 fixed this?

Member

dmethvin commented Apr 5, 2016

Ref gh-1764 and its PR gh-2401. There is no feature check (it would be pretty much impossible I think) so we're just blindly multiplying by 100 if we see fullscreen mode. Maybe IE11 fixed this?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 5, 2016

Member

I fired up the test case at /test/integration/gh-1764-fullscreen.html and it now fails (click the second button to see) in IE 11 on Windows 10 which means the bug was fixed in IE 11. I don't know if it was fixed in Windows 8.1 & Windows 7 variants of IE 11 (at least in BrowserStack they still have the bug) but it was definitely fixed in Windows 10.

@jacobrossi you said in #2401 (comment) that you're not going to fix this bug and purely based on that comment we landed the workaround... Why was it fixed contrary to that declaration? :(

Member

mgol commented Apr 5, 2016

I fired up the test case at /test/integration/gh-1764-fullscreen.html and it now fails (click the second button to see) in IE 11 on Windows 10 which means the bug was fixed in IE 11. I don't know if it was fixed in Windows 8.1 & Windows 7 variants of IE 11 (at least in BrowserStack they still have the bug) but it was definitely fixed in Windows 10.

@jacobrossi you said in #2401 (comment) that you're not going to fix this bug and purely based on that comment we landed the workaround... Why was it fixed contrary to that declaration? :(

@mgol mgol added this to the 1.12.4/2.2.4 milestone Apr 13, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 13, 2016

Member

I've added this to the 1.12.4/2.2.4 milestone so that we don't lose it. If Microsoft did fix it and the fix is going to stay we should just revert our "fix" and let people deal with it themselves.

I'd still like someone from the IE/Edge team to give more light here. @jacobrossi, @jdalton, could you help us here?

Member

mgol commented Apr 13, 2016

I've added this to the 1.12.4/2.2.4 milestone so that we don't lose it. If Microsoft did fix it and the fix is going to stay we should just revert our "fix" and let people deal with it themselves.

I'd still like someone from the IE/Edge team to give more light here. @jacobrossi, @jdalton, could you help us here?

@mgol mgol added Bug CSS labels Apr 13, 2016

@mgol mgol self-assigned this Apr 13, 2016

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 13, 2016

Member

Yep, I'll poke around. By poke around I mean ping Jacob :P

Member

jdalton commented Apr 13, 2016

Yep, I'll poke around. By poke around I mean ping Jacob :P

@jacobrossi

This comment has been minimized.

Show comment
Hide comment
@jacobrossi

jacobrossi Apr 13, 2016

I'm seeing IE11 on Windows 10 behave incorrectly and Microsoft Edge on Windows 10 behaving correctly. So to me it looks like we only fixed this in Edge.

image
(My screen is a 3000px display with a 2x DPI, so 1500 is the right value here)

jacobrossi commented Apr 13, 2016

I'm seeing IE11 on Windows 10 behave incorrectly and Microsoft Edge on Windows 10 behaving correctly. So to me it looks like we only fixed this in Edge.

image
(My screen is a 3000px display with a 2x DPI, so 1500 is the right value here)

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 13, 2016

Member

@jacobrossi The issue was that IE was setting values 100 times too small so we started to multiply them by 100 if in fullscreen mode in iframes (the code is here). So the fact that you see values 100 times too large in IE means that IE got fixed and our workaround is now broken (this test case uses jQuery 2.2.2 which includes the workaround).

Member

mgol commented Apr 13, 2016

@jacobrossi The issue was that IE was setting values 100 times too small so we started to multiply them by 100 if in fullscreen mode in iframes (the code is here). So the fact that you see values 100 times too large in IE means that IE got fixed and our workaround is now broken (this test case uses jQuery 2.2.2 which includes the workaround).

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 13, 2016

Member

The test case from our repo can be viewed via rawgit.com. If you click the second link in IE 11 you'll see the red background which means our workaround now breaks IE instead of fixing it.

Member

mgol commented Apr 13, 2016

The test case from our repo can be viewed via rawgit.com. If you click the second link in IE 11 you'll see the red background which means our workaround now breaks IE instead of fixing it.

@jacobrossi

This comment has been minimized.

Show comment
Hide comment
@jacobrossi

jacobrossi Apr 13, 2016

Got it. OK and you can confirm that IE11 on Windows 7 works differently?

jacobrossi commented Apr 13, 2016

Got it. OK and you can confirm that IE11 on Windows 7 works differently?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 13, 2016

Member
Member

mgol commented Apr 13, 2016

@jacobrossi

This comment has been minimized.

Show comment
Hide comment
@jacobrossi

jacobrossi Apr 13, 2016

Ok, I'll spin up a patched VM and see.

jacobrossi commented Apr 13, 2016

Ok, I'll spin up a patched VM and see.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 13, 2016

Member

For me it's giving the incorrect results (due to our patch) in IE11.212.10586.0 on Windows 10, which would indicate the bug was fixed at some point in IE11.

Member

dmethvin commented Apr 13, 2016

For me it's giving the incorrect results (due to our patch) in IE11.212.10586.0 on Windows 10, which would indicate the bug was fixed at some point in IE11.

@mgol mgol added the Needs review label Apr 18, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 18, 2016

Member

OK, thanks to Linked Clones in Parallels I could get the IE 11 on a Windows 7 machine running pretty quickly.

The bug existed (as expected) in IE 11.0.9600.17843 but it no longer exists in 11.0.9600.18282 which is the latest version for Windows 7.

@jacobrossi Could you check why it was fixed despite earlier claims? If the fix is going to stay, we'll need to revert the workaround. That would also mean asking BrowserStack to update their IE 11 copies so that we test on IEs behaving consistently.

Member

mgol commented Apr 18, 2016

OK, thanks to Linked Clones in Parallels I could get the IE 11 on a Windows 7 machine running pretty quickly.

The bug existed (as expected) in IE 11.0.9600.17843 but it no longer exists in 11.0.9600.18282 which is the latest version for Windows 7.

@jacobrossi Could you check why it was fixed despite earlier claims? If the fix is going to stay, we'll need to revert the workaround. That would also mean asking BrowserStack to update their IE 11 copies so that we test on IEs behaving consistently.

@mgol mgol changed the title from .width() in fullscreen mode in iframe is too large to .width() in fullscreen mode in iframe is too large in IE 11 Apr 20, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 26, 2016

Member

@jacobrossi Since this seems to be fixed in Windows 7 & Windows 10 versions of IE 11, we're going to assume it's most likely fixed in Windows 8.1 as well and we'll revert our hacky workaround.

We'd still love to hear a confirmation that it was fixed and why it happened so if you could share that, that'd be great.

Member

mgol commented Apr 26, 2016

@jacobrossi Since this seems to be fixed in Windows 7 & Windows 10 versions of IE 11, we're going to assume it's most likely fixed in Windows 8.1 as well and we'll revert our hacky workaround.

We'd still love to hear a confirmation that it was fixed and why it happened so if you could share that, that'd be great.

@mgol mgol closed this in ff1a082 Apr 26, 2016

mgol added a commit that referenced this issue Apr 26, 2016

CSS: Don't workaround the IE 11 iframe-in-fullscreen sizing issues
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

mgol added a commit that referenced this issue Apr 26, 2016

CSS: Don't workaround the IE 11 iframe-in-fullscreen sizing issues
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
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 26, 2016

Member

The fix landed on master (ff1a082), 2.2-stable (fb9adb9) & 1.12-stable (fb9adb9).

Member

mgol commented Apr 26, 2016

The fix landed on master (ff1a082), 2.2-stable (fb9adb9) & 1.12-stable (fb9adb9).

@yuhong

This comment has been minimized.

Show comment
Hide comment
@yuhong

yuhong Jun 11, 2016

Do anyone know exactly which IE11 cumulative update fixed the problem?

yuhong commented Jun 11, 2016

Do anyone know exactly which IE11 cumulative update fixed the problem?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 12, 2016

Member

We didn't get an answer from Microsoft on this so unfortunately, no. If you find out please post that information in the ticket.

Member

dmethvin commented Jun 12, 2016

We didn't get an answer from Microsoft on this so unfortunately, no. If you find out please post that information in the ticket.

@timmywil timmywil removed the Needs review label Aug 15, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.