Closed Bug 1057030 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 --- verified
firefox35 --- verified

People

(Reporter: jaws, Assigned: dao)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached image 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.
Flags: firefox-backlog+
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.
Hi,
I am a beginner and I would like to work on this bug.Could someone please assign this bug to 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.
Component: Toolbars and Customization → Theme
Attached patch lwt-footer-buttons.diff (obsolete) — Splinter Review
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8495475 - Flags: review?(jaws)
Iteration: --- → 35.2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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...
Attachment #8495475 - Flags: review?(jaws) → review-
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.
(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.
Attached patch lwt-footer-buttons.diff (obsolete) — Splinter Review
Attachment #8477280 - Attachment is obsolete: true
Attachment #8495475 - Attachment is obsolete: true
Attachment #8495514 - Attachment is obsolete: true
Attachment #8495580 - Flags: review?(jaws)
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.
Attachment #8495580 - Flags: review?(jaws) → review-
r+ if you switch it back to border-box :)
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.
Attachment #8495580 - Flags: review- → review?(jaws)
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.
(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.
Attachment #8495580 - Attachment is obsolete: true
Attachment #8495580 - Flags: review?(jaws)
Attachment #8496113 - Flags: review?(jaws)
https://hg.mozilla.org/mozilla-central/rev/915d72b3b0b7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Should we uplift this for 34? :-)
Flags: needinfo?(jaws)
Flags: needinfo?(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
Attachment #8496113 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dao)
Flags: needinfo?(jaws)
Comment on attachment 8496113 [details] [diff] [review]
lwt-footer-buttons.diff

Aurora+
Attachment #8496113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Depends on: 1081496
No longer depends on: 1081496
You need to log in before you can comment on or make changes to this bug.