282127 - Highlight odd tree rows in trees with multiple columns in Gnomestripe and Pinstripe

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Robert Strong (they/them - no direct email)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050209 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050209 Firefox/1.0+

It appears that alternatingbackground doesn't work for trees with Firefox. This
can be seen by inspecting DOM Inspector with DOM Inspector. The treeChildren in
the right hand panel have alternatingbackground set to true but the background
does not have the even / odd row alternating background color.

Reproducible: Always

Comment 1

[Mike Connor [:mconnor]]

I don't think we have any real support for this.

Comment 2

[Robert Strong (they/them - no direct email)]

I was able to style it myself in Firefox using treechildren::-moz-tree-row(odd)
etc. - I expect that it was either left out on purpose in the theme or it was
overlooked. I'm not an advocate of having this so much as would like to see it
consistent between the Suite and Firefox. 

Comment 3

[Steffen Wilberg]

Attachment: patch

alternatingbackground="true" is only styled in Pinstripe right now.
And the only users of it are DOM Inspector in the right-hand trees and about:config since I checked in bug 251843 today.

I'm adding the styling to Gnomestripe, where this is a platform convention.
I'm not so sure about the convention on Windows, where the styling is missing as well.

I've added alternatingbackground="true" to all treechildren I could find with multiple columns: Page Info, some dead code in the Sanitize dialog, the right-hand side of the Places Library, the Cookies Viewer, the Exceptions dialogs, the Search Engine manager, about:sessionrestore (including removal of nonfunctional styling), and the Password Manager.

I didn't touch richlistbox yet. Only the Downloads window has alternate row styling right now.

Comment 4

[Steffen Wilberg]

Comment on attachment 439755 [details] [diff] [review]
patch

Gavin, please see comment 3.

Comment 5

[[e-mail]]

