magicp Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160707083343

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Inspector with Firebug theme
3. Check sidebar-toggle


Actual results:

Inspector sidebar-toggle is darker than normal color. Other tools' toggle buttons are no problem.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=162b6fca7d868e8165d0f6ef80e02b5dd73161cb&tochange=652fa69526ff82afbe7b7e6a360709c457fddf19


Expected results:

Don't apply inverted filter for sidebar-toggle in Inspector.

	gasolin@mozilla.com Assignee
	Comment 1 • 8 years ago

The cause is when switch to firebug theme in preference, the window class is changed to "theme-light theme-firebug"(contain both) instead of just "theme-firebug". (switch to light or dark will contain `theme-light` or `theme-dark` only)

Is it intended or its the bug?

	gasolin@mozilla.com Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/63434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63434/

	gasolin@mozilla.com Assignee
	Comment 3 • 8 years ago

remove `theme-light` in firebug definition seems works well

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 4 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

https://reviewboard.mozilla.org/r/63434/#review60300

(In reply to Fred Lin [:gasolin] from comment #1)
> The cause is when switch to firebug theme in preference, the window class is
> changed to "theme-light theme-firebug"(contain both) instead of just
> "theme-firebug". (switch to light or dark will contain `theme-light` or
> `theme-dark` only)
> 
> Is it intended or its the bug?

This is intended behavior. Firebug theme is based on Light theme and shares many styles - thus it also uses the 'theme-light' class. So, this can't be removed from theme definition's `classList` .

Honza

	gasolin@mozilla.com Assignee
	Comment 5 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63434/diff/1-2/

	gasolin@mozilla.com Assignee
	Comment 6 • 8 years ago

Then the right solution would be add .theme-firebug style to not apply the icon filter

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 7 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

https://reviewboard.mozilla.org/r/63434/#review60604

::: devtools/client/themes/toolbars.css:999
(Diff revision 2)
>    filter: var(--icon-filter);
>  }
>  
> +.theme-firebug .devtools-button::before {
> +  filter: none;
> +}

Thanks!

Yep, we need this style.

Some comments:

1) I think we should move it into firebug-theme.css file into the existing "Remove filters on firebug specific images" CSS selectors group.
2) We can also remove
.theme-firebug #global-toolbar .devtools-button::before,
.theme-firebug #element-picker::before,

from that group.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 8 years ago

@ntim: do you see any reason why not to apply 'filter: none' on all .devtools-button::before in Firebug theme? Btw. this also fixes rewind-timeline and pause-resume-timeline buttons in the Animation panel.

Honza

	Tim Nguyen :ntim
	Comment 9 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> @ntim: do you see any reason why not to apply 'filter: none' on all
> .devtools-button::before in Firebug theme?
All our devtools icons are white currently. Using filter: none means that the icon will stay white for the fb theme, which lowers the icon contrast for icons that are unchanged by the firebug theme (add node button and clear button)

Once all icons are switched to SVG (bug 1260523), I plan to make all icons black by default, so both the Light and Firebug theme won't need to invert icons, therefor issues like this wouldn't happen again.


Anyway, if you plan to apply filter: none globally, it would be simpler to change the --icon-filter variable in toolbars.css.

	gasolin@mozilla.com Assignee
	Comment 10 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63434/diff/2-3/

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

https://reviewboard.mozilla.org/r/63434/#review60958

::: devtools/client/themes/firebug-theme.css
(Diff revision 3)
>  .theme-firebug .command-button-invertable::before,
>  .theme-firebug #sources-toolbar image,
>  .theme-firebug [id$="pane-toggle"] > image,
>  .theme-firebug [id$="pane-toggle"]::before,
> -.theme-firebug #global-toolbar .devtools-button::before,
> +.theme-firebug .devtools-button::before,
> -.theme-firebug #element-picker::before,

I am seeting that applying `filter: none` to all .devtools-button buttons affectes also toolbar buttons in the Performance panel (e.g. clear and record buttons). So, you need to come up with more specific selector and carefuly check the UI that all is ok.

Honza

	gasolin@mozilla.com Assignee
	Comment 12 • 8 years ago

I've checked the clear and record buttons in performance panel, they use `devtools-toolbar` style, which has no relation with this style.
And I also enable all panels in preference to check the style looks fine with the patch.


If a more specific syntax suits better for this case, I'd change the sentence to `.theme-firebug .devtools-button.sidebar-toggle::before` which is scoped to the sidebar-toggle button.

	Tim Nguyen :ntim
	Comment 13 • 8 years ago

(In reply to Fred Lin [:gasolin] from comment #12)
> I've checked the clear and record buttons in performance panel, they use
> `devtools-toolbar` style, which has no relation with this style.
> And I also enable all panels in preference to check the style looks fine
> with the patch.
The memory tool and the inspector are affected if you use the .devtools-button selector (Add node button for the inspector, and all memory tool icons will turn white)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 14 • 8 years ago

(In reply to Fred Lin [:gasolin] from comment #12)
> I've checked the clear and record buttons in performance panel, they use
> `devtools-toolbar` style, which has no relation with this style.
> And I also enable all panels in preference to check the style looks fine
> with the patch.
Sorry, I mixed this with the Memory panel. See also Tim's comment.

> 
> If a more specific syntax suits better for this case, I'd change the
> sentence to `.theme-firebug .devtools-button.sidebar-toggle::before` which
> is scoped to the sidebar-toggle button.
Note that targeting only the toggle button isn't enough since buttons in the Animation panel (rewind-timeline and pause-resume-timeline) need `filter: none` too.

Honza

	gasolin@mozilla.com Assignee
	Comment 15 • 8 years ago

thanks for point out, here's the screenshot with patch of these 3 icon styles

	gasolin@mozilla.com Assignee
	Comment 16 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63434/diff/3-4/

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 17 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

https://reviewboard.mozilla.org/r/63434/#review61204

The `#element-picker` button in the Animation panel is reverted and it should not be.
(I'll attach a screenshot)

Honza

	gasolin@mozilla.com Assignee
	Comment 19 • 8 years ago

I removed element-picker style based on comment 7, this patch add it back. (my git-cinnarbar is broken after the update, so here's the origin .patch file)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 20 • 8 years ago

https://reviewboard.mozilla.org/r/63434/#review61284

::: devtools/client/themes/firebug-theme.css:25
(Diff revision 4)
>  .theme-firebug #sources-toolbar image,
>  .theme-firebug [id$="pane-toggle"] > image,
>  .theme-firebug [id$="pane-toggle"]::before,
> -.theme-firebug #global-toolbar .devtools-button::before,
> -.theme-firebug #element-picker::before,
> +.theme-firebug .sidebar-toggle::before,
> +.theme-firebug #rewind-timeline::before,
> +.theme-firebug #pause-resume-timeline::before,

> I removed element-picker style based on comment
> 7, this patch add it back. (my git-cinnarbar is
> broken after the update, so here's the origin
> .patch file)

Since we can't use `theme-firebug .devtools-button::before {` selector (breaks other things) `#element-picker::before` needs to stay.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 21 • 8 years ago

(In reply to Fred Lin [:gasolin] from comment #19)
> Created attachment 8770862 [details] [diff] [review]
> Bug-1285449-Firebug-theme-Don-t-apply-inverted-filte.patch
> 
> I removed element-picker style based on comment 7, this patch add it back.
I don't see the selector: #element-picker::before in you patch, please fix it.

Here is what works for me:

.theme-firebug .devtools-tabbar .devtools-button::before,
.theme-firebug .devtools-option-toolbarbutton > image,
.theme-firebug .command-button-invertable::before,
.theme-firebug #sources-toolbar image,
.theme-firebug [id$="pane-toggle"] > image,
.theme-firebug [id$="pane-toggle"]::before,
.theme-firebug .sidebar-toggle::before,
.theme-firebug #element-picker::before,
.theme-firebug #rewind-timeline::before,
.theme-firebug #pause-resume-timeline::before,
.theme-firebug #debugger-controls .toolbarbutton-icon,
.theme-firebug #filter-button .toolbarbutton-icon {
  filter: none !important;
}

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 22 • 8 years ago

Comment on attachment 8770862 [details] [diff] [review]
Bug-1285449-Firebug-theme-Don-t-apply-inverted-filte.patch

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

Just removing the review flag (see my comment #21)

Honza

	gasolin@mozilla.com Assignee
	Comment 23 • 8 years ago

> +.theme-firebug .sidebar-toggle::before,
>  .theme-firebug #element-picker::before,
> +.theme-firebug #rewind-timeline::before,

the style `.theme-firebug #element-picker::before,` is exist in current code, 
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/themes/firebug-theme.css#L24

so I just keep it unchanged in my patch.

After apply patch in gecko-dev with command

> patch -p1 < 0001-Bug-1285449-Firebug-theme-Don-t-apply-inverted-filte.patch

The patched result looks exactly like comment 21.

	gasolin@mozilla.com Assignee
	Comment 24 • 8 years ago

Comment on attachment 8769608 [details]
Bug 1285449 - Firebug theme - Don't apply inverted filter for sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector;

I met mozreview/git-cinnabar problem after update those tool, so the .patch is the latest for review

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 25 • 8 years ago

Comment on attachment 8770862 [details] [diff] [review]
Bug-1285449-Firebug-theme-Don-t-apply-inverted-filte.patch

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

Works for me, thanks!

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 26 • 8 years ago

Just removing the NI flag, all resolved here.

Honza

	gasolin@mozilla.com Assignee
	Comment 27 • 8 years ago

Thanks you for review!

	Pulsebot
	Comment 28 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/30b6f751cdce
Firebug theme - Don't apply inverted filter to sidebar-toggle, rewind-timeline, pause-resume-timeline in Inspector; r=Honza

	Carsten Book [:Tomcat]
	Comment 29 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/30b6f751cdce

	Md. Rahimul Islam
	Comment 30 • 8 years ago

I have successfully reproduce this bug on firefox nightly 50.0a1 (2016-07-17)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0

I found this fix on latest nightly 50.0a1 (2016-07-23)

Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID : 20160723030206

[bugday-20160722]