Closed Bug 306990 Opened 14 years ago Closed 12 years ago

Items in trees has a cropped right-margin

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: stefanh, Assigned: neil)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(6 files, 4 obsolete files)

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.
Attached image Screenshots
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.
Blocks: 212789
Yeah, perhaps we're not adjusting the content area for the scrollbar?
Flags: blocking1.9a1+
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?
Attached patch Patch for to fix (obsolete) — Splinter Review
This should fix the problem, although it's not particularly pretty.
Attachment #198361 - Flags: review?(Jan.Varga)
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
Attachment #198361 - Flags: review?(Jan.Varga) → review+
Thanks. Made suggested changes. 

About the mColScrollView. Yes it could be a getter, but I guess that would be
another patch for another day.
Attachment #198361 - Attachment is obsolete: true
Attachment #198448 - Flags: superreview?(roc)
+  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.
Attachment #198448 - Attachment is obsolete: true
Attachment #198652 - Flags: superreview?(roc)
Attachment #198448 - Flags: superreview?(roc) → superreview-
Attachment #198652 - Flags: superreview?(roc)
Attachment #198652 - Flags: superreview+
Attachment #198652 - Flags: review+
Boris, do you have time to check this in? I don't have commit access.
Assignee: Jan.Varga → nielsen
Fixed on trunk.  Thanks, Nate!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch fixSplinter Review
This patch fixes the bug in Firefox Help, and also fixes it in the URL autocomplete dropdown.
Assignee: nielsen → roc
Status: REOPENED → ASSIGNED
Attachment #210975 - Flags: superreview?(dbaron)
Attachment #210975 - Flags: review?(Jan.Varga)
Comment on attachment 210975 [details] [diff] [review]
fix

looks good to me
Attachment #210975 - Flags: review?(Jan.Varga) → review+
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.
Attachment #210975 - Flags: superreview?(dbaron) → superreview+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
*** Bug 310710 has been marked as a duplicate of this bug. ***
Depends on: 334512
Blocks: 334512
No longer depends on: 334512
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Proposed patch (obsolete) — Splinter Review
* 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)
Assignee: roc → neil
Status: REOPENED → ASSIGNED
Attachment #275420 - Flags: superreview?(roc)
Attachment #275420 - Flags: review?(enndeakin)
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?
Attachment #275420 - Flags: review?(enndeakin) → review+
(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?
Attachment #277777 - Flags: superreview?(roc)
Comment on attachment 277777 [details] [diff] [review]
With the adjustment moved to GetRect/WidthInTwips

probably doesn't need another r
Attachment #277777 - Flags: superreview?(roc) → superreview+
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.
Attachment #277777 - Flags: approval1.9?
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago12 years ago
Resolution: --- → FIXED
Attached patch Better last column test (obsolete) — Splinter Review
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.
Attachment #275420 - Attachment is obsolete: true
Attachment #277967 - Flags: superreview?(roc)
Attachment #277967 - Flags: review?(enndeakin)
Attachment #275420 - Flags: superreview?(roc)
Attachment #277967 - Flags: superreview?(roc) → superreview+
Depends on: 393665
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.
Attachment #277967 - Attachment is obsolete: true
Attachment #278206 - Flags: superreview+
Attachment #278206 - Flags: review?(enndeakin)
Attachment #277967 - Flags: review?(enndeakin)
Attachment #278206 - Flags: review?(enndeakin) → review+
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...)
Attachment #278206 - Flags: approval1.9?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 278206 [details] [diff] [review]
Fixed last column test

a1.9=dbaron
Attachment #278206 - Flags: approval1.9? → approval1.9+
Fix checked in (including the regression fix regression fix).
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Attachment #283993 - Flags: superreview?(roc)
Attachment #283993 - Flags: review?(enndeakin)
Attachment #283993 - Flags: review?(enndeakin) → review+
That last patch was moved into bug 399376 and has been checked in.
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Blocks: 334737
No longer blocks: 334512
You need to log in before you can comment on or make changes to this bug.