(In reply to comment #3)
> about:sessionrestore (including removal of nonfunctional styling)
Nonfunctional? There is code that is supposed to invoke it: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/aboutSessionRestore.js#320

Comment 6

[Stefan [:stefanh]]

JFTR, on mac, we actually have "striped" trees even though there's no alternatingbackground attribute set to "true". The pinstripe rule below takes care of most cases:

tree:not([hidecolumnpicker="true"]) treechildren::-moz-tree-row(odd) {
  background-color: -moz-oddtreerow;
}

Comment 7

[Steffen Wilberg]

Comment on attachment 439755 [details] [diff] [review]
patch

(In reply to comment #5)
That explains why I didn't notice it: It colors window by window instead of line by line. That does indeed work on Linux as well.

(In reply to comment #6)
Interesting, I'll try that for Gnomestripe. But then Pinstripe's  styling for the alternatingbackground attribute is useless, isn't it?

Comment 8

[Steffen Wilberg]

> tree:not([hidecolumnpicker="true"]) treechildren::-moz-tree-row(odd)
This only styles trees with a column picker, i.e. about:config and the Media tab of Page Info.

However, it doesn't style these trees, which I do want to style since they have more than one column:
- Page Info, General tab
- Places Library, right-hand-side (which has configurable columns, but no column picker!?)
- Cookies Viewer
- Exceptions dialogs
- Search Engine Manager
- Password Manager

I don't want to style trees with a single column though, like the Bookmarks and History sidebar, the left-hand side of the Places Library, the password exceptions dialog etc., so I can't just drop :not([hidecolumnpicker="true"]), see bug 462620.

So I want a selector for odd tree rows in a tree with more than one column...

Comment 9

[Stefan [:stefanh]]

(In reply to comment #7)
> (In reply to comment #6)
> Interesting, I'll try that for Gnomestripe. But then Pinstripe's  styling for
> the alternatingbackground attribute is useless, isn't it?

I wouldn't say it's useless - anyone that choose to use the alternatingbackground attribute should get the right styling, shouldn't they?

(In reply to comment #8)
> > tree:not([hidecolumnpicker="true"]) treechildren::-moz-tree-row(odd)
> This only styles trees with a column picker, i.e. about:config and the Media
> tab of Page Info.

Yeah, I know - it's a bit limited. The best thing to do is probably to add the alternatingbackground attribute, provided that all OS flavours needs it.

Comment 10

[Steffen Wilberg]

Attachment: patch v2: highlight odd tree rows in trees with >1 column in Gnomestripe and Pinstripe

The selector I needed is
  tree:not([colCount="1"]) treechildren::-moz-tree-row(odd).
To make that work, I added a colCount attribute to tree.xml.

Before this patch, odd tree rows are highlighted in Pinstripe but not Gnomestripe if the column picker was visible (not hidden). This only applies to about:config and the Media tab in Page Info.

With this patch, odd tree rows are highlighted in Pinstripe and Gnomestripe if the tree has more than 1 column. This applies to most trees in the UI, see comment 8. I guess it wouldn't hurt to get ui-review for that.

Comment 11

[Stefan [:stefanh]]

Comment on attachment 439816 [details] [diff] [review]
patch v2: highlight odd tree rows in trees with >1 column in Gnomestripe and Pinstripe

I think the behavior might be a bit odd when columns are hidden/shown. I'm thinking of, for example, the library window where you can hide all columns except one. Let's say I open the library window, then hide all columns except one - the tree would still be striped, but the next time you open it it will not be striped, right (based on the constructor modification)?

Comment 12

[Stefan [:stefanh]]

And I also think that on mac, you might want the stripes even though there's only one column in certain places (like in library/places).

Comment 13

[Steffen Wilberg]

No, colCount is the number of all columns, whether hidden or not. So the styling doesn't change if there are multiple columns and you hide all but one.
E.g. the Library with one column is still striped even when you open it next time.
Same for Page Info - Media (which lets you hide *all* columns, ugh!)
about:config doesn't persist column hiding, so opens up with all columns again.

I can add stripes on one-column trees easily by adding alternatingbackground="true" like my first patch did.

Comment 14

[Stefan [:stefanh]]

(In reply to comment #13)
> No, colCount is the number of all columns, whether hidden or not. So the
> styling doesn't change if there are multiple columns and you hide all but one.
> E.g. the Library with one column is still striped even when you open it next

Ah, ok.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 439816 [details] [diff] [review]
patch v2: highlight odd tree rows in trees with >1 column in Gnomestripe and Pinstripe

ui-r+ just based on described functioanlity

Comment 16

[:Gavin Sharp [email: [e-mail]]]

One of the Neils is probably a better reviewer for this. I don't understand why the patch touches aboutSessionRestore.css.

Comment 17

[Steffen Wilberg]

about:sessionrestore displays like this:
window 1
  tab 1
  tab 2
window 2
  tab 1
  tab 2

It doesn't want to highlight line by line ("odd"), but window by window ("alternate"): window 1 is highlighted, window 2 is not, etc.

The change to aboutSessionRestore.css is to maintain that style by undoing row-highlighting in tree.css. I've added two comments in aboutSessionRestore.css trying to explain that...

We probably want to do that for Pinstripe's aboutSessionRestore.css as well to fix bug 462620.

Comment 18

[Steffen Wilberg]

Comment on attachment 439816 [details] [diff] [review]
patch v2: highlight odd tree rows in trees with >1 column in Gnomestripe and Pinstripe

Bumping review request to Neil Deaking per Gavin.

Comment 19

[Steffen Wilberg]

s/Deaking/Deakin/

Comment 20

[Neil Deakin]

It would be better to add another ::-moz-tree-row property for this (such as a singlecolumn property) The code for this is at nsTreeBodyFrame::PrefillPropertyArray. It would also automatically work if columns were added or removed dynamically.

Comment 21

[Steffen Wilberg]

Attachment: patch v3: use -moz-tree-row(multicol, odd)

I went for "multicol", since I want to style trees with multiple columns.
nsGkAtoms::multicol already existed, but unused.

Please review the nsTreeBodyFrame::PrefillPropertyArray bit carefully as I don't know what I'm doing...

Comment 22

[Steffen Wilberg]

Attachment: patch v3.1: use ::-moz-tree-row(multicol, odd)

Fixed a typo in Pinstripe's tree.css.

Comment 23

[Neil Deakin]

Comment on attachment 444216 [details] [diff] [review]
patch v3.1: use ::-moz-tree-row(multicol, odd)

>-treechildren::-moz-tree-row(selected, focus) {
>+treechildren::-moz-tree-row(selected, focus),
>+treechildren::-moz-tree-row(multicol, odd, selected, focus) {
>   background-color: Highlight;
> }

Why do we only want the selected and focused highlight colour on odd rows?

Comment 24

[Steffen Wilberg]

We always need to highlight the selected row, being multicol+odd or not.
So unfocused odd rows should have "background-color: -moz-oddtreerow;",
and focused rows "background-color: Highlight;".

This is already part of Pinstripe's tree.css, where I'm just replacing
  tree:not([hidecolumnpicker="true"])
by
  treechildren::-moz-tree-row(multicol)

Comment 25

[Neil Deakin]

The above is from the linux theme. The old line would highlight rows that are 'selected' and 'focused', but now you've made that also only apply to multicolumn and odd rows. How do single column trees and even rows get highlighted?

Comment 26

[Steffen Wilberg]

The old line is still there, I just added a comma...

+treechildren::-moz-tree-row(selected, focus),
+treechildren::-moz-tree-row(multicol, odd, selected, focus) {

The first line still applies to all selected+focused rows, like single column trees, even rows, etc.

Comment 27

[Neil Deakin]

Comment on attachment 444216 [details] [diff] [review]
patch v3.1: use ::-moz-tree-row(multicol, odd)

Ah, of course. Sorry for my confusion.

Comment 28

[Steffen Wilberg]

Thanks. Do I need another review for the nsTreeBodyFrame.cpp part? Looks like bz/roc/dbaron usually review here.

http://www.mozilla.org/about/owners.html#xptoolkit looks to be highly out of date.

Comment 29

[Neil Deakin]

(In reply to comment #28)
> Thanks. Do I need another review for the nsTreeBodyFrame.cpp part? Looks like
> bz/roc/dbaron usually review here.

No.

Comment 30

[Steffen Wilberg]

Cool!

http://hg.mozilla.org/mozilla-central/rev/e719b565185c

Comment 31

[Steffen Wilberg]

Coming back to the original summary of this bug, I filed bug 565460 to remove the styles for treechildren[alternatingbackground="true"] from Pinstripe's tree.css.