Hide validation icons from multiple selects by tagliala · Pull Request #33598 · twbs/bootstrap

tagliala · 2021-04-09T14:49:28Z

Implementation provided in #33411 does not take into account that some
Operating Systems may display a vertical scrollbar in the multiple
select field

This implementation will hide the validation icons from multiple select
fields, just like Bootstrap 4 does.

Fix: #33591

Demo fiddles

BS4: https://jsfiddle.net/tagliala/y1t4cuqg/show
BS5: https://jsfiddle.net/tagliala/s45v1hLy/show

Before (Win 10 / Chrome 89)

Main

5.0.0.beta3

After (Win 10 / Chrome 89)

Bootstrap 4 vs Bootstrap 5

tagliala · 2021-04-09T14:50:11Z

Hi @tkrotoff this mimics BS4 approach, any chance to take a look at this PR?

tkrotoff

What about:

  select { <=====
    @include form-validation-state-selector($state) {
      &[multiple],
      &[size]:not([size="1"]) {
        padding-right: $form-select-padding-x;
        background-image: none;
      }
    }
  }

instead of

  .form-select {
    @include form-validation-state-selector($state) {
      &[multiple],
      &[size]:not([size="1"]) {
        padding-right: $form-select-padding-x;
        background-image: none;
      }
    }
  }

This way it fixes also your comment: #33411 (comment)

mdo · 2021-04-09T16:40:48Z

There should be no difference in moving from .form-select to select because we only have one class for this in v5. I’d stick with the class selector. I’m also good with moving in this direction to hide the icon again.

tkrotoff · 2021-04-09T17:02:40Z

I see several solutions depending on whether #33411 (comment) should be treated or not (is Bootstrap 5 supposed to work if the user writes select.form-control instead of .form-select?)

Modify scss/mixins/_forms.scss

1- solution proposed in this PR: .form-select { @include form-validation-state-selector ... }, doesn't solve Fix validation feedback icon in select multiple #33411 (comment)
2- the same code with select instead .form-select, solves Fix validation feedback icon in select multiple #33411 (comment)

Modifying _forms.scss has the drawback to have almost the same code inside _form-select.scss but the code can be factorized (something like remove-select-multiple-background-image())

Modify scss/forms/_form-select.scss

3- add !important here:

bootstrap/scss/forms/_form-select.scss

Lines 38 to 39 in a48a29a

padding-right: $form-select-padding-x;

background-image: none;

, doesn't solve Fix validation feedback icon in select multiple #33411 (comment)

4- add

select {
  &[multiple],
  &[size]:not([size="1"]) {
    padding-right: $form-select-padding-x !important;
    background-image: none !important;
  }
}

solves Fix validation feedback icon in select multiple #33411 (comment)

if we modify _form-select.scss this way, we don't need the same code inside _forms.scss

The problem with !important is that the user cannot set its own background-image to the select/.form-select.

I'm not a affiliated with Bootstrap, just my 2€

tagliala · 2021-04-09T18:56:59Z

given @ffoodd's comment at #33411 (comment), would it be a better solution to only apply style to .form-select without multiple or with [size="1"]? Is there an edge case that I'm missing?

      @if $enable-validation-icons {
        &:not([multiple]),
        &[size="1"] {
          padding-right: $form-select-feedback-icon-padding-end;
          background-image: escape-svg($form-select-indicator), escape-svg($icon);
          background-position: $form-select-bg-position, $form-select-feedback-icon-position;
          background-size: $form-select-bg-size, $form-select-feedback-icon-size;
        }
      }

tkrotoff · 2021-04-10T21:55:24Z

@tagliala I've tested &:not([multiple]), &[size="1"] (nicer approach than my suggestions!) => does not work in all cases:

