Change background color of dialog close button

Hi @Chi. I can't replicate this issue using Drupal 8.4.x version.

I can see you reported this issue for version 8.3.x-dev.

So I guess this issue was already fixed on vesion 8.4.x. Could you update your Drupal version and test again?

Thanks.

Hi @alarez. The problem still occurs in 8.4.x. It doesn't appear in IE/Edge but does appear in Firefox and Chrome. It's a quick flash of yellow as you close the window.

It appears to be coming from the background color set in core/themes/seven/css/components/jquery.ui/theme.css, line 33 in 8.4.x-dev

.ui-state-active,
.ui-widget-content .ui-state-active {
  ...
  background: #fe6;
  ...
}

@benqwerty,

As of mention in @Chi comment it's not happen on mouse over in 8.4.x. version.

@sandipauti, this was logged about the active state, not the hover state. The behaviour is the same in 8.3.x-dev and 8.4.x-dev.

I'm looking to this right now. I think that I've found the css code that causes the issue (tested with 8.4.x-dev).

This can be fixed by changing the css in core/themes/seven/css/components/jquery.ui/theme.css to:

.ui-state-active, .ui-widget-content .ui-state-active {
background-color: transparent;
border: none;
}

A patch is created.

Changed status to "Needs review" to test patch change_background_color-2857933-7.patch

After applying the patch change_background_color-2857933-7.patch #7 in the 8.4.x version it seems & working in:

Chrome

• 58.0.3029
• 57.0.2987

Firefox

• 52.0.1
• 52.0
• 51.0.1
• 51.0
• 51.0.2
• 51.0.1
• 50.0

Thanks everyone for working on this issue! I agree that the yellow color does look a bit off from the Seven style guide. However, I'm not confident that it's a good idea to fully remove the active effect.

Hi @lauriii,

After reviewing the Seven style guide I don't see why removing the color from that button is not compatible with it.

Thanks!

This active effect does not make much sense, it briefly flashes and then the modal itself disappears so I'm fine with removing this. If we want to do something with this link it will be more interesting to do something on hover, but that's a different issue.

Thanks for confirming @yoroy!

+++ b/core/themes/seven/css/components/jquery.ui/theme.css
@@ -33,8 +33,8 @@
+  background: transparent;
+  border: none;

In that case, it is redundant to set the background to transparent and to set the border as none because this element doesn't have anything set for these properties before this selector.

Working on that

After working with the latest 8.4.x version I cannot replicate it, so I think it can be closed.

Tested on:
Chrome

58.0.3029
57.0.2987

Firefox

52.0.1
52.0
51.0.1
51.0
51.0.2
51.0.1
50.0

It is still there though, I doublechecked on simplytest.me. Maybe you have the previous patch still applied in your local environment?

Attaching a small screen recording

Nop, using the current 8.4.x branch :/

@agomezmoron, I'm still seeing the yellow in 8.4.x.

Attached a patch to remove the two lines that add the yellow border and background.

Also see #2863354 Add border to dialog close button for hover and focus states for possible addition of hover and focus states to the close button.

It works for me in branch 8.4.x using Chrome 57.0.2987.110 and Firefox 52.0.1

Thanks for the feeback @tar_inet

The css looks nice, simple and specific. The change does indeed work on the targeted browsers (I have even just checked Safari, too!).

Without patch, it appears just like yoroy's animation in #19 and with the patch as attached.

I'm happy to set this to RTBC.

After doing all the manual testing, could I claim for the credit?

Saving commit credit.

Oh by the way, that was my first commit and we forgot to finish the live commit session with me posting back here. And there were other things that went wrong too, but we got there :-)

Thanks all for working on this!

WOOHOO!

There is a follow-up for accessibility of this button: #2863354: Add border to dialog [x] close button for hover and focus states

tidy up of issue summary