Closed Bug 212789 Opened 21 years ago Closed 19 years ago

Horizontal Scrolling for XUL Tree Controls [Will Implement]

Categories

(Core :: XUL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: stef, Assigned: stef)

References

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 16 obsolete files)

5.40 KB, application/vnd.mozilla.xul+xml
Details
48.31 KB, patch
Details | Diff | Splinter Review
1.87 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
9.47 KB, patch
Details | Diff | Splinter Review
3.19 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.70 KB, application/vnd.mozilla.xul+xml
Details
55.35 KB, image/jpeg
Details
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:
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; }
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!
Assignee: varga → nielsen
Status: UNCONFIRMED → NEW
Ever confirmed: true
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);

Depends on: 213103
Fixed the slitter issue. Filed an additional bug report as it's a view issue:
bug# 213103
Attached patch Initial implementation (obsolete) — Splinter Review
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.
Note that the splitters between tree columns will not function correctly until
the patches from bug #213103 are applied to your source tree.
The original patch was missing a file.
Attachment #128194 - Attachment is obsolete: true
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.
Blocks: 112832
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.
Attachment #128199 - Attachment is obsolete: true
Attachment #128978 - Flags: review?(hyatt)
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.
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.
Nate, could you attach a new patch (for current trunk) in the meantime?
Thanks.
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
Attachment #128978 - Attachment is obsolete: true
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.
Fixed the overflow and scrollbar display issues.
Attachment #129391 - Attachment is obsolete: true
- Synced with trunk again
- Added null pointer check
Attachment #129871 - Attachment is obsolete: true
Added a small fix to EnsureCellVisible.
Attachment #130444 - Attachment is obsolete: true
Depends on: 221619
Blocks: 221619
No longer depends on: 221619
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...
Attachment #128978 - Flags: review?(hyatt)
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.
Attachment #131136 - Attachment is obsolete: true
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 :)
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
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.
>  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.
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.
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.
> - 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.
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.
Status: NEW → ASSIGNED
It shouldn't be hard to manually merge with my changes.
My changes affect mostly the way how we identify a column.
I added another bug for the nsIScrollbarMediator issue: bug# 232913 

