Jonathan Kingston [:jkt] he/him Reporter
	Description • 8 years ago

STR:
1. Click customise in the menu
2. Drag new tab button to the menu below by the downloads button default location

The background of this icon has the colour from the bar above rather than this bar (see attached screenshot).

	:Gijs (he/him)
	Comment 1 • 8 years ago

This selector:

https://dxr.mozilla.org/mozilla-central/rev/5e76768327660437bf3486554ad318e4b70276e1/browser/themes/linux/devedition.css#82-85

should be specific to the case where the #new-tab-button is a descendant of #TabsToolbar . The alltabs-button doesn't need that specificity as it is always in the tabs bar.

	Rishi Ahya
	Comment 2 • 8 years ago

Hi there 

How do I get started on this bug?, and can you assign it to me?

thanks

	:Gijs (he/him)
	Comment 3 • 8 years ago

(In reply to Rishi Ahya from comment #2)
> Hi there 
> 
> How do I get started on this bug?, and can you assign it to me?
> 
> thanks

You'd want to start by getting the source code and doing a first Firefox build ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build ), then checking you can reproduce the problem (only visible on linux and the devedition theme), then changing the CSS as suggested in comment #1, rebuilding and checking it's fixed. Let me know if you have more questions.

	tfe Assignee
	Comment 5 • 7 years ago

Hello. I am quite new in the firefox dev.
I am submitting you a patch to be reviewed.

Let me know if all is correct. Thanks!

	:Gijs (he/him)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8870831 [details]
Bug 1317190 - [Linux] On the dark theme, fix Background color of new-tab button when new-tab is not in tabs toolbar

https://reviewboard.mozilla.org/r/142400/#review146010

::: commit-message-3a82a:1
(Diff revision 1)
> +Bug 1317190 - Background color of new-tab button, on dark theme r?Gijs

Can you update this to specify this is Linux-specific, and that we're fixing the background color *when the button isn't in the tabs toolbar* ?

::: browser/themes/linux/compacttheme.css:43
(Diff revision 1)
>  /* Add the proper background for tab overflow */
>  #alltabs-button,
> -#new-tab-button {
> +#TabsToolbar #new-tab-button {
>    background: var(--chrome-background-color);
>  }

This is exactly what I said in comment 1, despite the file having moved, so in that sense this patch is correct - nice!

However, testing this, I'm realizing I was wrong in comment 1. :-(

2 things:
1) this change breaks the :hover styling for the button, when it's in its default place (the tabs toolbar) because now this rule is more specific than the rule for hovering items in the tabs toolbar (which uses #TabsToolbar .toolbarbutton-1 as a selector).
2) this rule actually seems to be unnecessary by now. I think we can just remove it. :-)

Can you update the patch to just remove this rule?

	:Gijs (he/him)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8870831 [details]
Bug 1317190 - [Linux] On the dark theme, fix Background color of new-tab button when new-tab is not in tabs toolbar

https://reviewboard.mozilla.org/r/142400/#review146044

::: browser/themes/linux/compacttheme.css:44
(Diff revision 2)
>    list-style-image: var(--urlbar-dropmarker-url);
>    -moz-image-region: var(--urlbar-dropmarker-region);
>  }
>  
>  /* Add the proper background for tab overflow */
> -#alltabs-button,
> +#alltabs-button {

I think you can remove the rule entirely, including this bit for the alltabs-button. Did you see something else that meant that didn't work right?

	tfe Assignee
	Comment 9 • 7 years ago

A quick question, because i think it will be a better way to test the xul styles.
Right now i am modifying the style, run a

> ./mach build browser && ./mach run -P

Then i check the style i updated.

But it is really an old school way of updating styles! (that rembered me how was the web developement on the 2000 :p)
Doesn't firefox has a way to have the "developper toolbar" but for the interface?

	:Gijs (he/him)
	Comment 11 • 7 years ago

(In reply to tfe from comment #9)
> A quick question, because i think it will be a better way to test the xul
> styles.
> Right now i am modifying the style, run a
> 
> > ./mach build browser && ./mach run -P
> 
> Then i check the style i updated.
> 
> But it is really an old school way of updating styles! (that rembered me how
> was the web developement on the 2000 :p)
> Doesn't firefox has a way to have the "developper toolbar" but for the
> interface?

You can use the browser toolbox to update styles, yes. See: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox for instructions . You'll need to enable it from the devtools settings, it's not available by default.

	:Gijs (he/him)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8870831 [details]
Bug 1317190 - [Linux] On the dark theme, fix Background color of new-tab button when new-tab is not in tabs toolbar

https://reviewboard.mozilla.org/r/142400/#review146322

Looks great, thanks!

	Pulsebot
	Comment 13 • 7 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e1ed9fd35ac2
[Linux] On the dark theme, fix Background color of new-tab button when new-tab is not in tabs toolbar r=Gijs

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e1ed9fd35ac2