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

Custom form control box shadow missing on focus when shadows are enabled #26802

Closed
jdanil opened this issue Jul 4, 2018 · 4 comments · Fixed by #30957
Closed

Custom form control box shadow missing on focus when shadows are enabled #26802

jdanil opened this issue Jul 4, 2018 · 4 comments · Fixed by #30957
Labels

Comments

@jdanil
Copy link
Contributor

jdanil commented Jul 4, 2018

When shadows are enabled in bootstrap, focus box shadows are missing for certain form control input states.

For example...

$enable-shadows: true;

// _variables.scss
$enable-shadows: false !default;
$custom-checkbox-indicator-indeterminate-box-shadow: none !default;

// mixins/_box-shadow.scss
@mixin box-shadow($shadow...) {
  @if $enable-shadows {
    box-shadow: $shadow;
  }
}

// _custom-forms.scss
.custom-checkbox {
  .custom-control-input:indeterminate ~ .custom-control-label {
    &::before {
      @include box-shadow($custom-checkbox-indicator-indeterminate-box-shadow);
    }
  }
}

Testing in SassMeister, this evaluates to...

.custom-checkbox .custom-control-input:indeterminate ~ .custom-control-label::before {
  box-shadow: none;
}

Since .custom-checkbox .custom-control-input:indeterminate ~ .custom-control-label::before is more specific, it overrides the focus box shadow in .custom-control-input:focus ~ .custom-control-label::before.

This affects indeterminate checkboxes and active inputs. checked appears to be okay, as the focus styling is declared after it.

Can I suggest that the default values of the following variables be updated from none to null, so that they don't override the focus styling unless they are set explicitly?
$custom-control-indicator-checked-box-shadow
$custom-control-indicator-active-box-shadow
$custom-control-indicator-indeterminate-box-shadow
Also the focus styling for custom control input should be declared after active, and the specificity of the rule should be increased so that it applies to indeterminate checkboxes if the box shadows are modified by consumers.

Happy to contribute if you're OK with the proposed solution.

@mdo mdo added this to Inbox in v5 via automation Dec 17, 2018
@mdo mdo removed this from Inbox in v5 Feb 3, 2020
@mdo
Copy link
Member

mdo commented Feb 3, 2020

Can you verify this is an issue in our latest v5 work from master? We've combined our normal checks and custom checks into a single set of form controls.

@ffoodd
Copy link
Member

ffoodd commented Jun 2, 2020

This does not apply to v5, since only :focus gets a box-shadow.

As a matter of fact, I think this should be fixed by defining (and applying) a preferred pseudo-class order. :focus should come after :indeterminate, in that case.

Remember "LoVe HAte"?—which was later extended to "LoVe Fuck HAte" to include :focus?

For form controls, I'd go with:

  • :required, :optional,
  • :disabled, [readonly],
  • :valid, :invalid,
  • :indeterminate,
  • :checked,
  • :hover,
  • :focus,
  • :active.

It may even allow us to drop a few overrides here and there.

Without going this far, I think moving :focus after :indeterminate would be safe.

@ffoodd ffoodd removed the v5 label Jun 3, 2020
@ffoodd ffoodd linked a pull request Jun 3, 2020 that will close this issue
@ffoodd
Copy link
Member

ffoodd commented Jun 3, 2020

FWIW, @jdanil's suggestion is by far the best way to solve this :)

@mdo mdo added this to Inbox in v4.5.1 via automation Jun 3, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Jun 4, 2020

Fixed by #30957.

@XhmikosR XhmikosR removed this from Inbox in v4.5.1 Jun 4, 2020
@ffoodd ffoodd closed this as completed Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants