Skip to content
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

Fix .embed-responsive class in flex container #19739

Merged
merged 1 commit into from
Dec 31, 2016
Merged

Fix .embed-responsive class in flex container #19739

merged 1 commit into from
Dec 31, 2016

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Apr 15, 2016

Firefox have trouble displaying .embed-responsive in a flex container. Firefox developer said that Firefox has the correct implementation, and other browsers which show .embed-responsive normally were wrong.

This RP fixes the problem regardless of browsers' implementation by move padding into pseudo class ::before, as the padding in pseudo class will be resolved against block .embed-responsive instead of flex parent.

Spec: http://dev.w3.org/csswg/css-flexbox/#item-margins

For blocks in CSS, % padding values are always resolved against the containing block width -- but in flexbox, they're resolved against the length of the containing block's corresponding dimension (width or height).

Authors should avoid using percentages in paddings or margins on flex items entirely, as they will get different behavior in different browsers.

Safari / WebKit: https://bugs.webkit.org/show_bug.cgi?id=113519
Chrome / Blink: https://bugs.chromium.org/p/chromium/issues/detail?id=229568
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=958714

@cvrebert cvrebert changed the title Port #19734 to v4 Fix .embed-responsive class in flex container Apr 15, 2016
@cvrebert
Copy link
Collaborator

CC: @mdo

@ntkme
Copy link
Contributor Author

ntkme commented Jul 10, 2016

On Firefox, the video in the sample below will show as 0 height, where the patched version will correctly display the height calculated based on the container width.

Bootstrap 3.3.6: http://codepen.io/pen/AXQVLB
Bootstrap 4.0.0-alpha.2: http://codepen.io/pen/bZoLwx
Patch: http://codepen.io/pen/aNYGYv

In my tests, this patch works for all major browsers that Bootstrap covers.


Can someone at least confirm this bug?

@ntkme
Copy link
Contributor Author

ntkme commented Jul 10, 2016

On the latest Safari, I can use the same code to reproduce a similar yet different problem now.

Calculated dimension in each browser without the patch (viewpoint width = 1920px):

Chrome 51.0.2704.106
.embed-responsive:
   width = 1920px (with an additional `width: 100%;` statement on `.embed-responsive`)
   height = 1080px
.embed-responsive-item:
   width = 1920px
   height = 1080px
Firefox 47.0.1
.embed-responsive:
   width = 1920px (with an additional `width: 100%;` statement on `.embed-responsive`)
   height = 0
.embed-responsive-item:
   width = 1920px
   height = 0
Safari 9.1.1 (11601.6.17)
.embed-responsive:
   width = 1920px (no need for `width: 100%;`)
   height = 1080px
.embed-responsive-item:
   width = 300px (which is the default for iframe; however it looks like width 0 and nothing is displayed)
   height = 1080px

I guess there is also a bug in WebKit.

@mdo mdo added this to the v4.0.0-alpha.6 milestone Nov 27, 2016
@mdo mdo merged commit 6a5a83e into twbs:v4-dev Dec 31, 2016
@mdo mdo mentioned this pull request Dec 31, 2016
@ntkme ntkme deleted the v4-responsive-embed branch December 31, 2016 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants