306990 - Items in trees has a cropped right-margin

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Stefan [:stefanh]]

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.

Comment 1

[Stefan [:stefanh]]

Attachment: Screenshots

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

Comment 2

[Stefan [:stefanh]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Stef Walter]

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?

Comment 5

[Stef Walter]

Attachment: Patch for to fix

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

Comment 6

[[e-mail]]

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?

Comment 7

[Stef Walter]

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 8

[Jan Varga [:janv]]

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

Comment 9

[Stef Walter]

Attachment: Now with Jan's review suggestions

Thanks. Made suggested changes. 

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 11

[Stef Walter]

Attachment: Including roc's suggestions

Comment 12

[Stef Walter]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Thanks, Nate!

Comment 14

[Stefan [:stefanh]]

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.

Comment 15

[Stef Walter]

Nate has no mac. Nate is a sad boy.

Anyone else with the necessary equipment want to take this?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Josh, Simon, can you maybe take a look?

Comment 17

[Stefan [:stefanh]]

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

Comment 18

[Stefan [:stefanh]]

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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

Comment 20

[Jan Varga [:janv]]

Comment on attachment 210975 [details] [diff] [review]
fix

looks good to me

Comment 21

[Stef Walter]

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. 

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[forrest gump]

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>

Comment 27

[[e-mail]]

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.

Comment 28

[[e-mail]]

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?

Comment 29

[forrest gump]

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.

Comment 30

[[e-mail]]

(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, [e-mail], [e-mail] and [e-mail]

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[[e-mail]]

(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.

Comment 33

[[e-mail]]

Attachment: Proposed patch

* 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 34

[Neil Deakin]

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?

Comment 35

[[e-mail]]

(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.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 37

[[e-mail]]

Attachment: With the adjustment moved to GetRect/WidthInTwips

Comment 38

[[e-mail]]

Comment on attachment 277777 [details] [diff] [review]
With the adjustment moved to GetRect/WidthInTwips

Does this need a new r?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 277777 [details] [diff] [review]
With the adjustment moved to GetRect/WidthInTwips

probably doesn't need another r

Comment 40

[[e-mail]]

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.

Comment 41

[[e-mail]]

Fix checked in.

Comment 42

[[e-mail]]

Attachment: Better last column test

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 43

[Neil Deakin]

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.

Comment 44

[[e-mail]]

Attachment: Fixed last column test

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 45

[[e-mail]]

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...)

Comment 46

[Karsten Düsterloh]

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   |

Comment 47

[[e-mail]]

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

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 278206 [details] [diff] [review]
Fixed last column test

a1.9=dbaron

Comment 49

[[e-mail]]

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

Comment 50

[[e-mail]]

Attachment: Fix hidden header case

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.

Comment 51

[Jesse Ruderman]

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