If you open the preference window or the Help Viewer in SeaMonkey, you'll notice that the items in the preference tree and the Help contents tree are cropped at the right. There used to be some space at the right and items that didn't fit would end with "...". This is not the case anymore. I see this on mac, but others have confirmed it on linux as well. I suppose the larger default font makes it more visible on mac. First seen: SeaMonkey 2005083110 (Mac OS 10.3.9) Does not occcur in: SeaMonkey 20050830009. Bonsai link: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-30+09%3A00%3A00&maxdate=2005-08-31+10%3A00%3A00&cvsroot=%2Fcvsroot My guess is that this is caused by bug 212789, but I leave it up to others who knows more to set dependencies.

Note that this makes the Help contents pane looks quite bad on mac (last two images) since it exposes a mac widget(?) issue.

Tested a bit more and it seems connected to the vertical scrollbar, the cropping and the absence of "..." only occurs when there's a vertical scrollbar. It's like the scrollbar is positioned over parts of the tree's rightmost content area.

Yeah, perhaps we're not adjusting the content area for the scrollbar?

Hmmm, this is not good. It's a little more complicated. It occurs when there's no treecolpicker in the treecols element (whether the treecols is visible or not). This a side effect of the modified layout layout suggested by beng in #212789. The column (again whether visible or not) expands to fill the full width (above scrollbar) of the area allocated for the tree. The cells in the view follow the size of the column, and the ellipses are written 'under' the scrollbar. LMK if that doesn't make sense, and we can have an ASCII art fest. Take a look at the 'bookmarks' window. It doesn't have this problem. This needs some thought to cover all eventualities. Any ideas?

This should fix the problem, although it's not particularly pretty.

Since I failed to understand it, does the patch make the last column narrower in the case where there is a vertical scrollbar but no column picker, without affecting the rendering in other cases?

Yes, that's right. In addition there's a semi-related update to CalcHorzWidth() which fixes calculating the width of the scrollable view for certain corner cases.

Comment on attachment 198361 [details] [diff] [review] Patch for to fix I don't think that the "hocus pocus" comment is necessary. We just need to fix this case and the patch looks quite precise. Anyway there are some nits. + if (aCol->GetNext() != NULL) + return; just use if (aCol->GetNext()) I've also noticed that mColScrollView could be replaced by a getter

Thanks. Made suggested changes. About the mColScrollView. Yes it could be a getter, but I guess that would be another patch for another day.

+ if (diff > 0) + rect.width -= diff; This could make the width negative, correct? Don't you want to PR_MAX(0, ...) here or similar? Otherwise looks fine.

Boris, do you have time to check this in? I don't have commit access.

Fixed on trunk. Thanks, Nate!

Unfortunately, this is not fixed on mac. I still see the issue with a seamonkey nightly from yesterday (2005101209). Sorry for not reporting earlier, but I've been on vacation.

Nate has no mac. Nate is a sad boy. Anyone else with the necessary equipment want to take this?

Josh, Simon, can you maybe take a look?

To reproduce this in a Firefox nightly, just open Help and expand some of the Contents items until a vertical scrollbar appear.

Hmm... by looking at the screenshots in bug 324869, I get the impression that it's not just mac that still suffers from this issue.

This patch fixes the bug in Firefox Help, and also fixes it in the URL autocomplete dropdown.

Comment on attachment 210975 [details] [diff] [review] fix looks good to me

Hmmm, part of me wants to ask: What about the corner case checks being removed. For example when the view bounds width zero (ie: no columns). But then I haven't worked with this code for a while now, so I'm assuming you've checked around for the effects, especially with widgets that aren't really full blown trees. I just remember without some of those checks scrollbars would appear or not appear in some strange places.

