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 toolbars positionning in IE11 #17894

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Fix toolbars positionning in IE11 #17894

merged 1 commit into from
Oct 14, 2019

Conversation

youknowriad
Copy link
Contributor

This PR fixes a visual glitch in ie11 where toolbar segments in block toolbars appear one per line instead of part of the same "line".

I'm interested in testing this PR in other browsers to ensure there's no unwanted side-effect.

Testing instructions

Test block toolbars in Chrome/Firefox in several situations/alignments... and ensure it displays properly.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Browser Issues Issues or PRs that are related to browser specific problems labels Oct 11, 2019
@youknowriad youknowriad self-assigned this Oct 11, 2019
@youknowriad youknowriad requested a review from jasmussen October 11, 2019 07:51
@jasmussen
Copy link
Contributor

Right off the bat it has a cost for non IE. This is Chrome:

Screenshot 2019-10-11 at 11 14 13

We have done some tricks in other code like this:

.block-editor-block-toolbar__slot {
	// Required for IE11.
	display: inline-block;

	// IE11 doesn't read rules inside this query. They are applied only to modern browsers.
	@supports (position: sticky) {
		display: inline-flex;
	}
}

@jasmussen
Copy link
Contributor

Screenshot 2019-10-11 at 11 14 13

What am I looking for in IE11? This is after setting it back to flex.

@youknowriad
Copy link
Contributor Author

@jasmussen try to the image block instead (upload an image and check the toolbar)

@jasmussen
Copy link
Contributor

I can't test anymore because my virtualbox started crashing hard, so hard the mac reboots :(

@jorgefilipecosta can you look?

However as noted, the inline-flex rule breaks it for me in Chrome. You might want to do the "@supports" trick.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Oct 11, 2019

Hi @youknowriad, I'm also noticing the problem @jasmussen referred to in other browsers.
This problem is complex:
We have this I.E specific toolbar styles:

 .block-editor-block-toolbar__slot {
-	// Required for IE11.
-	display: inline-block;
-
-	// IE11 doesn't read rules inside this query. They are applied only to modern browsers.
-	@supports (position: sticky) {
-		display: inline-flex;
-	}
+	display: inline-flex;
 }

If we remove and use inline-flex even on IE things start to work well in some blocks e.g: media & text but the toolbar of paragraphs starts breaking on IE.

The fix you proposed seems an improvement on IE (there is only some unalignment) maybe we can target the fix for IE only?

@youknowriad
Copy link
Contributor Author

The fix you proposed seems an improvement on IE (there is only some unalignment) maybe we can target the fix for IE only?

Yes, if we don't find anything better, we should do that.

@youknowriad
Copy link
Contributor Author

I only applied the fix to IE11, I think now it's decent in all browsers.

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.

LGTM 👍

@youknowriad youknowriad merged commit 6700f98 into master Oct 14, 2019
@youknowriad youknowriad deleted the fix/toolbars-ie11 branch October 14, 2019 13:49
@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
gziolo pushed a commit that referenced this pull request Oct 15, 2019
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.

3 participants