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

CSS settings leak on .form-input-{sm,lg} .form-control #763

Closed
koukou73gr opened this issue Nov 2, 2014 · 6 comments
Closed

CSS settings leak on .form-input-{sm,lg} .form-control #763

koukou73gr opened this issue Nov 2, 2014 · 6 comments
Labels
Milestone

Comments

@koukou73gr
Copy link

With version tagged as 3.3.0, in lines 328 and 330 of bootstrap/_forms.scss the selector changed to also include .form-input-{sm,lg} .form-control. That is:

@include input-size('.input-sm, .form-group-sm .form-control', $input-height-small, $padding-small-vertical, $padding-small-horizontal, $font-size-small, $line-height-small, $input-border-radius-small);
@include input-size('.input-lg, .form-group-lg .form-control', $input-height-large, $padding-large-vertical, $padding-large-horizontal, $font-size-large, $line-height-large, $input-border-radius-large);

However, in input-size mixin, the selector string is also used like this:

select#{$selector} and textarea#{$selector}

which ultimately expand to eg:

select.input-sm, .form-group-sm .form-control

which causes the CSS for select.input-sm to also apply in every .form-group-sm .form-control.

I believe this is unintentional. It is easily overridden but I guess you guys should fix this.

This also exists in the LESS version.

Thanks,
-Kostas

@koukou73gr koukou73gr changed the title CSS settings leak on .form-input-lg .form-control CSS settings leak on .form-input-{sm,lg} .form-control Nov 2, 2014
@glebm glebm added the bug label Nov 13, 2014
glebm added a commit that referenced this issue Nov 14, 2014
before:

    +input-size('.input-sm, .form-group-sm .form-control', ...)

after:

    +input-size('.input-sm', ...)
    +input-size('.form-group-sm .from-control', ...)
@glebm
Copy link
Member

glebm commented Nov 14, 2014

@koukou73gr @caleb Can you guys confirm whether the commit above fixes the issue? 🎉

@caleb
Copy link

caleb commented Nov 14, 2014

I haven't tested it, but I will.

I think that this still has the problem of generating css that doesn't make sense (see my bug on bootstrap: twbs/bootstrap#15074)

This is some of the CSS generated from the LESS version of input-size

textarea.input-lg,
textarea.form-group-lg .form-control,
select[multiple].input-lg,
select[multiple].form-group-lg .form-control {
  height: auto;
}

It doesn't seem to make sense that this would target textarea.form-group-lg and select[multiple].form-group-lg.

Wouldn't the form-control class be on the textarea and select[multiple]?

This shows the bug: http://jsbin.com/nomobegivu/1/edit?html,css,output

The textarea has rows=20 set, but the output has a set height because the height: auto rule listed above isn't being applied. Remove the form-group-lg class and the text area goes back to its correct height.

At any rate, your patch looks like it will solve part of the problem.

@glebm
Copy link
Member

glebm commented Nov 14, 2014

@caleb Please report the issue against the LESS version, thank you

@caleb
Copy link

caleb commented Nov 14, 2014

@glebm Ok:

twbs/bootstrap#15074

@koukou73gr
Copy link
Author

@glebm, would your change to less_conversion.rb change how the input-size mixin gets implemented in the SASS version? If it does, could you paste the new implementation somewhere please?

Because as it stands, just me splitting the selectors in the @include input-size calls would partially solve the problem, like @caleb said, as it would still target, say:

textarea.form-group-sm .form-control

(which I suppose does not exist anywhere intentionally...) while it should probably target:

.form-group-sm textarea.form-control

(or nothing at all...).

@glebm
Copy link
Member

glebm commented Nov 15, 2014

@koukou73gr Sass version aims to be 100% compatible with the LESS version (bugs included), so the actual issue has to be fixed upstream. Splitting the selector results in the CSS closer to the LESS version, because this is how the & selector behaves in LESS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants