Fix buttons :focus state styles

[mattez]

Small change which fix two issues:

1] Buttons :hover and :focus state shares styles now.

As you expect and as it is in BS3, :focus state is same as :hover state only extended by outline.

Before: .btn with :focus appears as normal button state with 'outline':

After: .btn with :focus looks like :hover state with 'outline':

2] Buttons :focus text color on 'a.btn' is now consistent with others '.btn'.

If you somewhere define specific color (e.g. $danger) for :focus state on <a> then ...

Before: This specific color is presented in :focus state on .btn made from <a>:

After: Specific color is overridden with right .btn color:

[MartijnCuppens]

:focus styles should be in sync with .focus styles, so it might be better to copy these lines:

bootstrap/scss/mixins/_buttons.scss

Lines 14 to 16 in 21e39ce

	color: color-yiq($hover-background);
	@include gradient-bg($hover-background);
	border-color: $hover-border;

to:

bootstrap/scss/mixins/_buttons.scss

Lines 19 to 21 in 21e39ce

	&:focus,
	&.focus {
	// Avoid using mixin so we can pass custom focus shadow properly

Maybe we should add this here:

bootstrap/scss/_buttons.scss

Line 25 in 21e39ce

&.focus {

(wait a sec before applying any changes, would like to have a second opinion from @mdo or @ysds)

[mattez]

@MartijnCuppens I agree :focus must be in sync with .focus. Very good point, thank you, I missed that. I'm for copy styles to:

bootstrap/scss/mixins/_buttons.scss

Lines 19 to 21 in 21e39ce

	&:focus,
	&.focus {
	// Avoid using mixin so we can pass custom focus shadow properly

One idea come to my mind: If there should be always available .focus styles where is :focus across the whole Bootstrap, if this is some kind of standard, then .focus may should be added to hover mixins. Like e.g. this:

bootstrap/scss/mixins/_hover.scss

Lines 16 to 21 in 21e39ce

	@mixin hover-focus {
	&:hover,
	&:focus {
	@content;
	}
	}

to:

@mixin hover-focus { 
   &:hover, 
   &:focus,
   &.focus
   { 
     @content; 
   } 
} 

etc. The others should be changed accordingly (:active = .active).
But this is may be to much right?

I look forward to your and @mdo / @ysds views.

[mdo]

I’m against adding a .focus across the board. I can’t think of why we even have it on buttons honestly—everything can be done with :focus. As for :active/.active`, it’s an unfortunately named class that means “selected” more than “active”. The pairing of the names is circumstantial.

[mattez]

@mdo
I searched a bit, and I found a reason for the existence of the .focus class on buttons.
Historically, it has been added here #13907
Actual Button plugin still adding .focus class on focus.

So @MartijnCuppens is right - :focus should still remain in sync with .focus class. Until someone revises whether it is still needed do add .focus on focus.

So I'm going to prepare new commit where styles will be copied to :focus, .focus definition.

[MartijnCuppens]

I'm not really convinced :focus and :hover states should have the same background. I like the current behaviour where I get :hover feedback when I hover a focused button. But I do like the color fix to prevent the behaviour in example 2.

I would go for adding color: color-yiq($background); to the focus state and move the hover styles under the focus block (to keep the color change on hover if needed). This would lead to the same result we have today, but it would fix the red btn in your second example.

What do you think?

[mattez]

1) color

Agreed. Add color: color-yiq($hover-background); to focus state.

2) background

So I tested the case when is default a styled not only with color but also background to look e.g. like:

("text utilities" is focused link with $danger color, $danger outline and $yellow background)

then button :focus state on .btn made from <a> looks like this:

so it follows that we should set some background to buttons focus state to ensure consistency of the buttons. So if you @MartijnCuppens prefer (and I agree so) to keep normal button state background, then we should add @include gradient-bg($background); to

bootstrap/scss/mixins/_buttons.scss

Lines 18 to 19 in 39b7686

	&:focus,
	&.focus {

3) border

A case where someone would add a border to link is really marginal (almost crazy? :). For demonstration it behaves as expected:

then button :focus state on .btn made from <a> looks like this:

IMHO we don't have to deal with this crazy rare case.

Summary:

I propose to add color: color-yiq($hover-background); and @include gradient-bg($background); to focus state:

&:focus,
&.focus {
	color: color-yiq($hover-background);
	@include gradient-bg($background);
	...
}

What do you think?

[mattez]

Hello everybody who is in charge here :]. So what? @MartijnCuppens please :]

Just summarize this:

This change is only in one file /scss/mixins/_buttons.scss.
Into this focus state

bootstrap/scss/mixins/_buttons.scss

Lines 19 to 21 in 21e39ce

	&:focus,
	&.focus {
	// Avoid using mixin so we can pass custom focus shadow properly

adding these lines of code:

&:focus,
&.focus {
  color: color-yiq($hover-background);
  @include gradient-bg($hover-background);
  border-color: $hover-border;
  // Avoid using mixin so we can pass custom focus shadow properly

(here is diff of /scss/mixins/_buttons.scss)

to fix issues specified above in my last issuecomment ie color, background and border style.

The intent is override customized user setup for link styles (basic a href) to keep consistency of focus state of buttons made from a hrefs.

IMHO This can not hurt anyone.
What do you think?

[MartijnCuppens]

LGTM 👍

[XhmikosR]

@MartijnCuppens: does this need to be backported to v4?

[MartijnCuppens]

Yeah, this fixes issues with focus styles. Good call @XhmikosR