1064471 - Storage inspector theme refresh

Status: VERIFIED FIXED

(DevTools :: Storage Inspector, defect)

Description

[Tim Nguyen :ntim]

The mockups shows a borderless, darker style for the table rows.

The table headers are being updated in other bugs.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch v2

Bunch of additional changes

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8616408 [details] [diff] [review]
Patch v2

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

Thanks for the patch, and sorry for the delay on reviews.  I like the new look and have a few comments.

There is a missing bottom border on the table now (I'll add a screenshot showing this)

::: browser/themes/shared/devtools/widgets.inc.css
@@ +9,5 @@
>  %define smw_itemDarkTopBorder rgba(0,0,0,0.2)
>  %define smw_itemDarkBottomBorder rgba(128,128,128,0.15)
>  %define smw_itemLightTopBorder rgba(128,128,128,0.15)
>  %define smw_itemLightBottomBorder transparent
> +:root.theme-dark {

In other places, I've just used .theme-dark / .theme-light as the selector for setting variables.  Let's keep that in use to stay consistent

@@ +1222,5 @@
>  /* Column Headers */
>  
> +.table-widget-column-header,
> +.table-widget-cell {
> +  -moz-border-end: 1px solid var(--table-splitter-color) !important;

Is the new !important here necessary?

@@ +1491,5 @@
>  }
>  
> +.theme-light .tree-widget-item.theme-selected[type]::after,
> +.theme-dark .tree-widget-item[type]::after {
> +  filter: invert(1);

Can we use this?  In toolbars.inc.css we are using `filter: url(filters.svg#invert);` and I thought we had to do that using the external file for some reason.

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#623

Comment 4

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8616408 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8616408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, and sorry for the delay on reviews.  I like the new
> look and have a few comments.
> 
> There is a missing bottom border on the table now (I'll add a screenshot
> showing this)
> 
> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +9,5 @@
> >  %define smw_itemDarkTopBorder rgba(0,0,0,0.2)
> >  %define smw_itemDarkBottomBorder rgba(128,128,128,0.15)
> >  %define smw_itemLightTopBorder rgba(128,128,128,0.15)
> >  %define smw_itemLightBottomBorder transparent
> > +:root.theme-dark {
> 
> In other places, I've just used .theme-dark / .theme-light as the selector
> for setting variables.  Let's keep that in use to stay consistent
Will do.

> @@ +1222,5 @@
> >  /* Column Headers */
> >  
> > +.table-widget-column-header,
> > +.table-widget-cell {
> > +  -moz-border-end: 1px solid var(--table-splitter-color) !important;
> 
> Is the new !important here necessary?
The rule didn't apply without it. I couldn't find what was overriding this, so I used important.

> @@ +1491,5 @@
> >  }
> >  
> > +.theme-light .tree-widget-item.theme-selected[type]::after,
> > +.theme-dark .tree-widget-item[type]::after {
> > +  filter: invert(1);
> 
> Can we use this?  In toolbars.inc.css we are using `filter:
> url(filters.svg#invert);` and I thought we had to do that using the external
> file for some reason.
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> devtools/toolbars.inc.css#623
CSS filters were not implemented yet at the time we introduced the light theme. Also, the SVG filter won't work in this case, since it only supports white to black inversion (not the other way).

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #4)
> > > +.theme-light .tree-widget-item.theme-selected[type]::after,
> > > +.theme-dark .tree-widget-item[type]::after {
> > > +  filter: invert(1);
> > 
> > Can we use this?  In toolbars.inc.css we are using `filter:
> > url(filters.svg#invert);` and I thought we had to do that using the external
> > file for some reason.
> > 
> > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> > devtools/toolbars.inc.css#623
> CSS filters were not implemented yet at the time we introduced the light
> theme. Also, the SVG filter won't work in this case, since it only supports
> white to black inversion (not the other way).

Should we switch the toolbars.inc reference to use this new method or is it somehow different?

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: border-bottom.png

screenshot of missing border

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch v3

(In reply to Brian Grinstead [:bgrins] from comment #3)
> @@ +1222,5 @@
> >  /* Column Headers */
> >  
> > +.table-widget-column-header,
> > +.table-widget-cell {
> > +  -moz-border-end: 1px solid var(--table-splitter-color) !important;
> 
> Is the new !important here necessary?
Took a look again, and yes, it is necessary, we apply the .plain class on each cell, which sets a border: none; with higher priority.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8627057 [details] [diff] [review]
Patch v3

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

Nice, thanks!

::: browser/themes/shared/devtools/widgets.inc.css
@@ -9,5 @@
>  %define smw_itemDarkTopBorder rgba(0,0,0,0.2)
>  %define smw_itemDarkBottomBorder rgba(128,128,128,0.15)
>  %define smw_itemLightTopBorder rgba(128,128,128,0.15)
>  %define smw_itemLightBottomBorder transparent
> -%define table_itemDarkStartBorder rgba(0,0,0,0.2)

Great to see more preprocessor defines going away

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/620febe27492

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/620febe27492

Comment 13

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 35.0a1 (2014-09-08)
with windows 10 (32 bit)
Mozilla/5.0 (Windows NT 6.3; rv:35.0) Gecko/20100101 Firefox/35.0

I found this fix on latest beta  42.0b4

Mozilla/5.0 (Windows NT 10.0; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID : 20151005144425