Last Comment Bug 212789 - Horizontal Scrolling for XUL Tree Controls [Will Implement]
: Horizontal Scrolling for XUL Tree Controls [Will Implement]
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- enhancement with 6 votes (vote)
: ---
Assigned To: Stef Walter
:
Mentors:
: 319448 (view as bug list)
Depends on: 307062 213103 221619 232913 291198 306575 306971 306990 308490 308520 310668 310710 330236
Blocks: 112832 371592 328948 330170 348764 399961
  Show dependency treegraph
 
Reported: 2003-07-15 11:29 PDT by Stef Walter
Modified: 2008-07-31 03:13 PDT (History)
34 users (show)
asa: blocking1.8b2-
benjamin: blocking1.8b3-
benjamin: blocking‑aviary1.5-
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial implementation (48.54 KB, patch)
2003-07-21 17:16 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Test XUL which exercises new functionality (5.40 KB, application/vnd.mozilla.xul+xml)
2003-07-21 17:20 PDT, Stef Walter
no flags Details
Initial implementation (corrected) (51.70 KB, patch)
2003-07-21 18:18 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation with tree column picker (53.74 KB, patch)
2003-07-31 17:34 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation patch updated (sync with trunk, fixes) (64.80 KB, patch)
2003-08-07 15:59 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation patch with overflow fixes (63.50 KB, patch)
2003-08-15 12:23 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation patch updated (sync with trunk, fixes) (63.77 KB, patch)
2003-08-26 19:34 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation patch updated (sync with trunk, fixes) (63.77 KB, patch)
2003-09-09 11:42 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Implementation patch updated (sync with trunk) (63.83 KB, patch)
2003-10-18 23:19 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Proof of concept patch handling scrolling internal to the tree (21.66 KB, patch)
2004-08-30 11:06 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch updated and synced with Trunk (21.75 KB, patch)
2005-04-20 11:23 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch: Nearly There (39.37 KB, patch)
2005-04-21 12:39 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Without further ado: Horizontal Scrolling for Trees (47.52 KB, patch)
2005-04-21 17:58 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch: Fixes flex (ie: normal non-horz-scroll) trees (47.74 KB, patch)
2005-04-22 09:32 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch: Now with proper variable scoping (47.75 KB, patch)
2005-04-22 13:41 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch: Now for all you other trees out there, a little something special. (48.04 KB, patch)
2005-04-23 13:01 PDT, Stef Walter
no flags Details | Diff | Splinter Review
Patch: Rounding Errors R Us (48.33 KB, patch)
2005-04-23 15:46 PDT, Stef Walter
bzbarsky: review+
roc: superreview+
Details | Diff | Splinter Review
Last patch updated to tip (48.31 KB, patch)
2005-08-30 11:33 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Additional patch that was needed to fix bustage (1.87 KB, patch)
2005-08-30 11:56 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Another additional patch, similar reasons. (1.30 KB, patch)
2005-08-30 12:23 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Fix issues from comment 74. (9.47 KB, patch)
2005-08-30 21:36 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Fix scrolling properly (checked in) (3.19 KB, patch)
2005-08-31 06:23 PDT, neil@parkwaycc.co.uk
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Test showing clipped text beyond original viewable area of tree (14.70 KB, application/vnd.mozilla.xul+xml)
2006-10-27 06:07 PDT, charles stephens
no flags Details
Screenshot of horizontal scrolling in Explorer (55.35 KB, image/jpeg)
2007-02-23 20:13 PST, Mime Čuvalo
no flags Details

Description Stef Walter 2003-07-15 11:29:29 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

I'm developing an application using Mozilla. Let's just say
it's been a bit of a love/hate relationship :) This application 
is an LDAP Data Environment. Sort of an 'MS Access' for LDAP. 

One problem I've run into is serious limitations with the tree
control. I'm trying to use it as a data grid. When adding 
horizontal scrolling (which I have working in a lobotomized way)
the control fails in 'creative' ways.

My employer is willing to have me implement this feature 
(horizontal scrolling), and then donate the changes back to 
the mozilla project. 

However he is only willing to do this if there is a chance 
that these changes will be accepted into the browser. One of 
our design goals is eventually to be able to run this application
on any mozilla browser ('past version x'). 

I've noted many requests for this feature in newsgroups, and I
believe certain features in the browser would be better off for it:

 - Mail/Newsgroups
 - DOM Inspector
 - Deeply Nested Bookmarks

I believe I can implement this in a competent and complete manner. 
But perhaps I'm missing something. Are there any serious issues 
(besides time and work) preventing the implementation of this 
feature, that I'm missing? See details below.

Naturally I would not change the default behavior of the tree 
control but rather add a switch which turns on the horizontal
scrolling behavior.

Thanks in advance for any ideas, gotchas and other input.



Rough Implementation Details

 - Implement horizontal scrolling for the treecols element. 
   This would require that the nsTreeBodyFrame be less delicate
   with regards to locating treecol frames.
 - Changes to nsTreeBodyFrame to have painting "follow" the 
   horizontal scrolling of the treecols frame, and clip 
   appropriately.
 - Fixing nsSplitterFrame so that it works within a scrolled 
   box. Currently screws up and doesn't 'release' the mouse.
 - I'm sure there will be other issues as well, but that's a 
   rough idea.


Reproducible: Always

Steps to Reproduce:
Comment 1 David Hyatt 2003-07-15 12:12:23 PDT
Yes, this is definitely a desirable feature.  I would use a CSS property that let you pick it. 

 -moz-tree-layout: fixed or auto

Then you could have an attribute on <tree> and use xul.css...

tree[layout=fixed] { -moz-tree-layout: fixed; }
Comment 2 Boris Zbarsky [:bz] 2003-07-15 12:18:12 PDT
Well, since hyatt is the module owner of this stuff and all that....  For my
part, I would say we definitely want this as an option for trees.

Over to Nate, since he plans to work on this.  And thank you in advance!
Comment 3 Stef Walter 2003-07-18 09:57:23 PDT
Okay. I have the basics working. It's implemented like this:

- Horizontal scrolling is facilitated by a nsIScrollableView
  (style='overflow: -moz-horizontal-scroll;') around treecols 
  and treerows elements. 

- nsTreeBodyFrame finds the horizontal scrollbar associated 
  with the above ScrollableView and gives it certain CSS classes. 
  This allows us to control the scrollbar visibility from XBL. 
  Also allows anyone who's used .tree-scrollbar to hide the 
  scrollbars or change their appearance to automatically gain 
  control over the horizontal scrollbar.

- The visibility of the horizontal and vertical scrollbars are 
  controlled using CSS.

- nsTreeBodyFrame searches the entire sub-tree for the treecols 
  element rather than just immediate children.