This works (but does not solve #33411 (comment)):

  .form-select {
      ...

      @if $enable-validation-icons {
        &:not([multiple]):not([size]),
        &[size="1"] {
          padding-right: $form-select-feedback-icon-padding-end;
          background-image: escape-svg($form-select-indicator), escape-svg($icon);
          background-position: $form-select-bg-position, $form-select-feedback-icon-position;
          background-size: $form-select-bg-size, $form-select-feedback-icon-size;
        }
      }

      ...
    }

Code to test:

<h2>select form-control (supported by Bootstrap 4, should it be supported by Bootstrap 5?)</h2>

<div class="mb-3">
  <select class="form-control is-valid">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select</h2>

<div class="mb-3">
  <select class="form-select is-valid">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select multiple</h2>

<div class="mb-3">
  <select class="form-select is-valid" multiple>
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select size="1"</h2>

<div class="mb-3">
  <select class="form-select is-valid" size="1">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

<h2>form-select size="2"</h2>

<div class="mb-3">
  <select class="form-select is-valid" size="2">
    <option>1</option>
    <option>2</option>
    <option>3</option>
    <option>4</option>
    <option>5</option>
  </select>
</div>

Screenshot with untouched v5 beta3:

btw you will notice that Bootstrap does not respect size="2" (it displays a bit more than 2 lines), screenshot with regular select size="2":

tagliala · 2021-04-11T12:51:05Z

Thanks, of course I was missing something

btw, &:not([multiple]):not([size]), &[size="1"] { does not cover multiple size="1"

          <form>
            <code>form-select</code>
            <div class="mb-3">
              <select class="form-select is-valid">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select size="1"</code>
            <div class="mb-3">
              <select class="form-select is-valid" size="1">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select size="2"</code>
            <div class="mb-3">
              <select class="form-select is-valid" size="2">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple>
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple size="1"</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple size="1">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
            <code>form-select multiple size="2"</code>
            <div class="mb-3">
              <select class="form-select is-valid" multiple size="2">
                <option>1</option>
                <option>2</option>
                <option>3</option>
              </select>
            </div>
          </form>

The correct inversion should be

&:not([multiple]):not([size]),
&:not([multiple])[size="1"] {

Bootstrap 4 vs Bootstrap 5

tkrotoff · 2021-04-11T13:10:28Z

👍 You're right about multiple size="1": it displays a scrollbar and thus there should be no feedback icon and no indicator icon.

Implementation provided in twbs#33411 does not take into account that some Operating Systems may display a vertical scrollbar in the multiple select field This implementation will hide the validation icons from multiple select fields, just like Bootstrap 4 does. Fix: twbs#33591

mdo

Wow, appreciate the effort in this PR! The selector specificity isn't my favorite, but it solves the problem well. I think we can ship this.

ffoodd

Agreed, thanks for the discussions and researches!

tkrotoff reviewed Apr 9, 2021

View reviewed changes

tagliala force-pushed the bugfix/hide-validation-for-multiple-selects branch from 26a26c2 to 88f5834 Compare April 11, 2021 15:02

tkrotoff approved these changes Apr 11, 2021

View reviewed changes

tagliala force-pushed the bugfix/hide-validation-for-multiple-selects branch from 88f5834 to 491095a Compare April 13, 2021 08:59

tagliala marked this pull request as ready for review April 13, 2021 09:02

tagliala requested a review from a team as a code owner April 13, 2021 09:02

mdo added css v5 labels Apr 13, 2021

mdo approved these changes Apr 13, 2021

View reviewed changes

ffoodd approved these changes Apr 13, 2021

View reviewed changes

mdo merged commit 6fe75df into twbs:main Apr 14, 2021

tagliala deleted the bugfix/hide-validation-for-multiple-selects branch April 14, 2021 07:00

This was referenced Jul 27, 2024

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/AspNetCore#34

Open

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/ElastiQuill#61

Open

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/AspNetCore#35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Hide validation icons from multiple selects#33598

Hide validation icons from multiple selects#33598
mdo merged 1 commit into
twbs:mainfrom
tagliala:bugfix/hide-validation-for-multiple-selects

tagliala commented Apr 9, 2021 •

edited

Loading

Uh oh!

tagliala commented Apr 9, 2021

Uh oh!

tkrotoff left a comment •

edited

Loading

Uh oh!

mdo commented Apr 9, 2021

Uh oh!

tkrotoff commented Apr 9, 2021

Uh oh!

tagliala commented Apr 9, 2021

Uh oh!

tkrotoff commented Apr 10, 2021 •

edited

Loading

Uh oh!

tagliala commented Apr 11, 2021 •

edited

Loading

Uh oh!

tkrotoff commented Apr 11, 2021 •

edited

Loading

Uh oh!

mdo left a comment

Uh oh!

ffoodd left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

tagliala commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo fiddles

Before (Win 10 / Chrome 89)

Main

5.0.0.beta3

After (Win 10 / Chrome 89)

Bootstrap 4 vs Bootstrap 5

Uh oh!

tagliala commented Apr 9, 2021

Uh oh!

tkrotoff left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdo commented Apr 9, 2021

Uh oh!

tkrotoff commented Apr 9, 2021

Modify scss/mixins/_forms.scss

Modify scss/forms/_form-select.scss

Uh oh!

tagliala commented Apr 9, 2021

Uh oh!

tkrotoff commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tagliala commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bootstrap 4 vs Bootstrap 5

Uh oh!

tkrotoff commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdo left a comment

Choose a reason for hiding this comment

Uh oh!

ffoodd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tagliala commented Apr 9, 2021 •

edited

Loading

tkrotoff left a comment •

edited

Loading

tkrotoff commented Apr 10, 2021 •

edited

Loading

tagliala commented Apr 11, 2021 •

edited

Loading

tkrotoff commented Apr 11, 2021 •

edited

Loading