Closed Bug 430852 Opened 16 years ago Closed 16 years ago

add attribute to restore treelines on XUL trees (treelines missing in MailNews)

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1a1

People

(Reporter: mnyromyr, Assigned: mnyromyr)

References

Details

(Keywords: dev-doc-complete, regression)

Attachments

(2 files, 5 obsolete files)

Treelines for Classic (proof of concept)
11.71 KB, patch
Details | Diff | Splinter Review
turn off treelines if attribute treelines is not true, xul.css version, v2 [checked in]
2.97 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
With bug 425131 "fixed", both Windows and Linux are missing treelines in trees in Classic. This is (a) violating the respective OS settings and (b) a real PITA when reading threaded mailing lists or usenet.

The "fix" was in global, so maybe we need a respective rule in communicator or messenger. Maybe we can work out a fix for both SM and TB, if they're interested?
Treelines missed not only mailnews, but bookmarks and preferences windows too.
(In reply to comment #0)
> Maybe we can work out a fix for both SM and TB, if they're
> interested?
> 

TB isn't affected.  They forked the widget theme and are still using a tree.css from 2006.
This is the patch to fix this one for SeaMonkey.
Attachment #317912 - Flags: superreview?
Attachment #317912 - Flags: review?(mnyromyr)
Attachment #317912 - Flags: superreview? → superreview?(mconnor)
Attachment #317912 - Flags: superreview?(mconnor)
Attachment #317912 - Flags: review?(mnyromyr)
Dropping review requests. The patch works if I build the following direction:

"make" in /suite/themes
"make" in /toolkit/themes

but then I have non-working checkboxes

the patch does not work if I build in the other direction (toolkit first). Then the checkboxes are OK, but the lines are still missing.

... and I don't know why this doesn't work...
ifdefs in toolkit are a non-starter anyway.  You should be able to specify this somewhere for seamonkey only, like we do when overriding behaviour for /browser.
(In reply to comment #6)
> ifdefs in toolkit are a non-starter anyway.  You should be able to specify this
> somewhere for seamonkey only, like we do when overriding behaviour for
> /browser.
I'm not sure what you mean here, you can't override a file just for one theme.
Even if ifdefs would be allowed, I still don't know why they don't seem to work there. The "MOZ_SUITE" just seems to be not existent while building and so the lines are still missing. Unfortunately the Mozilla build system is pretty complex and I just started to figure out how it works, so I don't know the reasons for those problems.

I don't think it's possible to override this one file. Maybe we could try to do this within the jar.mn in our classic theme. If this gets built after toolkit, then the file should be replaced. May work but IMHO this is a pretty silly hack.

For me this is just the first problem of this kind. In my opinion we will get much more of these. There will always be situations where something in the toolkit theme changes and we don't want this change. As long as we just import the global from toolkit, we'll get *any* change immediately in our SeaMonkey *primary* theme! Even changes we *don't* like!

Even worse if we are even unallowed to add ifdefs to toolkit, which seems to be the only "nice" solution, if we don't want to hack around.

I'm voting for forking the toolkit winstripe down to /suite/themes. If there, in future, will be really changes in toolkit that make our forked theme stop working, then we may easily port the patches from toolkit over to our theme. Thunderbird already did that and I think we should do the same.
I may be missing the point, but why don't we just override the tree styles in the chrome://communicator/ realm, like we do e.g. for preferences etc.?
No, we can't. This would be definetly hacking around!

We would have to do this in the CSS files for navigator, messenger, communicator->preferences, DOMI, ......

The loaded CSS file is defined in:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tree.xml#12

... and this is, again, a file under /toolkit.

As already written above: This is just the first issue like this. There will be more situations where something in Firefox gets changed and we immediately have this (more or less nice) change in SeaMonkey. Let's fork the files down to freeze the files for us and then lets fix in our fork.
About your point with the prefwindow: You load the communicator-CSS here:

http://lxr.mozilla.org/seamonkey/source/suite/common/pref/preferences.xul#39

Means: You load another CSS directly from XUL. That solution is OK for the SeaMonkey prefwindow, as other extensions don't have to use this. But this way we don't globally re-enable the lines for *all* trees in SeaMonkey. If we load the new CSS in prefwindow, then we still have no lines in mailnews. If we also fix in mailnews, DOMI is still missing, ... and who knows if we get our CSS loading patched into the sourcecode of DOMI...

... and extensions who use trees will have trees that look different to the trees built into SeaMonkey...

The default should be changed, Anything else are just hacks and no real solutions.
Actually, I tend to believe those so-called "missing" lines are actually the correct behavior for modern systems, and forking the default theme would be unacceptable in my eyes.
Robert, you can use "modern" theme.
This is not about what I want to use (I have my own theme in any case), this is about what's correct for the SeaMonkey default theme to do - and it's correct to do what the OS we're running on does by default.
If the OS is not using treelines by default, we also shouldn't use them. Of course, the ideal case would be to use what the OS is currently set to do, but this would need some work in nsITheme and I'm not even sure if we can get that data from the OS at all to follow it.
Ok. Then, OS allow to return this lines by 1 checkbox. Let's allow this change in default theme :)
AFAICT, not even the GtkTreeView widget cares about the system theme's setting for treelines, it's up to the application:
"The grid and tree lines are not drawn using the theme, and the only way to control them are the above style properties. The application decides whether to use them or not."
<http://live.gnome.org/GnomeArt/Tutorials/GtkThemes/GtkTreeView>

Given that even moderately threaded newsgroups will result in a blank subject area without treelines, we definitely need them in MailNews, and as long as can't get the native system setting, we should stick to the old behaviour - at least in MailNews.

(Bookmarks and preferences are probably less of an issue here, since they're usually not deeply nested.)

That said, I don't think that forking the default theme would be a good thing. It's just is a single "thing" we need to take care of here and do it differently...
Depends on: 425131
Depends on: 404100
This is a working ;-) proof of concept for returning treelines to SM.
But it's not actually as nice as it might could be, the tree styles probably should go in a file on their own.

Comments?
Attachment #317912 - Attachment is obsolete: true
Comment on attachment 318255 [details] [diff] [review]
Treelines for Classic (proof of concept)

Fishing for professional comments before stalking random sideproject folks. *g*
Attachment #318255 - Flags: superreview?(neil)
Comment on attachment 318255 [details] [diff] [review]
Treelines for Classic (proof of concept)

Why not use a % style for DOMI as well as Venkman?

Apparently Macs have never had dotted lines, do we want them on Mac too?
> Why not use a % style for DOMI as well as Venkman?

DOMI's trees are <pages> loaded into a <browser> - and there are 13 corresponding .xul files. Each of these does import chrome://inspector/skin/ which has only three content lines, so it's easy to supersede.

Actually, I would have loved to just style-overlay the global tree.css -
bug 297438, anyone? ;-)

> Apparently Macs have never had dotted lines, do we want them on Mac too?

It'd make sense for MailNews, but I need to check how it'll fit in (and whether the setting do even work there).
+tree[seltype="cell"] > treechildren::-moz-tree-line,
+tree[seltype="text"] > treechildren::-moz-tree-line,
+treechildren::-moz-tree-line {
+  border: 1px dotted ThreeDShadow;
+}
+
+tree[seltype="cell"] > treechildren::-moz-tree-line(active, selected, focus),
+treechildren::-moz-tree-line(selected, focus) {
+  border: 1px dotted HighlightText;
+}

Ugh, there is nothing here that you'd want for mac. If it's really that important (isn't this a pre-XP look?), make it platform-specific?
IMHO, it's wrong to do this on our side in any case. If at all, this should be in toolkit and have a pref switch that defaults to off to match defaults on all modern platforms.
> IMHO, it's wrong to do this on our side in any case.

Well, I'd love to not needing to do this, but see comment 6...

> this should be in toolkit and have a pref switch that defaults to off to 
> match defaults on all modern platforms.

This has absolutely *nothing* to do with "modern" or not.
This is about *usability*.

There's no system treeline style in Windows, and even Vista guidelines just want you to carefully consider your decision for/against. In fact, in many trees the depth of the tree isn't that important - items on the same level of a directory tree, for example, don't need to have the same importance or significance.

But MailNews trees are different, most prominently the news thread tree - postings have a strong hierarchical nature. It's not so much about being on the same level, but "who does the post *reply* to". Furthermore, threads tend to become deeply nested, with various subbranches, thus whitespace indented hierarchies get almost unreadable very fast.
(And, btw, reading news on Mac is rather unsatisfying currently, but that's the key point of this bug.)

To a lesser degree, the same holds for DOMI and Venkman, where the actual *path* to an element is important. Furthermore, like in MailNews, it's extremely hard to follow a hierarchy over several pages of data if you have absolutely no visual clue - pinning certain pixel by force of mind ;-) while scrolling isn't fun...

In theory, we could turn treelines on for certain trees and off for others, but *that* would be a visual inconsistency I honestly don't want.
Typos always find the most dangerous place. ;-)

(In reply to comment #23)
> (And, btw, reading news on Mac is rather unsatisfying currently, but that's

... NOT ...

> the key point of this bug.)
So what you're saying here is that only specific trees should have treelines, and the decision should depend on the developers of the specific component.
Probably the right thing to do in this case is to define a style for e.g. tree[treelines="true"] in toolkit CSS and then just add the treelines="true" attribute on trees where we specifically want it.
I think with quoting the docs you have looked into for this bug can convince toolkit people to add such a style.
I had a look at Karstens patch. It seems to be a bit hacky. But in the same way we may overlay the whole tree.css file. This makes *our* patch less complex, but as tree.css is pretty complex and maybe has differences in winstripe and gnomestripe, this may be a bad idea...

I like Roberts idea with tree[treelines="true"]. If we had this, then we may discuss which trees we like to have the lines. I think we should re-enable them everywhere to have all trees look equal, but we should discuss this in a separate bug as soon as toolkit is fixed.

Karsten: Could you please add me to CC as soon as you've created the bug?
Comment on attachment 318255 [details] [diff] [review]
Treelines for Classic (proof of concept)

I looked into our code and found about >100 trees, which we might have to change - given that submodules like DOMI or Venkman even would allow us to set this.
So I don't think this is feasible to fix this regression.

The cleanest way still to deal with this situation is IMO fixing bug 297438 and overlaying tree.css directly. (I may give that one a shot, but this is foreign territory for me.)

But in the meanwhile, I think my approach is a reasonable regression fix, I just need to ifdef out Mac in the communicator.css part. (Mac hasn't had treelines; and while I think this would be an improvement, Macies disagree - I just want to fix an obvious regression for Linux and Windows, not evangelize...).
Updated patch coming soon.
Attachment #318255 - Attachment is obsolete: true
Attachment #318255 - Flags: superreview?(neil)
Keywords: regression
I actually think forcing those ugly lines on all users and all trees would be an unacceptable regression to the much improved state we have right now. If at all, I'd prefer to have the treelines user-switchable. And IMHO, whatever is done _needs_ to be a toolkit feature and not a SeaMonkey hack.
> I actually think forcing those ugly lines on all users and all trees would be
> an unacceptable regression to the much improved state we have right now.

Well, that's the main problem: we all do think. ;-)

The facts are: We had treelines (and for good reasons!) and now we don't. There wasn't any decision to drop them, hence it's clearly a regression.

> If at all, I'd prefer to have the treelines user-switchable.

No disagreement here. 
If there was a pref, this could be done "easily" in PresShell::SetPreferenceStyleRules like we do for link colours and such.

> And IMHO, whatever is done _needs_ to be a toolkit feature and not a
> SeaMonkey hack.

That's where we disagree, and I have stated why am think like I think. 
This is no toolkit feature. There is no OS setting to follow. It's up to the application to define its use of treelines. 
(In reply to comment #29)
> That's where we disagree, and I have stated why am think like I think. 
> This is no toolkit feature. There is no OS setting to follow. It's up to the
> application to define its use of treelines. 

Only because OS toolkit developers themselves are weak and suck. ;-)

Actually, I think we should only use treelines on a few very selected trees if we use them anywhere, but NEVER on all trees in the app, as usually they only clutter the interface and there are only a handful (if at all) trees in the whole app where they get useful.
(In reply to comment #29)
> Well, that's the main problem: we all do think. ;-)

FULL ACK!

> The facts are: We had treelines (and for good reasons!) and now we don't.
> There wasn't any decision to drop them, hence it's clearly a regression.

FULL ACK again

Just because the Gnome developers and/or Microsoft does the mistake to remove the lines, the SeaMonkey team doesn't forcefully have to have this reduced usability.

I personally usually only see SeaMonkey and my goal is/would be to have a nice looking application with a interface that *helps* people to do their work.

Just once again: I strongly vote for *first* looking at usability. It's our choice how we design the SeaMonkey UI. Noone forces us to do this and such. Let's benefit from the freedom we have and lets make things *better*! Then, at *second* we may look what the others do.

Well, there may be places, where we don't forcefully need the treelines. For example we maybe don't need them in the preferences window, as we don't have very deep trees there. Maybe we also don't need them in the bookmarks manager, as I don't think someone really creates trees deeper than tree levels.

... but they won't hurt there if we would have them.

But what really hurts is to don't have treelines in Mail/News, DOMI and Venkman.

I aggree with Karsten: It may be problematic to get this fixed if we have to add something to DOMI and Venkman, so anything we could do, if KaiRo doesn't want all the trees with treelines, is to do it the other way around. We would have to default to "treelines enabled" and disable them where we don't want them.

@Karsten: Would it be possible to do something like this:
- Replace the original tree.css file with our version which just has our treeline-fixes, maybe with an check for an "treelines = false" attribute.
- Re-add the original tree.css renamed to, for example, "global-tree.css"
- Now @import the "global-tree.css" from our "tree.css"
(In reply to comment #31)
> Just because the Gnome developers and/or Microsoft does the mistake to remove
> the lines

It's no mistake to remove that ugly cruft.

> Well, there may be places, where we don't forcefully need the treelines.

I think we don't forcefully _need_ them anywhere, but I can understand a handful (at max) trees where someone could want them.

> I aggree with Karsten: It may be problematic to get this fixed if we have to
> add something to DOMI and Venkman

It's more problematic to get it fixed globally and not re-implement parts of the platform/toolkit.

> We would
> have to default to "treelines enabled" and disable them where we don't want
> them.

No, please, we ought to do it the other way round, and get a _general_ style of sucky-trees-with-lines into toolkit. If DOMI and venkman need them, they ought to use them regardless of the host app they are running in, which means that toolkit needs to support them.
And I don't see why anything other than the threadpane in threaded mode might want them in mailnews.
(In reply to comment #31)
> - Replace the original tree.css file with our version which just has our
> treeline-fixes, maybe with an check for an "treelines = false" attribute.
> - Re-add the original tree.css renamed to, for example, "global-tree.css"
> - Now @import the "global-tree.css" from our "tree.css"
We can't replace anything if we're to support XULRunner.
Even overriding tree.css is problemmatic as it affects every theme.
(In reply to comment #31)
> Just because the Gnome developers and/or Microsoft does the mistake to remove
> the lines, the SeaMonkey team doesn't forcefully have to have this reduced
> usability.

Agreed.

> Well, there may be places, where we don't forcefully need the treelines. For
> example we maybe don't need them in the preferences window, as we don't have
> very deep trees there. Maybe we also don't need them in the bookmarks manager,
> as I don't think someone really creates trees deeper than tree levels.

I think you would be surprised what some people do with their bookmarks, I think I have seen 10 levels deep, I have 6 levels myself. As for the preferences, I find the current state really weird, where sub-items just "hang in the air" like that. If they don't get tree lines they should at least get some other marker in front of them.

What's the way to proceed? KaiRo is strongly against it, Karsten for it. Do you vote on the council in such a case?
From IRC conversation, Karsten is looking into a pref solution, if that works out, we still can discuss defaults, but we seem to agree that this would be a good way to go.
Note that whatever happens those treelines are totally unacceptable on mac.
(In reply to comment #35)
A pref is inferior to an attribute. There's no right default or custom value if there are trees where it makes sense and trees where it doesn't make sense.

(In reply to comment #36)
> Note that whatever happens those treelines are totally unacceptable on mac.

As for the attribute, it would be up to the theme to respect it.
Assignee: mail → nobody
Component: MailNews: Main Mail Window → XUL Widgets
Product: Mozilla Application Suite → Toolkit
QA Contact: xul.widgets
Blocks: 404100, 425131
No longer depends on: 404100, 425131
Although the major platforms (Win, Linux/Gnome, Mac) don't even have a system setting for treelines, we can use our :-moz-system-metric mechanism to override Gecko's default (which is "draw treelines") with a pref. 
That way, interested apps/users can turn the treelines off/on, while themes still can provide override CSS to disable/enable them.

This patch:
- provides the new system metric 'treelines' which defaults to Gecko's default "draw treelines"
- provides default CSS for both disabled and enabled treelines
- disables treelines for Firefox and SeaMonkey on Mac
Assignee: nobody → mnyromyr
Status: NEW → ASSIGNED
Attachment #319463 - Flags: superreview?(neil)
Attachment #319463 - Flags: review?(roc)
(In reply to comment #38)
> That way, interested apps/users can turn the treelines off/on

This makes it seem like treelines would be application-specific while they are tree-specific. We don't want them in the Firefox history sidebar. We probably want them in DOM Inspector.

So how does this solve our problems? It all comes down to being able to point annoyed users to the hidden pref, which is, if at all, only a half-assed solution.
> This is no toolkit feature. There is no OS setting to follow. It's up to the
> application to define its use of treelines. 
> 
If anything, I think that this statement that you made in comment 29 necessitates the tree lines being a toolkit feature.  How are you planning on allowing apps to define their use?  By giving them an all-or-nothing option?  Let's think about what this would look like in the XUL documentation.  It's more sensible to tell app developers to use treelines="true" than it is to tell them "oh, BTW, you can add this line to the default prefs file".  Also keep in mind that the XUL ecosystem is such that most XUL applications are not standalone, but are extensions (DOMi) that could not rely on being able to control an application-wide preference like that.

If you want to give developers control, then I think that the treelines attribute is the only option.  If a preference is to be added, it should only be done so in conjunction with the attribute.
Attachment #319463 - Flags: superreview?(neil)
Attachment #319463 - Flags: review?(roc)
This alternative approach is based upon a new attribute "treelines" for <tree> elements:
On XBL creation,
- if the <tree> has an attribute "treelines", it governs whether this tree has treelines or not.
- if no such attribute is found, the pref "ui.treelines" is evaluated.
- if the pref doesn't exist either, Gecko's default rendering is used.
Attachment #319657 - Flags: superreview?(neil)
Attachment #319657 - Flags: review?
Attachment #319657 - Flags: review? → review?(mconnor)
Asking for blocking 1.9, since this regresses *all* toolkit consumers who don't have their own treeline CSS.
Flags: blocking1.9?
As far as I can see, it should be as easy as this:

tree:not([treelines="true"]) > treechildren::-moz-tree-line {
  visibility: hidden;
}

There's no platform where treelines should be used for all trees. Therefore ui.treelines=true doesn't make sense. If SeaMonkey happens to have only complex trees where treelines make sense, then just add the attribute to all trees. Or, if you think non-complex trees need treelines too, regardless of how the target platform handles this, then you could achieve this with custom CSS; I don't think toolkit code needs to support this.
> There's no platform where treelines should be used for all trees.

Users tend to disagree, you know. :-P

The pref is meant as a convenient way to enable/disable treelines for all trees,  and as such the main part of the actual *regression* fix.

> If SeaMonkey happens to have only complex
> trees where treelines make sense, then just add the attribute to all trees. 

That's well above 100 trees, thank you very much. DOMI alone has like 13 trees you'd need to touch then...

Toolkit as a real *XUL toolkit* should support both ways of treelines in the same easy manner.
Dao's proposal of adding an attribute would make Toolkit support both. I agree with him that this shouldn't be a pref. If SeaMonkey wants all of it's trees to have lines despite that not being the platform default on Windows, you can add the styling in your themes.
Comment on attachment 319657 [details] [diff] [review]
alternative approach: treelines-Attribut

And if particular users want all of their trees to have lines, they can use userChrome.css.
Attachment #319657 - Flags: review?(mconnor) → review-
This isn't going to block Gecko 1.9.
Flags: blocking1.9? → blocking1.9-
(In reply to comment #45)
> If SeaMonkey wants all of it's trees to
> have lines despite that not being the platform default on Windows, you can add
> the styling in your themes.

Well, from what I see, there's no unified opinion about that within the SeaMonkey or mailnews/Thunderbird(?) folks. Some want all trees to have lines, some want none, some want some.
Unfortunately, platforms don't have a consistent way of this either. Windows has a pref (defaulting to no lines) for Explorer at least, and has some apps that use treelines (by default, even in case of ones that are part of the default OS install) and some that don't. GNOME says nothing in the HIG and GTK says it's a decision of the app designers (see comment #16). KDE (as a reference, though we aren't building on it) seems to consistently use treelines on all trees (both KDE3 and KDE4). Mac OS X seems to consistently not use treelines on any trees.

So it's at best unclear what a toolkit that runs on all those platforms (at least Windows, GNOME and OS X) should do, as a mix of "use a pref", "let app people decide" and "consistently do one thing" is used across the platforms.

Probably the solution that would fit most individually would be to have a treelines attribute that can be explicitely set to true or false by the app maintainers, and all where this is undefined (or, say, auto) are default, which means depending on a pref.
This solution might be, for some people, overengineered though, so it seems we need to find a reasonable compromise.
To be clear, I'm fine with adding the attribute that controls whether the treelines appear. I just don't think there needs to be a pref, or a property on the binding.
> To be clear, I'm fine with adding the attribute that controls whether the
> treelines appear. I just don't think there needs to be a pref,

So at least we have a bit of common ground here:
show treelines:       <tree treelines="true">
don't show treelines: <tree treelines="false">

Since this attribute will be new, most our trees and "in the wild" won't have it. Without the pref, the tree's treelines mode will fall back unto "the default".
For Gecko, this is and has been for almost 10 years to draw treelines, and thus is the expected behaviour (at least by customary law).

Now the lazy hacks used in tree.css turn off treelines for *everyone* using the chrome://global/skin/, without a decent way to override that (except including its own stylesheet *everywhere* where chrome://global/skin/ is used or overriding the global tree.css as a whole).

Let's look (roughly) at how many trees are affected in the current codebase (searching MXR for "<tree " without the "", and ignoring any tests and such in the results):

/toolkit/mozapps: 1
/toolkit/components: 8
/browser: 12
/mail: 15
/mailnews: 22
/calendar: 4
/suite: 24

/extensions/inspector: 17
/extensions/venkman: 8
rest of /extensions: 14

/editor: 5
/security/manager/pki: 10


Given we use a treeline attribute:
- Using Gecko's default ("treelines on") means fixing about 20 trees for FF, almost all under /browser.
- Using "treelines off" means touching about 120 trees spread through all our codebase.


The unannounced/undiscussed tree.css changes of a long-time default in *toolkit* to achieve a certain goal in *Firefox* without caring a bit about other trees in the codebase or "out there in the wild" can't have been seriously thought out...
I really do hope the consequences were just not accounted for - else it would be like ... sabotage.

I'll happily write a fix to attach treelines="false" to all FF trees. :)

> or a property on the binding.

No doubt, the property is not important. It's just a convenience, but most probably not worth the effort.

"trees shouldn't have treelines by default" was not a Firefox decision, it was a Toolkit decision, and we did it because that is what platform standards dictate, at least in the majority of cases. There are other cases where we still want lines, and those cases should be supported using an attribute.

This has nothing to do with SeaMonkey vs. Firefox.
(In reply to comment #51)
> "trees shouldn't have treelines by default" was not a Firefox decision, it was
> a Toolkit decision, and we did it because that is what platform standards
> dictate, at least in the majority of cases.

But, as Robert wrote in Comment #48, even windows has some apps with treelines enabled and some with treelines disabled. In Explorer it's possible to change the setting with just one checkbox.

> There are other cases where we
> still want lines, and those cases should be supported using an attribute.

FULL ACK for this, but I think we also need the pref to override the treeline generate setting if the "treelines" attribute isn't set.

If we don't have the pref, then we have to get the Venkman and DOMI people to add the attribute "treelines = true". If they don't want to add this, then we have exactly the same problem, again, we currently have.

If the new default really has to be "treelines off", then I would modify the code of "_initTreelinesAttributeFromPref" in Karstens patch in the following way.

This code:

<code>
...
if (this.hasAttribute('treelines'))
  return;

var treelines = true; // Gecko's draws treelines by default.
try {
  treelines = Components.classes["@mozilla.org/preferences-service;1"]
              .getService(Components.interfaces.nsIPrefBranch)
              .getBoolPref('ui.treelines');
} catch (e) {}
this.treelines = treelines;
</code>

gets:

<code>
...
// If there is no treelines attribute, try to set it with "ui.treelines" pref
if (!this.hasAttribute('treelines')) {
  try {
    var treelines = Components.classes["@mozilla.org/preferences-service;1"]
                   .getService(Components.interfaces.nsIPrefBranch)
                   .getBoolPref('ui.treelines');
    this.treelines = treelines;
  } catch (e) {}
}
</code>

This smaller piece of code could get directly into the constructor and so there is now new method needed --> less change in codebase.

If the pref is not set at all (Firefox wouldn't set it by default), then this code does nothing, so the default set in toolkit or in the theme gets applied. If the pref is set, then all trees, where the author didn't decide how to display the tree, get the default value for "treelines" by the pref.

The goal of the pref is to modify the default for trees where noone specified if treelines have to be displayed! The user still has to use userChrome.css to override the setting of the treelines-Attribute.

I could upload a modified version of Karsten's patch if I don't get a veto before I get my patch finished.
> // If there is no treelines attribute, try to set it with "ui.treelines" pref
> if (!this.hasAttribute('treelines')) {
>   try {
>     var treelines = Components.classes["@mozilla.org/preferences-service;1"]
>                    .getService(Components.interfaces.nsIPrefBranch)
>                    .getBoolPref('ui.treelines');
>     this.treelines = treelines;
>   } catch (e) {}
> }

The property isn't needed, so this.treelines won't work. Maybe this, then:

// If there is no treelines attribute, try the "ui.treelines" pref.
// If there is no such pref either, let the toolkit default rule.
if (!this.hasAttribute('treelines')) {
  try {
    if (Components.classes["@mozilla.org/preferences-service;1"]
                  .getService(Components.interfaces.nsIPrefBranch)
                  .getBoolPref('ui.treelines'))
      this.setAttribute('treelines', true);
  } catch (e) {}
}

But this all is a mess fixing bug 297438 would (kind of) solve - I'll give that one a stab when I'm back and this regression here is still open. :-/
(In reply to comment #50)
> - Using "treelines off" means touching about 120 trees spread through all our
> codebase.

I disagree. I don't believe we would want more than a handful of trees with treelines in that case.

The problem here is that Karsten and Manuel want a KDE-style solution of treelines everywhere, while is seems that gavin and me (and those people who did the change in the toolkit themes) want a Mac-style solution of no treelines at all. Karsten and Manuel are willing to give the almost-Windows-style pref solution a try (Windows does have the pref in some apps, treelines in others, none in yet others), gavin is willing to give the GNOME/GTK-style solution of letting the app devs decide (which is no real solution IMHO) a try.

The truth is that there is no toolkit platform we (as in "the Mozilla platform") support that has treelines everywhere by default (we have no KDE support at all), and so the toolkit default _needs_ to be no treelines on all three default sub-themes (if we ever add KDE support we might need to revisit that for Linux).

So there clearly are people who would like treelines everywhere, people who need them in a _few_ selected trees that tend to be deeply nested, and other people who don't want the lines at all.
(In reply to comment #50)
> - Using "treelines off" means touching about 120 trees spread through all our
> codebase.

Err, no. The idea is to enable treelines where it makes sense, not everywhere.

(In reply to comment #52)
> (In reply to comment #51)
> > "trees shouldn't have treelines by default" was not a Firefox decision, it was
> > a Toolkit decision, and we did it because that is what platform standards
> > dictate, at least in the majority of cases.
> 
> But, as Robert wrote in Comment #48, even windows has some apps with treelines
> enabled and some with treelines disabled. In Explorer it's possible to change
> the setting with just one checkbox.

Yes, Windows has some apps with treelines, and we still want to support such cases. Those apps mostly consist of one big tree though, which hardly means that an app-wide pref is needed. An attribute would work fine for them. Implementing an Explorer-like checkbox would be as easy as setting and persisting the attribute.
As for KDE support: A hypothetical Qt toolkit theme could chose to always draw treelines regardless of whether treelines="true" is set, just like we never draw them on Mac.
> > - Using "treelines off" means touching about 120 trees spread through all
> > codebase.
> 
> Err, no. The idea is to enable treelines where it makes sense, not everywhere.

That was just the worstcase: there are at least six(!) times more trees affected  outside of FF than FF contains! All of them need to be looked at, by lots of different people and projects. This is a lot of work for a lot of people who didn't call for that change.

The actual global default (on or off) is not a problem.
Being able to correct it *easily* shouldn't be - I'm really quite amazed that reading a possibly even non-existing(!) pref once(!) on tree creation is such a big deal...
I think you're exaggerating. There's no need to review 120 trees, at least in a world where most trees aren't overly complex and thus don't need the lines. Just add them back where you actually miss them. Personally I miss them in DOM Inspector, and I'd be happy to patch it. In the end that's more straightforward than starting with a platform- and app-dependent pref, mixed with the attribute.
(In reply to comment #46)
> And if particular users want all of their trees to have lines, they can use
> userChrome.css.
Should we leave the line colours in tree.css?
(In reply to comment #59)
> (In reply to comment #46)
> > And if particular users want all of their trees to have lines, they can use
> > userChrome.css.
>
> Should we leave the line colours in tree.css?

Doesn't really bother me either way. I think we should make the non-contentious attribute change for 1.9, time is running out.
This patch turns off treelines for *stripe, if the treelines attribute is missing or not true. I did not set the visibility:hidden in xul.css, so that in case of other platform themes we won't need to change it there again.

JFTR: 
I still think that this is not the right way to fix this half-assed world break. :-P
Attachment #319811 - Flags: review?(gavin.sharp)
Attachment #319657 - Flags: superreview?(neil)
Comment on attachment 319811 [details] [diff] [review]
turn off treelines if attribute treelines is not true

>Index: toolkit/themes/gnomestripe/global/tree.css

>+tree[seltype="cell"] > treechildren::-moz-tree-line,
>+tree[seltype="text"] > treechildren::-moz-tree-line,
>+treechildren::-moz-tree-line {
>+  border: 1px dotted ThreeDShadow;
>+}
>+tree[seltype="cell"] > treechildren::-moz-tree-line(active, selected, focus),
>+treechildren::-moz-tree-line(selected, focus) {
>+  border: 1px dotted HighlightText;
>+}

>Index: toolkit/themes/pinstripe/global/tree.css

>+tree[seltype="cell"] > treechildren::-moz-tree-line,
>+tree[seltype="text"] > treechildren::-moz-tree-line,
>+treechildren::-moz-tree-line {
>+  border: 1px dotted ThreeDShadow;
>+}
>+tree[seltype="cell"] > treechildren::-moz-tree-line(active, selected, focus),
>+treechildren::-moz-tree-line(selected, focus) {
>+  border: 1px dotted HighlightText;
>+}

I'm not sure that these colors are correct for Linux or Mac - Mac already specifies a border style/color, perhaps you should just reuse it?
> I'm not sure that these colors are correct for Linux or Mac - Mac already
> specifies a border style/color, perhaps you should just reuse it?

Oops, I didn't intend to change pinstripe at all, please assume it's not in the patch.

As for Linux: without any border rules, the treelines colour (at least under KDE) seems to be random, so I, given the overall resemblance of gnomestripe and winstripe, picked the ones from winstripe. Any suggestions?


As a toolkit feature, I'd prefer putting it in xul.css so that "no treelines unless treelines=true" is our new global default, which would affect third-party themes too. If a theme doesn't want to support the attribute, it would have to hide/show the lines and override xul.css explicitly like pinstripe.

Is there a reason why you're not using the child selector?
Comment on attachment 319811 [details] [diff] [review]
turn off treelines if attribute treelines is not true

>+tree[seltype="cell"] > treechildren::-moz-tree-line,
>+tree[seltype="text"] > treechildren::-moz-tree-line,
>+treechildren::-moz-tree-line {
>+  border: 1px dotted ThreeDShadow;
>+}


Are |tree[seltype="cell"] >| and |tree[seltype="text"] >| really needed?
(In reply to comment #65)
> Are |tree[seltype="cell"] >| and |tree[seltype="text"] >| really needed?
> 

I think so.  It seems that the treelines at <http://mxr.mozilla.org/mozilla/source/toolkit/content/tests/widgets/test_tree_hier_cell.xul> don't show up without them (at least, they failed to show up when, thinking that they were superfluous, I had those lines removed from my userChrome.css).
I did some tests and I think these are the minimal rules for seltype=cell/text/- to work properly.
Attachment #319811 - Attachment is obsolete: true
Attachment #319865 - Flags: review?(gavin.sharp)
Attachment #319811 - Flags: review?(gavin.sharp)
Why is the seltype="text" rule needed, but not seltype="cell"? Would be best to just revert to the previous Windows styling (and copy that to gnomestripe) unless you're sure that you've covered all the cases. pinstripe should also have lines in the skin file if we're going this route, which probably just means removing the visibility: hidden line at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/pinstripe/global/tree.css&rev=1.18&mark=159#153 .

Otherwise this looks fine.
(In reply to comment #68)
> Why is the seltype="text" rule needed, but not seltype="cell"?
Selecting lines or cells changes their background, so that the line colour needs to change too. The style rules therefore normally change the line colour, but this is incorrect for seltype="text" so an extra rule is needed to set it back.
Ah, looking at some old styles I see that although Mnyromyr's code works for row and text selection he omitted the cell selection rules which means that things aren't going to work for inactive primary cells.
(In reply to comment #68)
> Would be best to
> just revert to the previous Windows styling (and copy that to gnomestripe)

Seems like bug 118312 made that stuff obsolete for gnomestripe.

> pinstripe should also
> have lines in the skin file if we're going this route, which probably just
> means removing the visibility: hidden line at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/pinstripe/global/tree.css&rev=1.18&mark=159#153

We didn't draw treelines on Mac for ages, and I think there was agreement that we don't want to change that.
(In reply to comment #65)
>(From update of attachment 319811 [details] [diff] [review])
>>+tree[seltype="cell"] > treechildren::-moz-tree-line,
>>+tree[seltype="text"] > treechildren::-moz-tree-line,
>>+treechildren::-moz-tree-line {
>>+  border: 1px dotted ThreeDShadow;
>>+}
>Are |tree[seltype="cell"] >| and |tree[seltype="text"] >| really needed?
Yes. Normally trees draw their lines in ThreeDShadow, but selected lines have a Highlight background so in that case the line colour gets changed to HighlightText. Now this is wrong for cell and text selections, where the lines should always have a ThreeDShadow colour, except in the case of an active primary cell (not text) line which again has a HightlightText colour.
(In reply to comment #71)
> We didn't draw treelines on Mac for ages, and I think there was agreement that
> we don't want to change that.

It would be strange to have an attribute on trees that doesn't work consistently across platforms. Since the default behavior on all platforms now is to have no treelines (is that right?), it seems like we would want Mac to support treelines="true". Perhaps that can be addressed separately, I guess.
I'd spec it like this: treelines="true" enables tree lines where they are allowed but not enforced. For platform consistency or aesthetic reasons, themes are allowed to enable or disable tree lines for all trees regardless.
(In reply to comment #73)
> Since the default behavior on all platforms now
> is to have no treelines (is that right?), it seems like we would want Mac to
> support treelines="true".

The difference is that Windows and GNOME still support tree lines; on Mac they are foreign.
(In reply to comment #75)
> (In reply to comment #73)
> > Since the default behavior on all platforms now
> > is to have no treelines (is that right?), it seems like we would want Mac to
> > support treelines="true".
> 
> The difference is that Windows and GNOME still support tree lines; on Mac they
> are foreign.
> 

Indeed. In the case that a xul file has treelines="true", I don't think you shall be forced to overrule that for mac. It'd make more sense if you where forced to overrule the "visibility: hidden;" in order to make treelines appear on mac.

OK, that makes sense.
Comment on attachment 319865 [details] [diff] [review]
turn off treelines if attribute treelines is not true, xul.css version

So per Neil's comments,  this patch isn't right - we should just restore the original style rules (and probably avoid changing gnomestripe).
Attachment #319865 - Flags: review?(gavin.sharp) → review-
This patch:
- disables treelines in xul.css for trees without treelines="true"
- doesn't touch pinstripe's global treeline disabling
- removes the now redundant global treeline disabling in gnomestripe
- restores the treeline rules in winstripe without the now redundant treeline disabling
Attachment #319463 - Attachment is obsolete: true
Attachment #319657 - Attachment is obsolete: true
Attachment #319865 - Attachment is obsolete: true
Attachment #321307 - Flags: review?(gavin.sharp)
I filed bug 434080 on the random treeline colour bug in GTK mentioned in comment 63.
Attachment #321307 - Flags: review?(gavin.sharp) → review+
Comment on attachment 321307 [details] [diff] [review]
turn off treelines if attribute treelines is not true, xul.css version, v2 [checked in]

Simple fix now, which moves treeline mode from theme CSS to XUL CSS, introducing  a new (not yet used) attribute for enabling treelines.
Attachment #321307 - Flags: approval1.9?
Renominating for blocking1.9, because it has very simple fix now and this toolkit(!) regression as such will affect all toolkit-based apps.
Flags: blocking1.9- → blocking1.9?
Whiteboard: [RC2?]
Flags: wanted1.9.0.x?
As safe as this is, I don't see a pressing need to take it before we ship, so moving it off to 1.9.0.1.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Flags: blocking1.9+
Whiteboard: [RC2?]
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1+
Flags: blocking1.9-
Flags: blocking1.9+
This is ready to land on trunk (mozilla-central), right?
Right. I'm not yet hg-ready, thus setting whiteboard.
Whiteboard: [checkin-needed of attachment #321307; but keep bug open afterwards!]
Keywords: checkin-needed
Checked in to mozilla-central, changeset:   15312:504fa3084eba
Keywords: checkin-needed
Whiteboard: [checkin-needed of attachment #321307; but keep bug open afterwards!]
Attachment #321307 - Attachment description: turn off treelines if attribute treelines is not true, xul.css version, v2 → turn off treelines if attribute treelines is not true, xul.css version, v2 [checked in]
Attachment #321307 - Flags: approval1.9?
You need to ask for approval if you want to land this on the branch.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Summary: [Classic] Missing Treelines in MailNews → add attribute to restore treelines on XUL trees (treelines missing in MailNews)
Target Milestone: --- → mozilla1.9
Attachment #318255 - Attachment is obsolete: false
Blocks: 438793
Any idea when this fix will be manifest in SeaMonkey trunk?

(In reply to comment #89)
> Any idea when this fix will be manifest in SeaMonkey trunk?

Once the attribute has been added to the relevant trees. See bug 438793.
To correct, this only will only be in SeaMonkey once we add the attribute like Dão mentioned _and_ SeaMonkey will be based on mozilla-central, see bug 437643.
Comment on attachment 321307 [details] [diff] [review]
turn off treelines if attribute treelines is not true, xul.css version, v2 [checked in]

I understand that Mark's checkin into Hg (comment 87) was into 1.9.1 aka Toolkit Trunk.

Requesting approval for CVS HEAD, which I understand is now known as 1.9.0. 

Please, anyone, who has a clear picture about which trunk/branch/HEAD/tip/whatever belongs to which code fragments, please correct  me. And do a wiki page or something, since existing newsgroup postings or IRC knowledge are rather woozy. :-(
Attachment #321307 - Flags: approval1.9?
Target Milestone: mozilla1.9 → mozilla1.9.1
Given bug 437643, is this still needed on CVS trunk? If it isn't, somebody should probably clear the blocking1.9.0.1 flag.
If bug 437643 is going to happen after July 1, then yes, I think this bug 
should still block 1901.
Blocks: 418623
> Given bug 437643, is this still needed on CVS trunk?

Yes, since that bug's resolution is not imminent, given the number of trees and participating sub-projects.
So any approval of the patch would still be very appreciated. ;-)
(In reply to comment #94)
> If bug 437643 is going to happen after July 1, then yes, I think this bug 
> should still block 1901.

This bug won't happen until after July 1, so I'm removing the blocking flag, but adding wanted. If I get a patch approval request with an explanation of risk we can evaluate this for the 1.9.0.x branch, esp. if SeaMonkey2 requires it.
Flags: blocking1.9.0.1+ → blocking1.9.0.1-
Comment on attachment 321307 [details] [diff] [review]
turn off treelines if attribute treelines is not true, xul.css version, v2 [checked in]

The patch restores rules recently deleted, but only for trees with an up-to-now unused attribute, and moves the activation rule into xul.css, again with that very attribute. 
So the risk is fairly minimal.
Attachment #321307 - Flags: approval1.9? → approval1.9.0.1?
Attachment #321307 - Flags: approval1.9.0.1? → approval1.9.0.2?
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
Since mailnews moved to comm-central/1.9.1, is this still wanted on the 1.9 branch?
Blocks: 448456
Attachment #321307 - Flags: approval1.9.0.2?
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: