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

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

Merged
merged 9 commits into from
Nov 21, 2018
Merged

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

merged 9 commits into from
Nov 21, 2018

Conversation

webmandesign
Copy link
Contributor

@webmandesign 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.

@tofumatt tofumatt self-requested a review August 21, 2018 18:28
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@webmandesign webmandesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member

@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
Copy link
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 a team November 16, 2018 20:32
@youknowriad youknowriad added this to the 4.5 milestone Nov 19, 2018
@catehstn
Copy link

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
@mtias mtias added [Type] Bug An existing feature does not function as intended Browser Issues Issues or PRs that are related to browser specific problems labels Nov 19, 2018
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

kjellr commented Nov 20, 2018

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

@jasmussen
Copy link
Contributor

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

@webmandesign
Copy link
Contributor Author

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

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@catehstn
Copy link

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

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
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
Copy link
Contributor

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.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, works for me! :shipit:

@tofumatt tofumatt dismissed jasmussen’s stale review November 21, 2018 13:50

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

@tofumatt tofumatt merged commit 53e4c40 into WordPress:master Nov 21, 2018
@kjellr
Copy link
Contributor

kjellr commented Nov 21, 2018

🎉 Thanks, everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IE11: cover image text and inline toolbar misplaced
10 participants