Closed Bug 282127 Opened 15 years ago Closed 10 years ago

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

Categories

(Toolkit :: Themes, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: robert.strong.bugs, Assigned: steffen.wilberg)

References

Details

Attachments

(1 file, 3 obsolete files)

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
I don't think we have any real support for this.
Assignee: firefox → webmail
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. 
Assignee: kevin → nobody
Component: General → Theme
OS: Windows XP → Linux
QA Contact: general → theme
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → steffen.wilberg
Status: NEW → ASSIGNED
Attachment #439755 - Flags: review?
Comment on attachment 439755 [details] [diff] [review]
patch

Gavin, please see comment 3.
Attachment #439755 - Flags: review? → review?(gavin.sharp)
(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
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 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?
Attachment #439755 - Attachment is obsolete: true
Attachment #439755 - Flags: review?(gavin.sharp)
> 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...
(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.
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.
Attachment #439816 - Flags: ui-review?(faaborg)
Attachment #439816 - Flags: review?(iamgavin)
Attachment #439816 - Flags: review?(iamgavin) → review?(gavin.sharp)
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)?
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).
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.
(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 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
Attachment #439816 - Flags: ui-review?(faaborg) → ui-review+
One of the Neils is probably a better reviewer for this. I don't understand why the patch touches aboutSessionRestore.css.
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
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 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.
Attachment #439816 - Flags: review?(gavin.sharp) → review?(enndeakin)
s/Deaking/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.
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...
Attachment #439816 - Attachment is obsolete: true
Attachment #444214 - Flags: ui-review+
Attachment #444214 - Flags: review?(enndeakin)
Attachment #439816 - Flags: review?(enndeakin)
Fixed a typo in Pinstripe's tree.css.
Attachment #444214 - Attachment is obsolete: true
Attachment #444216 - Flags: ui-review+
Attachment #444216 - Flags: review?(enndeakin)
Attachment #444214 - Flags: review?(enndeakin)
Summary: alternatingbackground does not work for trees → Highlight odd tree rows in trees with multiple columns in Gnomestripe and Pinstripe
Attachment #444216 - Attachment description: patch v3.1: use -moz-tree-row(multicol, odd) → patch v3.1: use ::-moz-tree-row(multicol, odd)
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?
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)
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?
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 on attachment 444216 [details] [diff] [review]
patch v3.1: use ::-moz-tree-row(multicol, odd)

Ah, of course. Sorry for my confusion.
Attachment #444216 - Flags: review?(enndeakin) → review+
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.
(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.
Cool!

http://hg.mozilla.org/mozilla-central/rev/e719b565185c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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.
Blocks: 565460
Depends on: 566178
You need to log in before you can comment on or make changes to this bug.