1483332 - Port bug 1469287 to TB: Implement new shared tree styling

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1469287 removes some stylings, especially the Windows Explorer styling, and moves the twisties. Also the treeitem will be a lot taller (24px).

Comment 1

[Richard Marti (:Paenglab)]

Attachment: treeStyles.patch

This fixes the styling on all platforms.

To test you can download the m-c patch from https://hg.mozilla.org/integration/autoland/rev/c8c488989a62. Then no update of m-c is needed.

Comment 2

[[:MakeMyDay]]

Comment on attachment 9000048 [details] [diff] [review]
treeStyles.patch

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

Thanks, the calendar part looks good based on a codewise chack.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9000048 [details] [diff] [review]
treeStyles.patch

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

That seems to restore the original look in the thread pane. However, I think applying https://hg.mozilla.org/integration/autoland/rev/c8c488989a62 is not enough, a clobber is needed since this removes files which are still in the dist directory.

::: mail/themes/windows/mail/messenger.css
@@ +336,5 @@
> +treechildren::-moz-tree-indentation {
> +  width: 12px;
> +}
> +
> +@media (-moz-windows-default-theme) {

Why do we need this huge block of code that follows?

Comment 4

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #3)
> Comment on attachment 9000048 [details] [diff] [review]
> treeStyles.patch
> 
> Review of attachment 9000048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That seems to restore the original look in the thread pane. However, I think
> applying https://hg.mozilla.org/integration/autoland/rev/c8c488989a62 is not
> enough, a clobber is needed since this removes files which are still in the
> dist directory.
> 
> ::: mail/themes/windows/mail/messenger.css
> @@ +336,5 @@
> > +treechildren::-moz-tree-indentation {
> > +  width: 12px;
> > +}
> > +
> > +@media (-moz-windows-default-theme) {
> 
> Why do we need this huge block of code that follows?

This is for the Windows Explorer styling. Without it would look like under Win7 Classic (blue background when selected).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d6e04b5f6706
Port bug 1469287 to TB: Implement new shared tree styling. r=jorgk,MakeMyDay

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6561d0242d8b
Follow-up: Add calendar-widgets.css to allowed-dupes.mn. rs=bustage-fix

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 9000048 [details] [diff] [review]
treeStyles.patch

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

::: calendar/base/themes/common/widgets/calendar-widgets.css
@@ +14,5 @@
> +  -moz-appearance: none;
> +  -moz-box-align: center;
> +  border: none;
> +  width: 9px; /* The image's width is 9 pixels */
> +  height: 9px;

The image width is actually 8 pixels.

::: mail/themes/windows/mail/messenger.css
@@ +317,5 @@
> +  background-color: -moz-cellhighlight;
> +}
> +
> +treechildren::-moz-tree-row(selected, focus) {
> +  background-color: Highlight;

Not sure why you need those 3 rules. This is the styling that's already in toolkit.

@@ +356,5 @@
> +    --treechildren-hoverCurrentBorder: var(--treechildren-currentColor);
> +    --treechildren-hoverCurrentBackground: rgba(131,183,249,.16);
> +    --treechildren-hoverSelectedBorder: var(--treechildren-focusColor);
> +    --treechildren-hoverSelectedBackground: rgb(205,232,255);
> +  }

I guess you don't want the Photon styling for TB, which is fine I guess. 

Anyway, note that you'll have a much harder time implementing the sidebar theme properties with the Windows styling (bug 1418602)

Comment 8

[Richard Marti (:Paenglab)]

(In reply to Tim Nguyen :ntim from comment #7)
> Comment on attachment 9000048 [details] [diff] [review]
> treeStyles.patch
> 
> Review of attachment 9000048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: calendar/base/themes/common/widgets/calendar-widgets.css
> @@ +14,5 @@
> > +  -moz-appearance: none;
> > +  -moz-box-align: center;
> > +  border: none;
> > +  width: 9px; /* The image's width is 9 pixels */
> > +  height: 9px;
> 
> The image width is actually 8 pixels.

Okay, that changed. Then I'll fix this in a new bug.

> ::: mail/themes/windows/mail/messenger.css
> @@ +317,5 @@
> > +  background-color: -moz-cellhighlight;
> > +}
> > +
> > +treechildren::-moz-tree-row(selected, focus) {
> > +  background-color: Highlight;
> 
> Not sure why you need those 3 rules. This is the styling that's already in
> toolkit.

You re-introduced the Zebra-striping in Windows. Because our treechildren::-moz-tree-row(multicol, odd) has more specificy than the rules in tree.css we need to overrule them again.

> @@ +356,5 @@
> > +    --treechildren-hoverCurrentBorder: var(--treechildren-currentColor);
> > +    --treechildren-hoverCurrentBackground: rgba(131,183,249,.16);
> > +    --treechildren-hoverSelectedBorder: var(--treechildren-focusColor);
> > +    --treechildren-hoverSelectedBackground: rgb(205,232,255);
> > +  }
> 
> I guess you don't want the Photon styling for TB, which is fine I guess. 

When Photon styling means the old Classic look on Windows, no. TB bases a lot more on trees and they should look like the standard (Windows Explorer).

Comment 9

[Tim Nguyen :ntim]

(In reply to Richard Marti (:Paenglab) from comment #8)
> You re-introduced the Zebra-striping in Windows. Because our
> treechildren::-moz-tree-row(multicol, odd) has more specificy than the rules
> in tree.css we need to overrule them again.

Ah interesting, I'll check if we can remove it. This only affects the library window and about:config in Firefox right ?

> > @@ +356,5 @@
> > > +    --treechildren-hoverCurrentBorder: var(--treechildren-currentColor);
> > > +    --treechildren-hoverCurrentBackground: rgba(131,183,249,.16);
> > > +    --treechildren-hoverSelectedBorder: var(--treechildren-focusColor);
> > > +    --treechildren-hoverSelectedBackground: rgb(205,232,255);
> > > +  }
> > 
> > I guess you don't want the Photon styling for TB, which is fine I guess. 
> 
> When Photon styling means the old Classic look on Windows, no. TB bases a
> lot more on trees and they should look like the standard (Windows Explorer).

Windows doesn't really have a well defined standard if you also consider UWP apps in the mix.

Comment 10

[Richard Marti (:Paenglab)]

(In reply to Tim Nguyen :ntim from comment #9)
> (In reply to Richard Marti (:Paenglab) from comment #8)
> > You re-introduced the Zebra-striping in Windows. Because our
> > treechildren::-moz-tree-row(multicol, odd) has more specificy than the rules
> > in tree.css we need to overrule them again.
> 
> Ah interesting, I'll check if we can remove it. This only affects the
> library window and about:config in Firefox right ?

about:config is styled through the in-content styles. The library doesn't have the striping. Not checked, but it must also have a styling somewhere. Also the treeitem height isn't 24px.

> > > @@ +356,5 @@
> > > > +    --treechildren-hoverCurrentBorder: var(--treechildren-currentColor);
> > > > +    --treechildren-hoverCurrentBackground: rgba(131,183,249,.16);
> > > > +    --treechildren-hoverSelectedBorder: var(--treechildren-focusColor);
> > > > +    --treechildren-hoverSelectedBackground: rgb(205,232,255);
> > > > +  }
> > > 
> > > I guess you don't want the Photon styling for TB, which is fine I guess. 
> > 
> > When Photon styling means the old Classic look on Windows, no. TB bases a
> > lot more on trees and they should look like the standard (Windows Explorer).
> 
> Windows doesn't really have a well defined standard if you also consider UWP
> apps in the mix.

We had this styling for years now in toolkit. Removing it now would be a step backwards.