1057030 - The arrow on the Themes panel within customization mode has a color that doesn't match the buttons that are next to it

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of bug

See attached screenshot, looking at the arrow on the panel. The arrow is white but the buttons on the panel are grey.

Comment 1

[jaramat]

Attachment: Similar issue with PanelUI panel-arrow.png

There is a similar issue with the PanelUI popup and its arrow when a submenu is entered, where the arrow doesn't match the grey header's color.

If there's a separate bug filled on this already or this is intentional, feel free to mark this attachment as obsolete.

Comment 2

[Athira S]

Hi,
I am a beginner and I would like to work on this bug.Could someone please assign this bug to me?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Athira S from comment #2)
> Hi,
> I am a beginner and I would like to work on this bug.Could someone please
> assign this bug to me?

Hi, thanks for your interest in this bug. Unfortunately I'm not sure how simple this bug will be, and it may not be a good bug to work on for your first bug as there will probably need to be some investigations as to how this bug can be fixed.

As comment #1 has pointed out, it's not the only place within Firefox that has this issue, so it would be good if we could get a wider-scope fix.

Comment 4

[Dão Gottwald [:dao]]

Attachment: lwt-footer-buttons.diff

As always, letting a custom background touch the arrow is a bad idea since the arrow can't adapt to that, it just matches the standard panel background. I don't think we should add custom arrow images for such cases either.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495475 [details] [diff] [review]
lwt-footer-buttons.diff

Review of attachment 8495475 [details] [diff] [review]:
-----------------------------------------------------------------

This breaks hover styling for the buttons. Attaching a screenshot now...

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495475 [details] [diff] [review]
lwt-footer-buttons.diff

Review of attachment 8495475 [details] [diff] [review]:
-----------------------------------------------------------------

This breaks hover styling for the buttons. Attaching a screenshot now...

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ -351,5 @@
>  .customization-lwtheme-menu-footeritem {
>    -moz-appearance: none;
>    -moz-box-flex: 1;
>    color: hsl(0,0%,50%);
> -  border: 1px solid transparent;

This rule needs to stay or the hover styling will be broken as my attachment shows.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8495475 [details] [diff] [review]
> lwt-footer-buttons.diff
> 
> Review of attachment 8495475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This breaks hover styling for the buttons. Attaching a screenshot now...

Not on my end. It's Windows-specific, I suppose.

Comment 9

[Dão Gottwald [:dao]]

Attachment: lwt-footer-buttons.diff

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495580 [details] [diff] [review]
lwt-footer-buttons.diff

Review of attachment 8495580 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +359,5 @@
>    margin-right: 0;
>  }
>  
>  .customization-lwtheme-menu-footeritem:hover {
> +  background: linear-gradient(hsla(210,4%,10%,.08) 40%, hsla(210,4%,10%,0)) padding-box;

Why did you change this to padding-box? border-box is necessary to show the top border at a darker hue on :hover. With padding-box, the top-border is the exact color as the background.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

r+ if you switch it back to border-box :)

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8495580 [details] [diff] [review]
lwt-footer-buttons.diff

No idea what you mean. .customization-lwtheme-menu-footeritem don't have a top border. Only .customization-lwtheme-menu-footeritem:first-child has an end border and letting that not bleed into the item's background is consistent with the hover style of the second footer item.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I'm talking about the top border on the footer. Before your patch, there is a noticeable top border, but your patch removes that because of the switch to padding-box. The other way to fix this would be to change the border-top-color to the multiplied rgba(24,25,26,.05) x2 value.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I'm talking about the top border on the footer. Before your patch, there is
> a noticeable top border, but your patch removes that because of the switch
> to padding-box. The other way to fix this would be to change the
> border-top-color to the multiplied rgba(24,25,26,.05) x2 value.

I see. Doesn't have anything to do with :hover or the use of padding-box in the code you commented on, though.

Comment 15

[Dão Gottwald [:dao]]

Attachment: lwt-footer-buttons.diff

Comment 16

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/915d72b3b0b7

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/915d72b3b0b7

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0

The theme panel arrow color (from customize mode) is now properly displayed on Nightly, build ID: 20141001030205.

Comment 19

[:Gijs (he/him)]

Should we uplift this for 34? :-)

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8496113 [details] [diff] [review]
lwt-footer-buttons.diff

Approval Request Comment
[Feature/regressing bug #]: bug 1007336
[User impact if declined]: see comment 0
[Describe test coverage new/current, TBPL]: no test coverage
[Risks and why]: low risk, only a few CSS changes
[String/UUID change made/needed]: none

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8496113 [details] [diff] [review]
lwt-footer-buttons.diff

Aurora+

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0b58de5f17

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0

Also confirming this fix on latest Aurora, build ID: 20141009004002.