1408198 - Inspector breadcrumbs scrollbutton is black in dark theme

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[atlanto]

Attachment: screenshot

mozregression tells
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=eaa0d99df02949e2553063b67b7f72264c4706f2&tochange=534f2a0caa2c98d1eabb00e9a74352831bc2f2de

Comment 1

[Patrick Brosset <:pbro>]

Thank you for filing this regression. It indeed works in 57, but doesn't in 58. mozregression indicates bug 1399886 as being the culprit.

Comment 2

[Tim Nguyen :ntim]

The scrollbuttons icons are currently PNG, but we can replace them with the photon ones which are SVG: http://design.firefox.com/icons/viewer/#arrowhead

Comment 3

[Mike Taylor [:miketaylr]]

Do we wanna block 58 on this? Not much time left in the 58 window.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

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

Comment 5

[Julian Descottes [:jdescottes]]

Taking this one.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: svg_icon_breadcrumbs.png

Ended up using "arrowhead left 16" from the Photon icons. "arrowhead left 12" was too thick, and stood out too much IMO. 

Screenshot with both light and dark themes.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

Clearing, needs some more work for firebug theme

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

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

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

https://reviewboard.mozilla.org/r/197714/#review202940

Thanks for working on this! It seems that you forgot to add the SVG in the patch :)

Comment 10

[Tim Nguyen :ntim]

Seems like this can be landed in time for 58.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

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

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim from comment #9)
> Comment on attachment 8926475 [details]
> Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;
> 
> https://reviewboard.mozilla.org/r/197714/#review202940
> 
> Thanks for working on this! It seems that you forgot to add the SVG in the
> patch :)

Oops, updated.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

https://reviewboard.mozilla.org/r/197714/#review202952

::: devtools/client/themes/breadcrumbs.css:31
(Diff revision 3)
>  .scrollbutton-down > .toolbarbutton-icon {
>    -moz-appearance: none;
> -  width: 7px;
> +  width: 20px;
>    height: 16px;
> -  background-size: 14px 16px;
> -  background-position: 0 center;
> +  background-size: 16px;
> +  background-position: 2px center;

background-position: center; just works fine, no need for the magic `2px` ;)

::: devtools/client/themes/breadcrumbs.css:236
(Diff revision 3)
>  .theme-firebug .breadcrumbs-widget-container .scrollbutton-up:not([disabled]):active:hover > .toolbarbutton-icon,
>  .theme-firebug .breadcrumbs-widget-container .scrollbutton-down:not([disabled]):active:hover > .toolbarbutton-icon {
> -  background-position: 0 center;
> +  background-position: 2px center;
>  }

This rule doesn't seem to be useful anymore (neither in the old debugger, neither in the inspector). Can you remove it?

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8926475 [details]
Bug 1408198 - Use SVG icon for breadcrumbs scrollbuttons;

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77d2425c4b40
Use SVG icon for breadcrumbs scrollbuttons;r=ntim

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/77d2425c4b40