Once that's accepted, I'll continue work on the suggested implementation changes.
Depends on: 232913
No longer blocks: 221619
Depends on: 163398, 221619
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.

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.
(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.
If nobody has any additional comments then (shortly) I'm going to start
reworking this patch  as I've outlined it. 
> 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.
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. 
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 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.
It'd be nice to get a resolution on bug 163398 as that affects the layout and
scrolling a lot. 
hyatt, could you help us?
Where would you place the column picker?

Thanks
Flags: blocking-aviary1.1?
Ben, this bug seems to be blocked on a ui design decision (see comment 39 and
comments leading up to it).  Wanna help?
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...   |
|<|[//////////////]                    |>| |
+----------------------------------------+-+

(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.
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. 
Attachment #157424 - Attachment is obsolete: true
Nate, is your latest patch ready for review?
Depends on: 291198
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. 
Attached patch Patch: Nearly There (obsolete) — Splinter Review
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.
Attachment #181315 - Attachment is obsolete: true
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. 
Flags: blocking1.8b2?
We've solved our column picker issues, no need to wait for bug #163398. That can
be worked out independently at a later date.
No longer depends on: 163398
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).
Attachment #181449 - Attachment is obsolete: true
Attachment #181482 - Flags: superreview?(hyatt)
Attachment #181482 - Flags: review?(jan)
Hmmm, flexible columns don't flex anymore. Must look into this.
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.
Attachment #181482 - Flags: superreview?(hyatt)
Attachment #181482 - Flags: superreview?(dbaron)
Attachment #181482 - Flags: review?(bzbarsky)
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.
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.
Attachment #181482 - Attachment is obsolete: true
Attachment #181539 - Flags: superreview?(dbaron)
Attachment #181539 - Flags: review?(jan)
Attachment #181482 - Flags: superreview?(dbaron)
Attachment #181482 - Flags: review?(jan)
Attachment #181482 - Flags: review?(bzbarsky)
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
My bad. I've attached a new patch.
Attachment #181539 - Attachment is obsolete: true
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.
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?
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.
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.
Attachment #181560 - Attachment is obsolete: true
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.
Attached patch Patch: Rounding Errors R Us (obsolete) — Splinter Review
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.
Attachment #181655 - Attachment is obsolete: true
> *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?
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. 

Attachment #181539 - Flags: superreview?(dbaron)
Attachment #181539 - Flags: review?(jan)
Attachment #181668 - Flags: superreview?(dbaron)
Attachment #181668 - Flags: review?(jan)
Oh, I see.  People extending tree.  Gotcha!
Benjamin, know of any other likely reviewers? 
The patch looks good. I didn't pay attention to any code style issues though.
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.
Just to confirm, code style in the patch matches the surrounding code. 
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.
Flags: blocking1.8b2? → blocking1.8b2-
r=varga
Comment on attachment 181668 [details] [diff] [review]
Patch: Rounding Errors R Us

Marking jan's r=.  roc, want to sr?
Attachment #181668 - Flags: superreview?(roc)
Attachment #181668 - Flags: superreview?(dbaron)
Attachment #181668 - Flags: review?(jan)
Attachment #181668 - Flags: review+
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.
Attachment #181668 - Flags: superreview?(roc) → superreview+
If we want this for aviary 1.1, we should try to get it into the 1.8 cycle asap,
I think...
Flags: blocking1.8b3?
This isn't going to make 1.8 now, we'll put this in early 1.9.
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1-
(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.
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.
This is just the "Rounding Errors R Us" patch merged to tip (mostly to changes
from bug 303779).
Attachment #133622 - Attachment is obsolete: true
Attachment #181668 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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. 
I landed this with r+sr=roc.  Went with 10px for the scroll because that's what
scrollframes do.
This fix is still wrong (as is the CSS in xul.css, but you knew that).
In this case, everything is much easier if the scrollbar is calibrated in
twips.
Attachment #194430 - Flags: superreview?(bzbarsky)
Attachment #194430 - Flags: review?(bzbarsky)
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... :(
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview?(bzbarsky)
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review?(bzbarsky)
Neil, what's wrong with xul.css here?
(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.
> 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...
Depends on: 306971
Depends on: 306990
Depends on: 306575
Depends on: 307062
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. 
You used the wrong attribute names here. show-vscroll should be showvscroll and
show-hscroll should be showhscroll. (or showverticalscrollbar or something).
Neil, that's in which diff?  Could you file a followup bug on it, please?
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.
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview+
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review+
roc, is that a r+sr= as in "land that"?  That doesn't match your comment, does it?
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

"oops"
Attachment #194430 - Flags: superreview-
Attachment #194430 - Flags: superreview+
Attachment #194430 - Flags: review-
Attachment #194430 - Flags: review+
(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 on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

Rerequesting r+sr based on comment 98.
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview-
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review-
Depends on: 308490
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)?
Bug 308490 covers additional issues, including my comment.
Ah, excellent.
Depends on: 308520
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.
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview-
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review-
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
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview-
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review-
Comment on attachment 194430 [details] [diff] [review]
Fix scrolling properly (checked in)

huh. sorry. I don't know what I was thinking about.
Attachment #194430 - Flags: superreview?(roc)
Attachment #194430 - Flags: superreview+
Attachment #194430 - Flags: review?(roc)
Attachment #194430 - Flags: review+
Attachment #194430 - Attachment description: Fix scrolling properly → Fix scrolling properly (checked in)
Depends on: 310668
Depends on: 310710
*** Bug 319448 has been marked as a duplicate of this bug. ***
Blocks: 328948
Depends on: 330236
Blocks: 330170
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.
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.
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...

> 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.
Flags: in-testsuite?
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
Ah, heh.  That's exactly what I came out with as the cause of bug 358398.  ;)
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?
(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.
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.
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.
How can I add horizontal scrollbar in Bookmarks Manager?
Blocks: 371592
(In reply to comment #117)
> How can I add horizontal scrollbar in Bookmarks Manager?

I filed bug 371592 for this.
> I filed bug 371592 for this.

Thanks.
I filed bug 372206 as suggested by Martijn for my comment 115.
Blocks: 348764
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.
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.
(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.
Is there an example I can look at?  I'm still unclear on how this works.
Blocks: 399961
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.
(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.
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.