Closed Bug 415257 Opened 12 years ago Closed 12 years ago

Editable trees: input sized wrongly and rows jumping with flexible and non-flexible treecols

Categories

(Core :: XUL, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: moz-bugs, Assigned: paul)

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre
Build Identifier: 

With an editable trees consisting of a treecol with flex="1" and one with flex="0", the edit field is sized wrongly and the flexible column jumps around when the inflexible column is edited.

I'll attach a testcase and a patch immediately.

Reproducible: Always
Attached file testcase
Attached patch trivial fix (obsolete) — Splinter Review
Did you want someone to review this patch?
Well, I have no idea what to put into all these fields for the patch to be reviewed. But since I had already found the problem when trying to find an error in my code, I figured I could also attach the patch.
Attached patch patch (obsolete) — Splinter Review
tree.xml:
The input field must be displayed *after* resizing.

nsTreeBodyFrame.cpp:
GetCoordsForCellItem must consider scrolled position.
EnsureCellIsVisible must not remove the column picker width (since cells of the last column use its space).
Attachment #307239 - Flags: review?(enndeakin)
Comment on attachment 307239 [details] [diff] [review]
patch

Not sure what the change in  EnsureCellIsVisible is for. Maybe another Neil should review this.
Attachment #307239 - Flags: review?(neil)
In the specific case of the last column, the real width of the cell includes the columnPicker width. Currently, if you use EnsureCellIsVisible with such a cell, the page is not enough scrolled (missing the columnPicker size), so the input field overlap the scrollbar (because the input field use the good with of the cell, with the columnPicker width).
Comment on attachment 307239 [details] [diff] [review]
patch

>   nscoord currX = mInnerBox.x;
I think you should subtract the horizontal position here, rather than alter.

>-  if (col->IsLastVisible(this))
>-    columnWidth -= mAdjustWidth; // this is one case we don't want to adjust
I can't remember why I wrote this, but I agree it's wrong - I created a cell-based selection and found that this fixed selecting the rightmost cell.

>+            input.hidden = false;
I didn't test this but it looks right.
Attachment #307239 - Flags: review?(neil) → review+
(In reply to comment #8)
> (From update of attachment 307239 [details] [diff] [review])
> >   nscoord currX = mInnerBox.x;
> I think you should subtract the horizontal position here, rather than alter.
Yes, you're right.

> >-  if (col->IsLastVisible(this))
> >-    columnWidth -= mAdjustWidth; // this is one case we don't want to adjust
> I can't remember why I wrote this, but I agree it's wrong - I created a
> cell-based selection and found that this fixed selecting the rightmost cell.
> 
> >+            input.hidden = false;
> I didn't test this but it looks right.
If it's done before being resized, the old size could cause some weird scroll behavior.


Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch v1Splinter Review
Assignee: nobody → paul.rouget
Attachment #300890 - Attachment is obsolete: true
Attachment #307239 - Attachment is obsolete: true
Attachment #307905 - Flags: review?
Attachment #307239 - Flags: review?(enndeakin)
Attachment #307905 - Flags: review? → review?(neil)
Comment on attachment 307905 [details] [diff] [review]
patch v1

I tried this with a columnpicker and no scrollbar, and with a scrollbar and no columnpicker, and with neither.
Attachment #307905 - Flags: review?(neil) → review+
Who should I ask super review to ?
Attachment #307905 - Flags: superreview?(enndeakin)
Attachment #307905 - Flags: approval1.9?
Comment on attachment 307905 [details] [diff] [review]
patch v1

I'm not a superreviewer but we can ask the other Neil
Attachment #307905 - Flags: superreview?(enndeakin) → superreview?(neil)
Attachment #307905 - Flags: superreview?(neil) → superreview+
Comment on attachment 307905 [details] [diff] [review]
patch v1

a1.9=beltzner
Attachment #307905 - Flags: approval1.9? → approval1.9+
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.350; previous revision: 1.349
done
Checking in toolkit/content/widgets/tree.xml;
/cvsroot/mozilla/toolkit/content/widgets/tree.xml,v  <--  tree.xml
new revision: 1.55; previous revision: 1.54
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.