(In reply to comment #21) > Hmmm, part of me wants to ask: What about the corner case checks being > removed. For example when the view bounds width zero (ie: no columns). All the cases that this patch fixes have no column headers. The innerBox.width is guaranteed to give us the true right edge of the content area, excluding the vertical scrollbar, so we definitely don't want any cell to go beyond it ... and it's OK for the last cell to go up to that limit. > But then I haven't worked with this code for a while now, so I'm assuming > you've checked around for the effects, especially with widgets that aren't > really full blown trees. I just remember without some of those checks > scrollbars would appear or not appear in some strange places. Yeah, there are lots of cases and I don't understand the code very well (nor does anyone else, I suggest), but this approach seems logical to me and we need to make forward progress. Anything suggestions on what to test would be appreciated.

Comment on attachment 210975 [details] [diff] [review] fix sr=dbaron. If a patch with this high a -/+ line ratio fixes bugs and has module owner review, it has to be good.

checked in.

*** Bug 310710 has been marked as a duplicate of this bug. ***

The fix in CalcColumnRect goes wrong when the mInnerBox.width<rect.x and there is a HScroll. This results in PaintCell not called in PaintRow even if rect.x-mHorzPosition<mInnerBox.width since the returned rect got a zero width. Shall we change the code in CalcColumnRect like rect.width = PR_MAX(0, PR_MIN(rect.width, mInnerBox.width - rect.x + mHorzPosition)); The BUG accurs when tree.width<total width of columns. CELLS NOT PAINTED after hscroll. Here is the test case: <vbox flex="1"> <tree id="my-tree" width="300" > <treecols class=""> <treecol class="" width="200" id="datecol1" label="date1" type="text"/> <treecol class="" width="200" id="datecol2" label="Date2" type="text"/> <treecol class="" width="200" id="datecol3" label="Date3" type="text"/> <treecol class="" width="200" id="datecol4" label="Date4" type="text"/> </treecols> <treechildren class="gridlines" > <treeitem> <treerow> <treecell label="date1"/> <treecell label="date2"/> <treecell label="date3"/> <treecell label="date4"/> </treerow> </treeitem> </treechildren> </tree>

Thanks for spotting that. However I personally think that your correction has an unwanted side-effect. Consider the case of a cell with a long label: Scrolled to the right: || This is supposed to be a very long header label | || This is supposed to be a very long cell label | Scrolled to the left: | This header label is short | This is supposed to | | This cell label is short | This is supposed ...| I would not expect the cell label ellipsis. I tried changing the formula to rect.width = PR_MAX(0, PR_MIN(rect.width, mHorzWidth - rect.x)); but then it sometimes paints outside of the tree.

In fact there's a clipping issue with painting partially scrolled column background/borders. You can demonstrate that in the test case from comment #26 by changing the id of the second column from "datecol2" to "insertafter". Perhaps it's worth filing a new bug on these issues?

Thanks for comment, Neil; I tried to change the id of column 2; an unexpected vertical line appears. But I don't know when will we change the id like "insertafter"? By the way, GetCoordsForCellItem in nsTreeBodyFrame.cpp goes wrong too, when mHorzPosition>0. ============================================================= for (nsTreeColumn* currCol = mColumns->GetFirstColumn(); currCol && currX < mInnerBox.x + mInnerBox.width; currCol = currCol->GetNext()) { ============================================================= currX < mInnerBox.x + mInnerBox.width results in empty rect returned in this function for cells with rect.x>mInnerBox.x + mInnerBox.width. Why not compute rect for all columns? HScroll faild, inline edit(https://bugzilla.mozilla.org/show_bug.cgi?id=201499) failed :( I'm looking forward to a patch on inline edit support with h-scroll-able tree.

(In reply to comment #29) >I tried to change the id of column 2; an unexpected vertical line appears. The line itself is expected, but it's supposed to paint between columns 2 and 3. >But I don't know when will we change the id like "insertafter"? Don't worry, that was just a test, insertafter is normally used by drag'n'drop. >By the way, GetCoordsForCellItem in nsTreeBodyFrame.cpp goes wrong too, when >mHorzPosition>0. >inline edit(https://bugzilla.mozilla.org/show_bug.cgi?id=201499) failed :( I really think you should file new bug(s?) on these horizontal scrolling errors, and CC me, roc@ocallahan.org, jan.varga@gmail.com and enndeakin@sympatico.ca

This change broke text display in horizontally scrolling trees (because now all cells to the right of the originally visible area are clamped to 0 width, as far as I can tell). See bug 358398.

(In reply to comment #31) >This change broke text display in horizontally scrolling trees (because now all >cells to the right of the originally visible area are clamped to 0 width, as >far as I can tell). See bug 358398. My fix to bug 358398 has unfortunately regressed this bug. Patch coming up.

* Switches from mColumnsScrollableView->GetView()->GetBounds to mColumnsFrame->GetRect() as this works when the height is zero (roc?) * Caches the difference in widths between the columns frame and itself * Adds on that difference almost every time the last column is used (the exception is in EnsureCellIsVisible)

Comment on attachment 275420 [details] [diff] [review] Proposed patch > // If no horz scrolling periphery is present, then just >- // return the width of the main box >+ // return the width of the columns > if (width == 0) { >- CalcInnerBox(); >- width = mInnerBox.width; >+ width = aParts.mColumnsFrame->GetRect().width; > } > >+ // Compute the adjustment to the last column. This varies depending on the >+ // visibility of the columnpicker and the scrollbar. >+ mAdjustWidth = mRect.width - aParts.mColumnsFrame->GetRect().width; >+ Do we still need to add back on the padding/border here? >+ // The amount by which to adjust the width of the last cell. >+ // This depends on whether or not the columnpicker and scrollbars are present. >+ nscoord mAdjustWidth; Add a more detailed comment here that says what this is width is, the difference between the treechildren and the treecols excluding column picker, right?

(In reply to comment #34) >>+ mAdjustWidth = mRect.width - aParts.mColumnsFrame->GetRect().width; >Do we still need to add back on the padding/border here? I don't know; tree body frames don't seem to like left/right padding/border. >>+ // The amount by which to adjust the width of the last cell. >>+ // This depends on whether or not the columnpicker and scrollbars are present. >>+ nscoord mAdjustWidth; >Add a more detailed comment here that says what this is width is, the >difference between the treechildren and the treecols excluding column picker, >right? Will do.

Would it make more sense to add mAdjustWidth in col->GetRect and GetWidthInTwips instead, and then subtract it where we don't need it?

Comment on attachment 277777 [details] [diff] [review] With the adjustment moved to GetRect/WidthInTwips Does this need a new r?

Comment on attachment 277777 [details] [diff] [review] With the adjustment moved to GetRect/WidthInTwips probably doesn't need another r

Comment on attachment 277777 [details] [diff] [review] With the adjustment moved to GetRect/WidthInTwips Looking to finally correctly fix this regression from the horizontal tree scrolling implementation.

Fix checked in.

D'oh, my previous patch didn't test for the last column very well. This test works when you hide the last column(s) with the columnpicker.

Comment on attachment 277967 [details] [diff] [review] Better last column test >+PRBool >+nsTreeColumn::IsLastVisible(nsTreeBodyFrame* aBodyFrame) >+{ >+ // we're certainly not the last visible if we're not visible >+ if (GetFrame(aBodyFrame)->GetRect().width == 0) >+ return PR_FALSE; >+ >+ // try to find a visible successor >+ for (nsTreeColumn *next = GetNext(); next; next = next->GetNext()) { >+ if (next->GetFrame(aBodyFrame)->GetRect().width > 0) >+ return PR_FALSE; >+ } >+ return PR_TRUE; >+} >+ GetFrame can return null.

D'oh, I'm not doing well with these null pointer tests, am I? This patch only null-checks next->GetFrame(aTreeBodyFrame) as all three callers have already checked the result of this->GetFrame directly or indirectly.

Comment on attachment 278206 [details] [diff] [review] Fixed last column test Fix for regression from previous fix. (And it's not even the only one. Sigh...)

With attachment 278206 [details] [diff] [review] applied, any cycler column at the right end of a tree with enabled columnpicker will have its values centered on the dividing line between column and columnpicker, if there is no scrollbar. This is not intended, I hope? Example: MailNews threadpane, "junk" column J, columnpicker P: ---+---+---+ ...| J | P | +---+---+ | J | | J | | J |

Reopening again while I'm waiting for attachment 278206 [details] [diff] [review] approval.

Comment on attachment 278206 [details] [diff] [review] Fixed last column test a1.9=dbaron

Fix checked in (including the regression fix regression fix).

I thought I had it working before, but I've just noticed that this isn't fixed in the case of a tree where all the columns are hidden. I think the width adjustment should apply even if the scrollable view was not found or empty. I therefore separated out the width adjustment to clarify the distinction.

That last patch was moved into bug 399376 and has been checked in.