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

RESOLVED FIXED in mozilla1.9beta5

Status

()

--
minor
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: moz-bugs, Assigned: paul)

Tracking

Trunk
mozilla1.9beta5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 300888 [details]
testcase
(Reporter)

Comment 2

11 years ago
Created attachment 300890 [details] [diff] [review]
trivial fix
Did you want someone to review this patch?
(Reporter)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

11 years ago
Created attachment 307239 [details] [diff] [review]
patch

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)
(Assignee)

Comment 7

11 years ago
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 8

11 years ago
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+
(Assignee)

Comment 9

11 years ago
(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
(Assignee)

Comment 10

11 years ago
Created attachment 307905 [details] [diff] [review]
patch v1
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)
(Assignee)

Updated

11 years ago
Attachment #307905 - Flags: review? → review?(neil)

Comment 11

11 years ago
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+
(Assignee)

Comment 12

11 years ago
Who should I ask super review to ?
(Assignee)

Updated

11 years ago
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)

Updated

11 years ago
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+
Keywords: checkin-needed
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
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5

Updated

10 years ago
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.