* The checkmarks aren't ideally placed yet. Since real checkboxes can't be displayed in a tree, we might rather want something like in Thunderbird's Subscribe to Newsgroup dialog (i.e. empty square/square with checkmark). * Icons don't display correctly on Gnome (cf. bug 448976 comment #32). * Maybe the order of the buttons should change for Gnome and OS X?

- I feel that this style is necessary. treechildren::-moz-tree-row(alternate, selected) { background-color: -moz-cellhighlight; } - In the third party theme that displays treechildren::-moz-tree-line, the terminal of treechildren::-moz-tree-line in the last tree needs improving. /* display the tree line */ treechildren::-moz-tree-line { visibility: visible; }

I found the time to make a patch for GNOME. This will at least fix the icons and any big HIG discrepancies.

Comment on attachment 342769 [details] [diff] [review] Patch for Linux > #buttons > button { >- margin-top: 2em; >+ margin-top: 1em; >+ -moz-margin-start: 5px; > } The margin-top is mirrored from <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/netError.css#85>, so for consistency's sake we might want not to change that (or change it in netError.css as well). Otherwise r+=me and thanks!

Just got rid of the rule change.

Comment on attachment 342774 [details] [diff] [review] Patch for Linux 2 (checked in) >-/* XXXzeniko will need an appropriate color here (or none?) */ > treechildren::-moz-tree-row(alternate) { >- background-color: #EEEEEE; This needs to be fixed on Windows as well for a11y. Either remove it or use rgba.

Comment on attachment 342774 [details] [diff] [review] Patch for Linux 2 (checked in) http://hg.mozilla.org/mozilla-central/rev/27efd8a373aa

This also fixes the first point of comment #1 (which might affect the other platforms as well).

Comment on attachment 342788 [details] [diff] [review] use rgba for the alternate windows I think we should just remove this for Windows.

Alright, lets drop this nonnative bit completely.

Comment on attachment 342794 [details] [diff] [review] no alternate color on Windows (checked in) http://hg.mozilla.org/mozilla-central/rev/f9dbebe66234

When I go to about:sessionrestore now every other tab/item I click on the name of the tab disappears. Will this be fixed in final release for this? Als, what is the point of having the list like that if I can't uncheck a tab and not have it restored?

(In reply to comment #11) A screenshot or even your build identifier would quite help in determining your issue (as this is most likely OS dependent). As for unchecking: have you tried clicking on the checkmark in the Restore column (and if the checkmark is missing, that'd be another bug to fix).

Markus: You seem to be quite invested in polishing our OS X theme. Would you mind having a look at the new about:sessionrestore page? I'm quite sure that it must be somehow wrong on OS X (not having been able to test it myself). Thanks.

This is about:sessionrestore with the first item selected. Notice how it doesn't highlight in blue but instead erases the text of my first tab. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre

This is about:sessionrestore with the second item selected. Notice it highlights in blue as it should unlike with the first screenshot. This happens for every other tab. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre

(In reply to comment #12) > (In reply to comment #11) > A screenshot or even your build identifier would quite help in determining your > issue (as this is most likely OS dependent). As for unchecking: have you tried > clicking on the checkmark in the Restore column (and if the checkmark is > missing, that'd be another bug to fix). Simon, at least on OS X the checkbox isn't visible. Shall I file a new bug on that? Further I can also see that for each odd row the textcolor is changed to white when selecting the entry.

(In reply to comment #16) > Simon, at least on OS X the checkbox isn't visible. That's bug 459746... > Further I can also see that for each odd row the textcolor is changed ... and this bug 459740. For both bugs, patches are welcome!

The OS X polish should really happen before Beta 2 if we don't want a whole lot of duplicates of the three bugs this one depends on.

I'll start working on them tomorrow.

Comment on attachment 342774 [details] [diff] [review] Patch for Linux 2 (checked in) >diff -r f6ed4aa2071c browser/themes/gnomestripe/browser/aboutSessionRestore.css > #buttons { > width: 100%; >+ -moz-box-direction: reverse; Can we actually hardcode this in the page's source with ifdefs? This way you'd still have the platform order when using a third-party theme. This is directly related to bug 459751.

As for bug 459740, I'd suggest just dropping the whole "alternate" rules. I'd originally planned to use alternate colors for different windows instead of different lines - but as I've proven myself, that's quite easy to get quite wrong...

This fixes bug 459740 and adds the missing icons (checkmarks, question mark, folder). I didn't turn off the "alternate" styling because on OS X every tree has that styling. Instead I made the general styles in tree.css apply to trees without a column picker, too, and removed them in aboutSessionRestore.css. I'm not really happy with the appearance yet but I think this is good enough for now.

Comment on attachment 344868 [details] [diff] [review] Patch for OS X, v1 (In reply to comment #22) > I didn't turn off the "alternate" styling because on OS X every tree > has that styling. Part of the issue is that "alternate" and "odd" aren't the same in this case ("odd" changes per line, "alternate" changes per window). > Instead I made the general styles in tree.css apply to trees without a > column picker, too, and removed them in aboutSessionRestore.css. You'll need review from a Toolkit peer for that change, since I haven't got the slightest idea why trees without column pickers are to be treated differently. r+=me and thanks for the rest.

(In reply to comment #20) > Can we actually hardcode this in the page's source with ifdefs? Good idea. Would you mind offering a patch over in bug 459751 (also removing the then unneeded Gnomestripe rule)?

Yeah, I can do that.

Comment on attachment 344868 [details] [diff] [review] Patch for OS X, v1 >diff --git a/browser/themes/pinstripe/browser/aboutSessionRestore.css b/browser/themes/pinstripe/browser/aboutSessionRestore.css > treechildren::-moz-tree-image(container, open, noicon) { >- -moz-image-region: rect(16px, 32px, 32px, 16px); > } Oops, forgot to qrefresh. The other two lines should be removed as well, of course. (In reply to comment #23) > (From update of attachment 344868 [details] [diff] [review]) > (In reply to comment #22) > > I didn't turn off the "alternate" styling because on OS X every tree > > has that styling. > > Part of the issue is that "alternate" and "odd" aren't the same in this case > ("odd" changes per line, "alternate" changes per window). Ah, I didn't notice that. Ryan, could you review the tree.css change?

I checked this patch in without the tree.css change. http://hg.mozilla.org/mozilla-central/rev/272cab893e34

Even though the bug is marked as fixed we should add bug 462618 and bug 462620 to the dependencies list for open OS X issues.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2