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

4.1.2 breaks alignment of inline SVG icons #26878

Closed
nifte opened this issue Jul 14, 2018 · 12 comments
Closed

4.1.2 breaks alignment of inline SVG icons #26878

nifte opened this issue Jul 14, 2018 · 12 comments
Labels
Projects

Comments

@nifte
Copy link

@nifte nifte commented Jul 14, 2018

A new change in 4.1.2 causes inline SVG icons to become unaligned.

The code in question, /scss/_reboot.scss, line 277:

svg:not(:root) {
    overflow: hidden;
    vertical-align: middle; <--- (Line 277)
}

The issue can be seen in the following examples:
Version 4.1.1 (Icons aligned)
Version 4.1.2 (Icons not aligned)

@mdo mdo added css v4 labels Jul 14, 2018
@mdo

This comment has been minimized.

Copy link
Member

@mdo mdo commented Jul 14, 2018

Ah, yes, the selector is too strong and overrides classes.

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 18, 2018

Removing :not(:root) and overflow: hidden; is a solution?

svg {
  vertical-align: middle;
}

Because,

svg:not(:root) {	
  overflow: hidden;	
}

is for IE9, so we can remove it.
ref: necolas/normalize.css@004d58b#diff-04c6e90faac2675aa89e2176d2eec7d8L73

And...

svg:not(:root) {
  vertical-align: middle;
}

SVG can be the root element, but I'm not sure you need :not(:root). Even if that is the root, I don't think that vertical-align: middle will cause something.

@mdo

This comment has been minimized.

Copy link
Member

@mdo mdo commented Jul 18, 2018

Yup, I agree with that. Feel free to open a PR @ysds, or I can later this week. Hoping to do a 4.1.3 next week to fix some issues.

@mdo mdo added this to Inbox in v4.1.3 via automation Jul 18, 2018
@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 18, 2018

@mdo sure. Which is preferred?

img {
  vertical-align: middle;
  border-style: none; // Remove the border on images inside links in IE 10-.
}

svg {
  vertical-align: middle;
}

or

img, svg {
  vertical-align: middle;
}

img {
  border-style: none; // Remove the border on images inside links in IE 10-.
}
@MartijnCuppens

This comment has been minimized.

Copy link
Member

@MartijnCuppens MartijnCuppens commented Jul 18, 2018

I would say option 1?

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 20, 2018

I found @XhmikosR 's comment #26296 (comment) and https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7226597/.

Unfortunately, that bug still exists in IE11. I don't have a environment to test with IE10.

https://codepen.io/anon/pen/pZNaqN

image

@Herst

This comment has been minimized.

Copy link
Contributor

@Herst Herst commented Jul 20, 2018

@ysds

is for IE9, so we can remove it.

It is not only for IE9, it is necessary for all IE versions.

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 20, 2018

@Herst Looks like it :)

@MartijnCuppens

This comment has been minimized.

Copy link
Member

@MartijnCuppens MartijnCuppens commented Jul 20, 2018

Is there any reason why we add :not(:root)? We can just leave that out and specificity problem is solved? Just add overflow: hidden to all svg? And as @ysds pointed out:

SVG can be the root element, but I'm not sure you need :not(:root). Even if that is the root, I don't think that vertical-align: middle will cause something.

This demo works in IE10 and IE11: https://codepen.io/MartijnCuppens/pen/QBGoKd?editors=1100

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 20, 2018

Is there any reason why we add :not(:root)?

Yeah, I have the same doubt. I can not imagine the scenario where Bootstrap is used in SVG documents. like:

<svg xmlns="http://www.w3.org/2000/svg">
  <style>
    @import url(bootstrap.css);
  </style>
  <rect .../>
</svg>

ref: https://www.w3.org/TR/SVG2/styling.html#LinkElement

or

<svg xmlns="http://www.w3.org/2000/svg">
  <style>
    /*!
     * Bootstrap v4.1.2 (https://getbootstrap.com/)
     ...*/
  </style>
  <rect .../>
</svg>

@mdo Do you expect any usage that Bootstrap is used in SVG documents? If not, the :not(:root) could be remove. It is not necessary to verify and waste time.

@mdo

This comment has been minimized.

Copy link
Member

@mdo mdo commented Jul 20, 2018

Definitely don't expect that usage :).

@ysds

This comment has been minimized.

Copy link
Member

@ysds ysds commented Jul 20, 2018

@mdo thanks :) I’m going to update my PR.

ysds added a commit to ysds/bootstrap that referenced this issue Jul 20, 2018
* `svg:not(:root)` specificity is very high (necolas/normalize.css#718)
* Bootstrap do not support SVG documents (See twbs#26878)
@mdo mdo moved this from Inbox to Ready to merge in v4.1.3 Jul 20, 2018
@mdo mdo closed this in #26927 Jul 20, 2018
v4.1.3 automation moved this from Ready to merge to Shipped Jul 20, 2018
mdo added a commit that referenced this issue Jul 20, 2018
* `svg:not(:root)` specificity is very high (necolas/normalize.css#718)
* Bootstrap do not support SVG documents (See #26878)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v4.1.3
  
Shipped
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.