- nsTreeBodyFrame makes sure to use only vertically oriented 
  scrollbars for it's vertical scrollbar stuff.

If anyone wants patches against what I have now let me know. 
Otherwise I'll just post the patches when I'm done with the 
things below.


THOUGHTS:

- Initially didn't use the automatic horizontal scrollbar built
  into the scrollable view. But syncing the maxpos, increment,
  etc... was too tedious. nsIScrollableView provides methods for
  listening to scroll position, but nothing else.
  It would be nice if there was a way to plug external scrollbars
  into a scrollable view.

- The showing and hiding of scrollbars appropriately works but is 
  sort of delicate presently. This is mainly to the fact that 
  overflow and underflow events don't seem to hold any reference 
  to the originating element. Correct me if I'm wrong here...

- Although using CSS styles as hyatt suggested is interesting, 
  currently a much simpler solution is employed. If there aren't any
  flexible treecol elements, the treecols element overflows and
  the horizontal scrollbar is shown automatically. Is this okay with
  everyone?


LEFT TO DO (I'll be working on these next):

- Splitter needs some fixing to work inside a scrollable view.

- nsTreeBodyFrame should fire a scroll event when scrolling vertically.
  Horizontal scrolling already does this.

- nsITreeBoxObject.getCellAt needs to be fixed to take the scrolling
  into account.

- The nsITreeBoxObject needs some way of controling horizontal scrolling
  and ensuring a position is visible in the tree. At least I need this
  for my application. The options are:
    o Adding methods to nsITreeBoxObject 
    o Adding another interface with additional methods
  The first option is simpler but might break binary compatibility. I 
  noticed that the interface is not frozen. Unless someone has anything
  against it, I'll do that. Here are the methods and properties I'd add:

    readonly attribute long rowWidth;
    readonly attribute long horizontalPosition;
    void ensureCellIsVisible(in long row, in wstring colID);
    void scrollToCell(in long row, in wstring colID);
    void scrollToColumn(in wstring colID);
    void scrollTo(in long row, in long horizontalPosition);

Comment 4 Stef Walter 2003-07-18 12:19:51 PDT
Fixed the slitter issue. Filed an additional bug report as it's a view issue:
bug# 213103
Comment 5 Stef Walter 2003-07-21 17:16:26 PDT
Created attachment 128194 [details] [diff] [review]
Initial implementation

Here's my initial implementation, subject to revision if anyone has comments or
changes. All issues above have been addressed, and horizontal scrolling seems
to work. I'll include a test XUL file which exercises all of the new features. 


There is one remaining issue that I can think of (see below). Because of this
this patch is NOT ready to be checked into the tree.
Comment 6 Stef Walter 2003-07-21 17:20:35 PDT
Created attachment 128196 [details]
Test XUL which exercises new functionality

Note that the splitters between tree columns will not function correctly until
the patches from bug #213103 are applied to your source tree.
Comment 7 Stef Walter 2003-07-21 18:18:59 PDT
Created attachment 128199 [details] [diff] [review]
Initial implementation (corrected)

The original patch was missing a file.
Comment 8 Stef Walter 2003-07-21 18:23:29 PDT
REMAINING ISSUE (please advise):

Because of the way the tree handles horizontal scrolling the treecolpicker is in
an awkward place. Previously it was the last child of the treecols element. It
fit above the vertical scrollbar because of this. However now treecols has moved
into the scrolling view. 

If the columns flex (no horizontal scrollbar) then the treecolpicker currently
takes up a fixed amount of space in the tree on the right as if it were a blank
column (the non-horizontal scrolling tree had this behaviour when there was no
scrollbar).

If the columns are of fixed width, then the treecolpicker currently only appears
 when the view is scrolled to the extreme right. You can see this in the testcase.

Putting the treecolpicker above the vertical scrollbar changes it's nature
slightly. It's formatted as a treecol, and doesn't fit above the scrollbar
properly in most themes. So it'd have to change (the image would need to be
cropped at least) in all the themes would at least slightly. 

As a side note, when the columns don't flex (and horizontal scrolling comes in)
the treecolpicker loses some of it's importance.
Comment 9 Stef Walter 2003-07-31 17:34:01 PDT
Created attachment 128978 [details] [diff] [review]
Implementation with tree column picker

Okay, here's the complete implementation complete with the tree column picker. 


It fits above the scrollbar pretty well though not perfectly. Theme creators
will probably want to make a small change to it eventually. But for now it's
functional and doesn't look too bad.
Comment 10 David Hyatt 2003-08-06 23:42:26 PDT
You'll need to patch the files in the new XUL toolkit as well (the tree XBL and
the xul.css file).  That's under mozilla/toolkit.
Comment 11 David Hyatt 2003-08-06 23:45:42 PDT
I'll try to get you review comments this Saturday.  (I don't think there's a
huge rush, since this won't need to land until after 1.5).  I'm very excited
that you implemented this, and I think it's going to be a great addition to the
toolkit.
Comment 12 Jan Varga [:janv] 2003-08-07 09:46:23 PDT
Nate, could you attach a new patch (for current trunk) in the meantime?
Thanks.
Comment 13 Stef Walter 2003-08-07 15:59:29 PDT
Created attachment 129391 [details] [diff] [review]
Implementation patch updated (sync with trunk, fixes)

Here's a new patch. Although it includes updates to the firebird toolkit, i
haven't had a chance to test that. Changes to the patch:

- Patches tree.xml and xul.css under mozilla/toolkit
- Removed the 'XXX' comment from 
  nsTreeBodyFrame::AdjustEventCoordsToBoxCoordSpace. This is 
  apparently being taken care of by bug #161477
- Removed flawed 'doScroll' logic in nsTreeBodyFrame::ScrollInternal
- Some coding style fixes
- Changed nsTreeBodyFrame patched functions to mirror the 
  changes that were recently made elsewhere in the file
Comment 14 Stef Walter 2003-08-14 16:22:38 PDT
I've found some new issues with empty trees. On an empty tree (without any rows)
the vertical scrollbar shows up when it shouldn't. Obviously need to fix the
'overflow' event issue better. I'll work on this tomorrow.
Comment 15 Stef Walter 2003-08-15 12:23:52 PDT
Created attachment 129871 [details] [diff] [review]
Implementation patch with overflow fixes

Fixed the overflow and scrollbar display issues.
Comment 16 Stef Walter 2003-08-26 19:34:15 PDT
Created attachment 130444 [details] [diff] [review]
Implementation patch updated (sync with trunk, fixes)

- Synced with trunk again
- Added null pointer check
Comment 17 Stef Walter 2003-09-09 11:42:16 PDT
Created attachment 131136 [details] [diff] [review]
Implementation patch updated (sync with trunk, fixes)

Added a small fix to EnsureCellVisible.
Comment 18 Boris Zbarsky [:bz] 2003-10-17 20:36:01 PDT
Comment on attachment 128978 [details] [diff] [review]
Implementation with tree column picker

Removing review request on obsolete patch.

Nate, are you blocked on waiting for reviews?  If so, I can try to roust some
up...
Comment 19 Stef Walter 2003-10-18 23:19:34 PDT
Created attachment 133622 [details] [diff] [review]
Implementation patch updated (sync with trunk)

Another patch update which applies to 1.5. For any testers etc...

Apparently, Jan Varga is rewriting a lot of the tree code and is incorporating
these patches (bug# 221619). So, I'm guessing they wouldn't have to be reviewed
individually.
Comment 20 Jan Varga [:janv] 2003-12-30 00:03:38 PST
I'm terribly sorry for not responding sooner. I've been busy, fortunately the
guys on IRC reminded me of this bug :)

Technically, the patch looks good. David Hyatt and I concluded that you know
what you are doing, in other words it looks professional.

1. I have this patch in my development tree along with many changes I've done
for bug 221619. It works quite well, but one thing I don't like. With this patch
we would use different techniques for the horizontal and vertical scrolling.
Currently we use a standalone vertical scrollbar and we have to handle it
correctly in the tree widget code (either C++ or XBL). The patch uses
nsIScrollableView for horizontal scrolling. I tried to use nsIScrollableView for
vertical scrolling too, but that seems to be a step back in performance.
So we could use a standalone scrollbar for horizontal scrolling but now I can't
recall if it was possible when I tried it before.

2. We could use the new tag "scrollcorner" which has been added recently.

3. I'm not sure if the vertical scrollbar should use all the vertical space
(headers + body) or just space for tree body. The latter seems to look nicer
when the column picker is enabled.

4. I also experimented with a better layout of these elements (columns, body,
scrollbars, scrollconer) by using the grid tag, but IIRC it caused a crash :(

Once again sorry for not letting you know sooner, I just couldn't get to this.
Thanks God, we have X-mas at least :)
Comment 21 Jan Varga [:janv] 2003-12-30 00:14:04 PST
and ideally the tree widget would honour the "overflow" CSS property correctly
e.g. it would be possible to hide vertical scrollbar even if there is more rows
to display
Comment 22 Stef Walter 2004-01-03 15:43:40 PST
Thanks for the comments Jan. 

SCROLLBAR IMPLEMENTATION

Yes, I agree that using two different methods for the horizontal and vertical
scrolling is less than desirable. Initially I tried to use a standalone
scrollbar for the horizontal scrolling similar to how the vertical scrolling is
currently done. However there were problems.

nsIScrollbarMediator (which nsTreeBodyFrame currently uses to get notifications
from the vertical scrollbar) only supports one scrollbar. Of course this
interface cannot be implemented "twice" on nsTreeBodyFrame. Solutions:

- Modify the interface to support two scrollbars. 
  Problem: Changing interfaces isn't cool.
  
- Implement nsIScrollbarMediator on a second proxy
  object which calls back into nsTreeBodyFrame.
  Problem: Kludgy
  
However if people think that either of these two approaches is the way to go,
I'll implement it that way.


SCROLLCORNER

Will look into this.


VERTICAL SCROLLBAR / TREECOL OVERLAP

I initially implemented it like you suggested (ie: with the vertical scrollbar
only as long as the tree body) however this causes layout problems when there is
no tree column picker. 

- With horizontal scrolling...
  In this case the last column shown overhangs the 
  scrollbar and the column header is wider than the 
  actual tree cell shown. 

- Without horizontal scrolling...
  A blank spot is left in the top right corner of the tree.

I thought it was more asthetically pleasing without these issues. As a side note
in almost all toolkits (GTK, QT, Windows) the scrollbar covers the entire
tree/list height including the column headers. 

Again, if these are acceptable tradeoffs then I'll lay it out as you suggested.


OVERFLOW

Good point. However as the current tree doesn't have this behavior WRT the
vertical scrollbar, I would suggest putting this in another bug.
Comment 23 Boris Zbarsky [:bz] 2004-01-03 17:50:25 PST
>  Problem: Changing interfaces isn't cool.

This "interface" is a private (it's not in IDL and not publicly exported)
interface of the layout engine.  So changing it is not so bad.  The only
implementations are tree and listbox, and the only callers are either layout/xul
code or some widget/ files that have special knowledge about trees.
Comment 24 Markus Fischer 2004-01-26 08:17:50 PST
First, I apologize for this probably useless post.

I just wanted to let you people know that I've been gladly waiting for this
feature hitting Mozilla finally. In the past if you would have anything to
display with many columns which cries for a horizont scrollbar, basically you
were screwed with Mozilla (in case you want to use a native widget and not
insert html content inside a XUL document). Imho this wold become one of the
killer feature for Mozilla (for the developers). I'ld also be willing to donate
for this bug as I really would like to see it finally getting in.
Comment 25 Jan Varga [:janv] 2004-01-26 08:40:22 PST
I must confess, it's mostly my fault, but I didn't forget on this bug.
However, I'd like to finish my big tree widget refactoring first.
Comment 26 Jan Varga [:janv] 2004-01-26 12:18:34 PST
> - Modify the interface to support two scrollbars. 
>   Problem: Changing interfaces isn't cool.

I incline to this solution, but I'm still not sure what scrollbar layout would
be the best.
Comment 27 Stef Walter 2004-01-27 10:09:02 PST
Jan, I don't mind reimplementing using the solution agreed upon. Just wondering
if the tree widget refactoring will change things, rendering obselete any work I
do on this right now. LMK.
Comment 28 Jan Varga [:janv] 2004-01-27 10:14:08 PST
It shouldn't be hard to manually merge with my changes.
My changes affect mostly the way how we identify a column.
Comment 29 Stef Walter 2004-02-02 16:12:09 PST
I added another bug for the nsIScrollbarMediator issue: bug# 232913 

Once that's accepted, I'll continue work on the suggested implementation changes.
Comment 30 Stef Walter 2004-03-31 19:13:59 PST
Recently there's been some discussion of where (in code) we should handle the
horizontal scrolling.Here's a diagram of the current state of the horizontal
scrolling patch. 

        --------scrollable view (horizontal)---------^
--------|--------------treecols---------------------|#-------
|  treec|l  |  treecol  |  treecol  |  treecol  |  t|#ecol  |
--------|-------------------------------------------|#-------
--------|-------treechildren (custom painted)-------|#-------
|       |   .           .           .           .   |#      |
|       |   .           .           .           .   |#      |
|       |   .    (only rows scrolled in view    .   |#      |
|       |   .         are painted here)         .   |#      |
|       |   .           .           .           .   |#      |
|       |   .           .           .           .   |#      |
--------|-------------------------------------------|#-------
        ---------------------------------------------v
        <#####(scrollbar from scrollable view)######>

So in this case the vertical scrolling is handled in nsTreeBodyFrame, which
paints <treechildren> element. This is done for efficiency. Only the rows that
are needed are painted. Any other solution here would be a resource hog, and
completely unneccessary. This should not change, IMO.

With the current patch horizontal scrolling is handled by a scrollable view with
attached scrollbar. This is different from the vertical scrolling. It's handled
by the layout view code, and <treechildren> doesn't know anything about it. In
fact <treecols> and <treechildren> are scrolled together. nsTreeBodyFrame paints
all the columns whether they're needed or not. This works fine because the
horizontal scroll domain is usually not that large. (Although in the case of
someone implementing a spreadsheet this would no longer be true, heh heh). 

It was suggested that we handle the horizontal scrollbar internally in
nsTreeBodyFrame as well. After working on it I got the <treechildren> to draw
itself in arbitrary horizontal scroll positions without issue. The problem is
that the <treecols> needs to scroll anyway. <treecols> and <treecol> elements
are XBL all the way. Barring some major rewrites, the columns need a scrollable
view anyway. 

So basically, if we were to handling horizontal scrolling internally we'd be
adding further complexity rather that simplifying things. There are a few things
we could make cleaner or more efficient:

  1. nsTreeBodyFrame could take the horizontal scroll 
     position into account and not draw columns that 
     aren't in the current viewport [easy].
  2. nsIScrollableView could provide a way to hook in 
     a scrollbars (so we can specify the horizontal
     scrollbar in tree.xml similarly to how the vertical
     bar is done) [more difficult].

In all I'm suggesting that although the current solution splits horizontal and
vertical scrolling, it's the most efficient and fits in with XBL the best. Of
course I could be missing something. Comments welcome.

Comment 31 Jan Varga [:janv] 2004-04-01 02:45:53 PST
I've got another crazy idea. Make treecols scrollable and suppress horizontal
scrollbar visibility by CSS. Call nsIScrollBoxObject::ScrollTo() method to keep
treecols in synch with treebody.
This way we could avoid adding <vbox> in XBL that requires nested traversing of
content to find treecols element.
You also need to add CheckHorizontalOverflow() method or join it with the
existing one for vertical overflow. You can check there if the width of treecols
is greater than the width of treebody (mRect.width).
If that doesn't work you could try to use overflow events fired by treecols
element (if they work in this case of course :)

This is just a theory, but I think it's worth to think about it and possibly
implement it.
Comment 32 Stef Walter 2004-04-01 09:53:15 PST
(In reply to comment #31)
> Call nsIScrollBoxObject::ScrollTo() method to keep treecols in 
> synch with treebody.

I'm guessing you mean nsIScrollableView::ScrollTo(). nsIScrollBoxObject only
applies to <xul:scrollbox> elements, and introducing one of those would up the
complexity again. 

> This way we could avoid adding <vbox> in XBL that requires nested 
> traversing of content to find treecols element.

Okay, that could be a factor. Although finding the <treecols> occurs once and
traversing the descendants looking for it isn't a preformance or even a
complexity issue really. OTOH, I may be missing something here. 

The question that begs to be answered however, is if we're going to use a
scrollable view to scroll <treecols>, why not just use it to scroll the treebody
as well? 

I agree that the method in which the scrollbar interacts with the scrollable
view, and various other details could do with some cleaning up, but I think the
idea of scrolling both elements together is sound. I've tried implementing some
of the other solutions and the complexity increases greatly.
Comment 33 Stef Walter 2004-04-21 09:28:06 PDT
If nobody has any additional comments then (shortly) I'm going to start
reworking this patch  as I've outlined it. 
Comment 34 Jan Varga [:janv] 2004-04-21 09:54:49 PDT
> I'm guessing you mean nsIScrollableView::ScrollTo(). nsIScrollBoxObject only
> applies to <xul:scrollbox> elements, and introducing one of those would up the
> complexity again. 

Yeah, you're right my memory failed.

> Okay, that could be a factor. Although finding the <treecols> occurs once and
> traversing the descendants looking for it isn't a preformance or even a
> complexity issue really. OTOH, I may be missing something here. 

Finding the <treecols> occurs every time the column cache is invalidated. That
is, every time a column is added and when the ordinal and primary attribute
changes. See nsTreeColFramce.cpp
But I agree it's not a big deal.
 
> The question that begs to be answered however, is if we're going to use a
> scrollable view to scroll <treecols>, why not just use it to scroll the treebody
> as well? 
> 
> I agree that the method in which the scrollbar interacts with the scrollable
> view, and various other details could do with some cleaning up, but I think the
> idea of scrolling both elements together is sound. I've tried implementing some
> of the other solutions and the complexity increases greatly.
> 

Could you give me an idea of this complexity. I must confess that I have no idea
what arch changes it requires at the moment. It would be nice if you could
summarize key points of this complexity.

I know that there's an implementation and it works, but in my opinion it would
be better to use the same technique as I mentioned earlier.
Comment 35 Stef Walter 2004-08-27 11:09:34 PDT
Hi. I'm working on this bug again. Sorry for the delay but it simply wasn't a
priority to reimplement this of late.

I've started work on the patches that implement Jan's idea. They are more
complex and require many small changes to the tree painting code. I'll post
patches shortly. 
Comment 36 Stef Walter 2004-08-30 11:06:35 PDT
Created attachment 157424 [details] [diff] [review]
Proof of concept patch handling scrolling internal to the tree

This patch is a proof of concept. It's coded along the lines of Jan's
suggestion. Due to a lot of wasted time earlier, I'm not going to complete this
until I hear back on whether this is indeed the way to proceed. 

The following things are missing from the patch:
- Change current scrollbar terminology to 'vertical' scrollbar
- Add support for showing and hiding horizontal scrollbar
  in response to overflow events
- Complete layout issues: little square to the right of the 
  horizontal scrollbar when vert is present, column picker 
  (which may now be obsolete) etc...
- Extend the nsITreeBoxObject interface and complete all
  additional methods.
- Check coding style on submitted code
- Perhaps put overflow:-moz-scrollbars-none; style on treecols
  itself which will require changes in nsTreeColumns::EnsureColumns
- Hook horizontal scrollbar buttons and page inc/dec in properly
- Patch the old mozilla xpfe components

Jan, anyone else, please take a look and comment back. I'll put work on this on
hold until I hear further.
Comment 37 Jan Varga [:janv] 2004-08-30 11:30:40 PDT
Comment on attachment 157424 [details] [diff] [review]
Proof of concept patch handling scrolling internal to the tree

This patch looks very promising. I like it.
I think it doesn't add too much complexity and it's pretty straightforward on
first glance.
Comment 38 Stef Walter 2004-09-01 14:22:22 PDT
It'd be nice to get a resolution on bug 163398 as that affects the layout and
scrolling a lot. 
Comment 39 Jan Varga [:janv] 2004-09-06 01:49:58 PDT
hyatt, could you help us?
Where would you place the column picker?

Thanks
Comment 40 Ben Goodger (use ben at mozilla dot org for email) 2005-02-23 15:31:07 PST
Any updates here? 
Comment 41 Boris Zbarsky [:bz] 2005-02-23 15:43:35 PST
Ben, this bug seems to be blocked on a ui design decision (see comment 39 and
comments leading up to it).  Wanna help?
Comment 42 Ben Goodger (use ben at mozilla dot org for email) 2005-02-23 15:53:13 PST
The column picker would always be visible... it would be above the vertical
scrollbar if present... otherwise space would be allocated to it. Whether that
space would follow all the way down (i.e. space made for a vertical scrollbar
even if there was none) or if the column picker would eat that space from the
last visible column header I have no preference... just as long as it's always
visible. 

i.e.

+------------------------------------------+
| Column 1 | Column 2 | Column 3 | Col...|=|
|----------+----------+----------+-------+-|
| Data 1     Data 2     Data 3     Data... |
| Data 1     Data 2     Data 3     Data... |
| Data 1     Data 2     Data 3     Data... |
| Data 1     Data 2     Data 3     Data... |
| Data 1     Data 2     Data 3     Data... |
|<|[//////////////]                      |>|
+------------------------------------------+

or

+------------------------------------------+
| Column 1 | Column 2 | Column 3 | Col...|=|
|----------+----------+----------+-------+-|
| Data 1     Data 2     Data 3     Da...   |
| Data 1     Data 2     Data 3     Da...   |
| Data 1     Data 2     Data 3     Da...   <- empty vertical space where
| Data 1     Data 2     Data 3     Da...   |  v. scrollbar would go. 
| Data 1     Data 2     Data 3     Da...   |
|<|[//////////////]                    |>| |
+----------------------------------------+-+

Comment 43 Markus Fischer 2005-02-23 22:52:53 PST
(In reply to comment #42)
> +------------------------------------------+
> | Column 1 | Column 2 | Column 3 | Col...|=|
> |----------+----------+----------+-------+-|
> | Data 1     Data 2     Data 3     Data... |
> | Data 1     Data 2     Data 3     Data... |
> | Data 1     Data 2     Data 3     Data... |
> | Data 1     Data 2     Data 3     Data... |
> | Data 1     Data 2     Data 3     Data... |
> |<|[//////////////]                      |>|
> +------------------------------------------+

This would lead to a UI like:

+------------------------------------------+
| Column 1 | Column 2 | Column 3 | Col...|=|
|----------+----------+----------+-------+-|
| Data 1     Data 2     Data 3     Data...^|
| Data 1     Data 2     Data 3     Data...||
| Data 1     Data 2     Data 3     Data...+| <- down arrow
|<|[//////////////]                      |>|
+------------------------------------------+

By comparing a few UI application I couldn't spot one where the down arrow and
right arrows are immidiatly next to it. Usually they leave a small box space in
the corner, e.g.

+------------------------------------------+
| Column 1 | Column 2 | Column 3 | Col...|=|
|----------+----------+----------+-------+-|
| Data 1     Data 2     Data 3     Data...^|
| Data 1     Data 2     Data 3     Data...||
| Data 1     Data 2     Data 3     Data...+| <- down arrow
|<|[//////////////]                     |> |
+------------------------------------------+

Btw, having this bug as a blocker would be great. I've had more then one
scenario in the past where I would need this, but efforts to do it without
native support where too big.
Comment 44 Stef Walter 2005-02-25 17:07:59 PST
Apologies all round. I haven't had time to work on this, and probably won't for
the next few weeks at least. If this bug is blocking stuff then anyone else
willing to take the bug is welcome to. 
Comment 45 Stef Walter 2005-04-20 11:23:35 PDT
Created attachment 181315 [details] [diff] [review]
Patch updated and synced with Trunk
Comment 46 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-04-20 12:03:32 PDT
Nate, is your latest patch ready for review?
Comment 47 Stef Walter 2005-04-20 12:52:46 PDT
No, it's not ready yet. The scrolling basics are there, but there's still a good
deal of polishing and periphery to be done. 
Comment 48 Stef Walter 2005-04-21 12:39:41 PDT
Created attachment 181449 [details] [diff] [review]
Patch: Nearly There

K, here's a working patch, which incorporates Ben's UI suggestions, and follows
Jan's desired methodology. The following still needs to be done:

 - Blt the scroll where possible.

 - XPFE needs copy of the toolkit XUL code.

 - Tree column splitters don't work. 
   This smells like a bug in the view code which I've fixed before. 
   more regression :-(

 - The new nsITreeBoxObject methods needs testing.

 - Regression patch on bug #291198 needs to be reviewed/committed 
   before the scrollbars show and hide themselves properly.
Comment 49 Stef Walter 2005-04-21 12:43:56 PDT
Meh .... and the scroll buttons don't work properly yet. 

FWIW, although the code may look better (and there's a whole lot more of it), it
seems this method is much more complex and inefficient (ie: more redraws, on
slow machines column titles play 'catch up' with the view) than just putting the
whole thing in a scroll view. 

But at this point I really don't care one bit. 
Comment 50 Stef Walter 2005-04-21 15:46:32 PDT
We've solved our column picker issues, no need to wait for bug #163398. That can
be worked out independently at a later date.
Comment 51 Stef Walter 2005-04-21 17:58:44 PDT
Created attachment 181482 [details] [diff] [review]
Without further ado: Horizontal Scrolling for Trees

Step right up. Please take a copy for your testing pleasure. 

 - All previous issues fixed. 
 - Please test and note any bugs or regression.
 - No, I'm not going to rewrite it yet again. I don't have the time.
   If anyone thinks they have an awesome new way to do horizontal 
   scrolling, please take ownership of this bug (or have a concerned 
   friend do so).
Comment 52 Stef Walter 2005-04-21 20:09:07 PDT
Hmmm, flexible columns don't flex anymore. Must look into this.
Comment 53 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-04-22 05:34:37 PDT
Comment on attachment 181482 [details] [diff] [review]
Without further ado: Horizontal Scrolling for Trees

Picking more likely reviewers, if this is to make 1.1a.
Comment 54 Boris Zbarsky [:bz] 2005-04-22 09:14:00 PDT
There's no way I can review this within the next two weeks.  I just don't know
the code well enough and have too much on my plate already.
Comment 55 Stef Walter 2005-04-22 09:32:06 PDT
Created attachment 181539 [details] [diff] [review]
Patch: Fixes flex (ie: normal non-horz-scroll) trees

Now the columns flex properly as expected, and no scrollbar will show if the
(flexed) columns fit into the tree.

BTW, that's how the horizontal scrollbar is used. If the columns fit (which
they usually do when treecol elements have a flex="x" attribute) then no
scrollbar is shown. 

The minute it overflows the scrollbar appears. This can either be the result of
'non-flex' treecol elements, or treecol elements pushed smaller than their
min-width etc.
Comment 56 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-04-22 13:05:37 PDT
I've tried to build the patch in comment 55 with my MingW build, but I'm getting
this build error: http://wargers.org/builderror.txt
Comment 57 Stef Walter 2005-04-22 13:41:32 PDT
Created attachment 181560 [details] [diff] [review]
Patch: Now with proper variable scoping

My bad. I've attached a new patch.
Comment 58 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-04-23 01:47:08 PDT
Ok, was able to build now. Looks very nice!
But I get 8 of these assertions before my Firefox build has started up.
###!!! ASSERTION: no scroll bar: 'mScrollbar && mHorzScrollbar && mColScrollView
', file c:/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, lin
e 790
I get these kind of assertions also a lot with the options window.
Comment 59 Stef Walter 2005-04-23 08:36:49 PDT
Those are the result of the nsTreeBodyFrame code not finding the scrollbars
and/or scrollable column view. 

Did you build everything, or just layout directory?
Are you building mozilla or firefox? 
Did tree.xml and xul.css patch?
Comment 60 Stef Walter 2005-04-23 11:54:25 PDT
Found the problem. *Everybody* seems to reimplement the <tree> XBL binding. 
This means either we patch everywhere and add the horizontal scrollbar 
thingyness, or add code to nsTreeBodyFrame.cpp to get along without the 
presence of a (normally hidden) horizontal scrollbar.
Comment 61 Stef Walter 2005-04-23 13:01:42 PDT
Created attachment 181655 [details] [diff] [review]
Patch: Now for all you other trees out there, a little something special.

Changes:

- Works with all the other derived XBL <tree> implementations.
- If no horzizontal scrollbar or scrollable columns found, horizontal scrolling
is disabled internally.
- Keep the CSS class for the vertical scrollbar the same.
Comment 62 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-04-23 14:45:29 PDT
Yes, with that last patch all works fine now, no assertions.
I see a small problem. Sometimes, when resizing (gradually making wider or
smaller) a window with a tree control (dom inspector), I get a horizontal
scrollbar, which should not appear.
Comment 63 Stef Walter 2005-04-23 15:46:30 PDT
Created attachment 181668 [details] [diff] [review]
Patch: Rounding Errors R Us

Thanks for pointing that out Martijn. Turns out that the tree does in fact
overflow horizontally (and shows the scrollbar etc.), but by only one twip (or
whatever the singular of twips is). Probably the result of a rounding error
(ie: pixel -> twips conversion) in the box layout code.

So I added in a little code that suppresses overflows which are less that half
a pixel in size. Not terribly elegant, but gets the job done.
Comment 64 Boris Zbarsky [:bz] 2005-04-24 12:41:40 PDT
> *Everybody* seems to reimplement the <tree> XBL binding. 

Do you mean the forks in xpfe & toolkit & mail?  If so, the right answer is to
patch them all, curse people for forking a bit more, and hope that the unforking
happens soon...

Or do you mean something else?
Comment 65 Stef Walter 2005-04-24 15:52:38 PDT
Well the xpfe duplication is already covered by this patch. This is what I'm
talking about:

http://lxr.mozilla.org/mozilla/search?string=treerows%3E

Many implementations of the actual XBL bindings, in autocomplete.xml, select.xml
and elsewhere. But no worries, the latest version of the patch simply disables
horizontal scrolling if the XBL doesn't have the necessary scrollbars and other
periphery. 

Comment 66 Boris Zbarsky [:bz] 2005-04-24 16:05:25 PDT
Oh, I see.  People extending tree.  Gotcha!
Comment 67 Stef Walter 2005-04-28 15:19:13 PDT
Benjamin, know of any other likely reviewers? 
Comment 68 Jan Varga [:janv] 2005-04-29 00:12:21 PDT
The patch looks good. I didn't pay attention to any code style issues though.
Comment 69 Stef Walter 2005-04-29 01:12:22 PDT
I've tried to match the style of the surrounding code, as there are several
styles in that file. But I can go over it again and double check things.
Comment 70 Stef Walter 2005-04-29 09:57:15 PDT
Just to confirm, code style in the patch matches the surrounding code. 
Comment 71 Jan Varga [:janv] 2005-04-30 04:55:07 PDT
Sorry, I didn't want to say that your code style was bad. Your patch looks good,
other (super)reviewer might find some nits, that's all.
Comment 72 Jan Varga [:janv] 2005-06-10 08:19:41 PDT
r=varga
Comment 73 Boris Zbarsky [:bz] 2005-06-10 08:25:23 PDT
Comment on attachment 181668 [details] [diff] [review]
Patch: Rounding Errors R Us

Marking jan's r=.  roc, want to sr?
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-06-19 21:46:41 PDT
Comment on attachment 181668 [details] [diff] [review]
Patch: Rounding Errors R Us

+    if (aNewIndex > aOldIndex)
+      ScrollHorzInternal(mHorzPosition + t2p);
+    else 
+      ScrollHorzInternal(mHorzPosition - t2p);

We scroll left/right by one pixel every time the scrollbar button is pressed?
That seems wrong.

+PRBool 
+nsTreeBodyFrame::OffsetHorzScroll(nsRect& rect, PRBool clip)
I wish this had a better name.

+  if (clip && rect.x < mInnerBox.x) {
+    rect.width -= mInnerBox.x - rect.x;
+    rect.x = mInnerBox.x;

Why don't we clip if the rectangle goes off the right side of mInnerBox?

+  nscoord mHorzWidth;

Can you put a comment here?

Other than that it looks fine.
Comment 75 Boris Zbarsky [:bz] 2005-07-01 06:47:43 PDT
If we want this for aviary 1.1, we should try to get it into the 1.8 cycle asap,
I think...
Comment 76 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-01 13:45:45 PDT
This isn't going to make 1.8 now, we'll put this in early 1.9.
Comment 77 Lukas Ramach 2005-07-04 22:24:59 PDT
(In reply to comment #76)
> This isn't going to make 1.8 now, we'll put this in early 1.9.

Thats really bad news for all peaople that think of firefox/thunderbird as
superior in usability and in my opinion it is a decision in the wrong direction.
It took two years to get a patch up and running for a BASIC usabilty feature and
then it gets delayed for another half a year. I'm building an application on top
of the gecko engine and all major usabilty bugs, i get confronted with, can be
traced down to this bug.

Please reconsider your decision.
Comment 78 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-05 08:13:03 PDT
I'm sorry. I wanted this to make 1.8 just as much as anyone, but it's a fact
that this patch is likely to cause regressions, both crashes and UI regressions,
and we do not have the time or developer resources to catch and fix these
regressions in the remaining timeframe for 1.8.
Comment 79 Boris Zbarsky [:bz] 2005-08-30 11:33:30 PDT
Created attachment 194340 [details] [diff] [review]
Last patch updated to tip

This is just the "Rounding Errors R Us" patch merged to tip (mostly to changes
from bug 303779).
Comment 80 Boris Zbarsky [:bz] 2005-08-30 11:56:43 PDT
Created attachment 194344 [details] [diff] [review]
Additional patch that was needed to fix bustage
Comment 81 Boris Zbarsky [:bz] 2005-08-30 12:23:25 PDT
Created attachment 194348 [details] [diff] [review]
Another additional patch, similar reasons.
Comment 82 Boris Zbarsky [:bz] 2005-08-30 13:45:46 PDT
Checked in on trunk.  My apologies that this took so long after 1.9 opened...

Nate, thank you very much for doing this and for dealing with the glacial
slowness at which reviews have a tendency to happen.  :(

Marking fixed.  Any remaining problems should go in new bugs; regressions should
go in new bugs blocking this one.
Comment 83 Stef Walter 2005-08-30 15:25:22 PDT
That's really wonderful. Thanks. 

I noticed that roc's comments (see comment 74) haven't been addressed though.
The first one is probably important. My initial suggestion would be just to
change the code to something like: 

+    if (aNewIndex > aOldIndex)
+      ScrollHorzInternal(mHorzPosition + (t2p * 16));
+    else 
+      ScrollHorzInternal(mHorzPosition - (t2p * 16));


I had meant to get around to this, but mozilla takes a whole lot of time to get
back into, and work has been hectic with other stuff. Sorry bout that. 
Comment 84 Boris Zbarsky [:bz] 2005-08-30 21:36:54 PDT
Created attachment 194412 [details] [diff] [review]
Fix issues from comment 74.

I landed this with r+sr=roc.  Went with 10px for the scroll because that's what
scrollframes do.
Comment 85 neil@parkwaycc.co.uk 2005-08-31 05:39:30 PDT
This fix is still wrong (as is the CSS in xul.css, but you knew that).
Comment 86 neil@parkwaycc.co.uk 2005-08-31 06:23:06 PDT
Created attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

In this case, everything is much easier if the scrollbar is calibrated in
twips.
Comment 87 Boris Zbarsky [:bz] 2005-08-31 08:23:19 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

roc, could you please look at this?  I still don't know this code, and I'm
trying to get to other reviews in a reasonable timeframe; I'd not be able to do
this for a week or two... :(
Comment 88 Boris Zbarsky [:bz] 2005-08-31 08:24:30 PDT
Neil, what's wrong with xul.css here?
Comment 89 neil@parkwaycc.co.uk 2005-08-31 08:54:37 PDT
(In reply to comment #88)
>Neil, what's wrong with xul.css here?
Mainly, the use of descendent selectors and also the existing XXX'd hardcoded
styles (the comment only mentions the width but the background colour is also
incorrect); also the indentation is incorrect.

Correct use of xbl:inherits can remove nearly all of the visibility rules.
Comment 90 Boris Zbarsky [:bz] 2005-08-31 10:51:45 PDT
> Mainly, the use of descendent selectors 

Doh.  Can we replace those with child selectors?

> Correct use of xbl:inherits can remove nearly all of the visibility rules.

Any help here is much appreciated... I just wanted to get this finally checked
in after the way we've dragged our feet on it; I don't really know this code or
the tree code in general...
Comment 91 Boris Zbarsky [:bz] 2005-09-05 19:00:45 PDT
This caused bug 306575, bug 306971, bug 306990.
Comment 92 Stef Walter 2005-09-07 15:36:52 PDT
Most likely I'll be able to work on these bugs in about a week. If anyone wants
to have a go before then, by all means feel free. 
Comment 93 Neil Deakin 2005-09-08 16:05:45 PDT
You used the wrong attribute names here. show-vscroll should be showvscroll and
show-hscroll should be showhscroll. (or showverticalscrollbar or something).
Comment 94 Boris Zbarsky [:bz] 2005-09-08 16:13:00 PDT
Neil, that's in which diff?  Could you file a followup bug on it, please?
Comment 95 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-13 22:09:58 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

But scrollbar attributes aren't in twips elsewhere. Even if you can get away
with it here, the inconsistency would be bad.
Comment 96 Boris Zbarsky [:bz] 2005-09-13 22:52:57 PDT
roc, is that a r+sr= as in "land that"?  That doesn't match your comment, does it?
Comment 97 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-14 02:54:55 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

"oops"
Comment 98 neil@parkwaycc.co.uk 2005-09-14 07:08:45 PDT
(In reply to comment #95)
>But scrollbar attributes aren't in twips elsewhere. Even if you can get away
>with it here, the inconsistency would be bad.
Actually they are for historic reasons (the thumb was sized based on the height
of the scrollbar in twips instead of the page increment attribute). But thanks
to my fix for bug 236545 you can now use any convenient unit scheme. And it
turns out that for horizontal scrollbars that twips is most convenient.
Comment 99 neil@parkwaycc.co.uk 2005-09-14 07:09:58 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

Rerequesting r+sr based on comment 98.
Comment 100 Boris Zbarsky [:bz] 2005-09-14 08:26:33 PDT
So could we please, please get separate bugs filed on the followup issues?  Esp.
on comment 93 (which I don't understand well enough to file a bug on)?
Comment 101 Neil Deakin 2005-09-14 08:36:14 PDT
Bug 308490 covers additional issues, including my comment.
Comment 102 Boris Zbarsky [:bz] 2005-09-14 08:51:19 PDT
Ah, excellent.
Comment 103 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-14 15:11:51 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

I mean that scrollbars used elsewhere in Gecko have those attributes in pixels.
Comment 104 neil@parkwaycc.co.uk 2005-09-14 16:48:45 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsGfxScrollF
rame.cpp&rev=3.229&mark=2356,2398#2398
Comment 105 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-14 20:14:27 PDT
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

huh. sorry. I don't know what I was thinking about.
Comment 106 Nickolay_Ponomarev 2005-12-09 07:11:51 PST
*** Bug 319448 has been marked as a duplicate of this bug. ***
Comment 107 Jason Barnabe (np) 2006-05-30 20:35:40 PDT
Is there any documentation for this feature? It's difficult to glean out of 100+ comments what exactly this can do/how to do it.
Comment 108 charles stephens 2006-10-27 06:07:13 PDT
Created attachment 243782 [details]
Test showing clipped text beyond original viewable area of tree

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1

I have been looking at horizontal scrolling for use in a new application, the text in columns beyond the original window/tree width appears to be clipped. Text only appears in original viewable area.
Comment 109 Stef Walter 2006-10-27 08:26:30 PDT
Various pieces of this code have been removed by people to fix other minor bugs, until the feature itself has almost completely regressed in recent mozilla builds. I've given up working on this though (and on mozilla in general). 

There's a bug around somewhere where someone (sorry don't remember exactly where) is trying to fix this feature back...

Comment 110 Boris Zbarsky [:bz] 2006-10-27 09:42:21 PDT
> Various pieces of this code have been removed by people to fix other minor
> bugs,

Er... do you happen to have the bug numbers?  This sort of thing should not be happening, and if people have been reviewing and landing such patches without committing to fixing the resulting regressions then we have a major problem.  If nothing else, we need to add this to whatever regression tests we run...

> I've given up working on this though (and on mozilla in general). 

:(

I can't find the bug you mention, so I filed bug 358398 on the problem Charles points out in comment 108.  I'll see if I can figure out what regressed it.
Comment 111 Stef Walter 2006-10-27 10:18:58 PDT
Hmmm, looking through my sent mail, I found this. I remember there being other regressions, but I believe this to be the main one:

Date: Mon, 31 Jul 2006 21:51:41 
From: Nate Nielsen <nielsen@memberwebs.com>
To: jin gan <javajin@hotmail.com>
CC:  mark@standard8.demon.co.uk
Subject: Re: Question about nsTreeBodyFrame.cpp

jin gan wrote:
> Thanks anyway.
>   I debuged the xulrunner, and found out the problem of CalcColumnRect in 
> nsTreeBodyFrame.cpp. The mInnerBox.width is always the width of the tree, and 
> the mInnerBox.x=0 even if the tree is HScrolled. so , the returned RECT.width of 
> cell is ZERO if CELL.RECT.x>mInnerBox.width. This results in PaintCell not 
> called in PaintRow even though the cell is visible in ScrollView.
>   I will try to fire a BUG on bugzilla, and ask help on mozilla.dev.tech.layout.

Look at the code removed from CalcColumnRect, which I believe broke the
scrolling:

Bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=306990

Patch:
https://bugzilla.mozilla.org/attachment.cgi?id=210975

Cheers,
Nate
Comment 112 Boris Zbarsky [:bz] 2006-10-27 10:25:28 PDT
Ah, heh.  That's exactly what I came out with as the cause of bug 358398.  ;)
Comment 113 Mime Čuvalo 2007-02-23 11:23:11 PST
I apologize if this is a dumb question but it's similar to comment 107 which went unanswered: how exactly do you enable this feature?  I've been testing on Firefox 3.0a1 trying to get FireFTP to have some horizontal scrollbars in the trees but to no avail.  Is it something in CSS, an attribute or is it supposed to be automatic?
Comment 114 neil@parkwaycc.co.uk 2007-02-23 13:26:41 PST
(In reply to comment #113)
>Is it something in CSS, an attribute or is it supposed to be automatic?
As I recall, with normal trees you set flexes on your treecol elements.
If instead you set widths on your treecol elements then they will scroll.
Comment 115 Mime Čuvalo 2007-02-23 20:13:02 PST
Created attachment 256249 [details]
Screenshot of horizontal scrolling in Explorer

Ah, I see.  I got it to work now.  It's a pretty nice feature - however, I guess I was expecting something different.  I think a lot of users have more of an issue with the tree columns that have primary="true" and not being able to see the hierarchy if you go too deep into the tree (e.g. FireFTP or DOM Inspector or the Bookmarks Manager).  See attached screenshot of what I'm talking about - the Folder pane in explorer has horizontal scrolling that would be great to have.  I know primary type columns aren't necessarily alone (as in DOM Inspector) so it would look like crap to add scrolling to an individual primary column.  But would it be feasible maybe to perhaps "shift" the contents of a column to the left as you went deeper into the tree?  Or maybe have a special type of horizontal scrolling tree to accommodate trees that just have a single primary column?  I suppose that's asking a bit much - but I thought I'd throw the idea out there.
Comment 116 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-24 15:29:35 PST
Mime, could you file new bugs on your suggestions in comment 15?
This bug is fixed for this particular problem. For the issues you see/suggestions you have, it is common to file new bugs on that.
Also, if you have figured out how horizontal scrolling for xul tree controls works, you could write an article for that in http://developer.mozilla.org/ , explaining it. That would be much appreciated.
Comment 117 Gomita 2007-02-24 19:32:49 PST
How can I add horizontal scrollbar in Bookmarks Manager?
Comment 118 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-25 00:49:51 PST
(In reply to comment #117)
> How can I add horizontal scrollbar in Bookmarks Manager?

I filed bug 371592 for this.
Comment 119 Gomita 2007-02-26 08:10:34 PST
> I filed bug 371592 for this.

Thanks.
Comment 120 Mime Čuvalo 2007-02-28 18:08:42 PST
I filed bug 372206 as suggested by Martijn for my comment 115.
Comment 121 Eric Shepherd [:sheppy] 2007-08-22 15:27:04 PDT
Can someone point me to the final syntax for the XUL for the horizontal scrolling support, so I can make sure it's documented properly?  There are so many discussed ideas here that I'm not clear on what you decided on.
Comment 122 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-22 15:47:49 PDT
See comment 114.
With some limited testing, I noticed that setting a flex attribute on just 1 treecol makes the scrolling feature disappear.
Setting no widths or flexes, then you still have the scrolling feature.
Comment 123 neil@parkwaycc.co.uk 2007-08-22 15:55:02 PDT
(In reply to comment #122)
>Setting no widths or flexes, then you still have the scrolling feature.
Setting no flexes is important. Setting widths is useful but not necessary.
Comment 124 Eric Shepherd [:sheppy] 2007-08-23 09:08:50 PDT
Is there an example I can look at?  I'm still unclear on how this works.
Comment 125 Maros Ivanco 2008-07-08 06:47:48 PDT
For those who are unable to find out how this works:

<tree style="width:200px;">
  <treecols>
    <treecol primary="true" style="width:250px;overflow:-moz-scrollbars-horizontal;"/>
    <treecol style="width:150px;overflow:-moz-scrollbars-horizontal;"/>
  </treecols>
  <treechildren/>
</tree>

Important: No flex and tree width < sum(treecol width).

Most probably it is not what everybody has hoped for, see #115.
Comment 126 neil@parkwaycc.co.uk 2008-07-08 07:02:43 PDT
(In reply to comment #125)
>     <treecol primary="true" style="width:250px;overflow:-moz-scrollbars-horizontal;"/>
>     <treecol style="width:150px;overflow:-moz-scrollbars-horizontal;"/>
You shouldn't need that overflow style.

Note You need to log in before you can comment on or make changes to this bug.