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

Fixing IE11 flexbox alignment when min-width is set #9196

Merged
merged 9 commits into from Nov 21, 2018

Conversation

@webmandesign
Contributor

webmandesign commented Aug 21, 2018

Description

Fixes #5791

For more info on the fix please see philipwalton/flexbugs#231

How has this been tested?

In Internet Explorer 11 on Windows 10 by applying the CSS directly in code inspector of the browser.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

webmandesign and others added some commits Aug 21, 2018

Updating the IE11 alignment fix
Updating the fix to use philipwalton/flexbugs#231 (comment) approach for cases with larger cover image text.

@tofumatt tofumatt self-requested a review Aug 21, 2018

@tofumatt

I think it would be helpful to have before/after screenshots for this PR; I'm not exactly sure what it's meant to fix. When I checked out this branch in IE11 it seemed similar to the screenshots in the issue, and
#5791 (comment) still happened for me.

@webmandesign

@tofumatt Thanks for the change.

I have put together a video of the fix applied directly onto installed Gutenberg plugin stylesheets. Check at https://www.dropbox.com/s/b9orj1mcltd3zaw/gutenberg-ie11-fixing-flex-align.mp4

Tested (and recorded) on Windows 10, IE11.

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Sep 19, 2018

@webmandesign, I think I see the disconnect. In my testing, this fix is working in IE11 on Windows 10 but not in IE11 on Windows 7.

@kjellr

This comment has been minimized.

Contributor

kjellr commented Nov 16, 2018

I think I see the disconnect. In my testing, this fix is working in IE11 on Windows 10 but not in IE11 on Windows 7.

@brandonpayton Good catch. Turned out the :after element required display: block to get it to appear that context. @webmandesign, I went ahead and added that, and fixed the conflict with master as well.

I think this should be good to go. Here are screenshots:

Before:

Editor:
screen shot 2018-11-16 at 3 09 30 pm

Front end:
screen shot 2018-11-16 at 3 11 12 pm

After:

Editor:
screen shot 2018-11-16 at 3 07 59 pm

Front end:
screen shot 2018-11-16 at 3 11 41 pm

Adding this didn't seem to have any negative effects on non-IE11 browsers as far as I could tell. That said, this could use one more set of 👀 before merging.

@kjellr kjellr requested a review from WordPress/gutenberg-core Nov 16, 2018

@youknowriad youknowriad added this to the 4.5 milestone Nov 19, 2018

@catehstn

This comment has been minimized.

catehstn commented Nov 19, 2018

No rush to get this merged in 4.5, so moving to 4.6.
@jasmussen maybe you could take a pass when you have a moment? Thank you!

@catehstn catehstn modified the milestones: 4.5, 4.6 Nov 19, 2018

@jasmussen

I can confirm that this fixes the alignment of the text in IE11. Nice work!

However it introduces a regression with the placeholder for the cover block:

screenshot 2018-11-20 at 10 17 49

It's no longer vertically centered there. This is in all browsers.

However the following fixes it:

	// Prevents misplaced flex alignment in IE11.
	&::after {
		display: block;
		content: "";
		font-size: 0;
		min-height: inherit;

		// IE doesn't support flex so omit that.
		@supports (position: sticky) {
			content: none;
		}
	}

If you add that, I think we can ship this. Thanks for the PR!

@kjellr

This comment has been minimized.

Contributor

kjellr commented Nov 20, 2018

Good catch, @jasmussen! I pushed that change in 0954f43. Mind giving it another test?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

I already tested this. I'm inclined to either approve, or get @tofumatt sanity check.

@webmandesign

This comment has been minimized.

Contributor

webmandesign commented Nov 20, 2018

@kjellr @jasmussen Thank you guys for improving the code and making it work!

@tofumatt

I think we need some context for the CSS hack but otherwise code seems fine to me.

&::after {
display: block;
content: "";
font-size: 0;

This comment has been minimized.

@tofumatt

tofumatt Nov 20, 2018

Member

That seems weird, could you include context for this fix in the CSS so future developers can understand this hack?

This comment has been minimized.

@kjellr

kjellr Nov 20, 2018

Contributor

Agreed. To be honest though, I'm not sure why this works. 😄

Maybe something like this would be more informative...

// Using flexbox without an assigned height property breaks vertical center alignment in IE11. 
// Apphending an empty ::after element corrects this issue.

@webmandesign do you happen to know more details on the fix? There isn't much context here.

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Nov 20, 2018

Contributor

Appending the pseudo element is sort of like providing a float clearing element; it tricks IE into giving the element an implicit height rather than collapsing to zero and preventing the vertical alignment.

This comment has been minimized.

@kjellr

kjellr Nov 20, 2018

Contributor

Thanks for clarifying that, @chrisvanpatten. I've added a clearer description. 👍

kjellr added some commits Nov 20, 2018

@catehstn

This comment has been minimized.

catehstn commented Nov 21, 2018

@tofumatt I think your comments have been addressed, can we get this approved and merged now?

@jorgefilipecosta

In my tests on IE the placeholder problem referred by @jasmussen still happens:
screenshot 2018-11-21 at 12 06 21

It does not happen in other browsers. I think we need a rule in the editor styles to avoid this behavior on the placeholder. The frond styles look correct 👍

@kjellr

This comment has been minimized.

Contributor

kjellr commented Nov 21, 2018

@jorgefilipecosta: That happens for all block placeholders in IE11, and isn't a regression from this PR. I don't believe we have a PR open to fix that this point, but the issue tracking it is #8607

The fix could likely be similar, but this PR is meant to just fix the vertical text alignment within the (non-placeholder) cover image block itself (both in the front and back end).

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 21, 2018

I think I agree with Kjell on this one. Unless we can find a quick fix for vertically centering the text in all placeholders in IE11, probably good to ship this as is.

IE 11 has to_work_, but it doesn't have to be perfect.

@tofumatt

Awesome, works for me! :shipit:

You said it yourself: good to ship as-is :-)

@tofumatt tofumatt merged commit 53e4c40 into WordPress:master Nov 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kjellr

This comment has been minimized.

Contributor

kjellr commented Nov 21, 2018

🎉 Thanks, everyone!

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fixing IE11 flexbox alignment when min-width is set (WordPress#9196)
* Fixing IE11 flexbox alignment when min-width is set

For more info on the fix please see philipwalton/flexbugs#231

* Updating the IE11 alignment fix

Updating the fix to use philipwalton/flexbugs#231 (comment) approach for cases with larger cover image text.

* Documenting the code

* chore: Tweak code style

* Add display: block, remove Github ticket mention.

* Ignore the IE11 cover image fix in browsers that support flex.

* Add a better description of the IE fix.

* Fix typo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment