301342 - treecell xul tag does not align or display text content only labels

Status: NEW

(Core :: XUL, defect)

Description

[skeemer]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5

<treecell> ignores align="center", align="end", and CSS.
It also will not show anything nested (ex. <treecell>some text</treecell>).
It appears the only thing that will display is <treecell label="sometext"/>.

Reproducible: Always

Steps to Reproduce:
1. <treecell>some text</treecell>
2. <treecell label="some text" align="center"/>
3. <treecell label="some text" style="text-align: center;"/>
Actual Results:  
1. No text shows.
2. "some text" remains left justified
3. Same as 2. DOM inspector shows the style command did insert it into the CSS

Expected Results:  
1. Show "some text" in cell
2. Center justify "some text"
3. Same as 2.

This remains true in recent Deer Park builds.

Comment 1

[skeemer]

Sorry, the expected may be wrong and should be a pack instead of align, but neither work.

Comment 2

[Neil Deakin]

Currently, the way to do text alignment is <treecol style="text-align: center"/>

But we should implement support for text alignment of individual cells.

Comment 3

[JP]

I need this feature only for cells of type TEXT, and i want to contribute. HOw i submit the patch for nsTreeBodyFrame.cpp? What branch version should be used?

Comment 4

[Stefan [:stefanh]]

(In reply to comment #3)
> I need this feature only for cells of type TEXT, and i want to contribute. HOw
> i submit the patch for nsTreeBodyFrame.cpp? What branch version should be used?
> 

Use the trunk version. To get a patch in to a branch tree will require approval from drivers (and it has to be tested on trunk first anyway). CC:ing Neil, who might be able to confirm the bug.

Comment 5

[[e-mail]]

I would have thought it unlikely we ever support arbitrary CSS style.
Neil Deakin already confirmed that it's reasonable to align individual cells.
However it won't be possible to implement this on a branch as it will require changes to branch-frozen interfaces.

Comment 6

[JP]

Ok, i will use trunk version.

Comment 7

[JP]

Attachment: Patch for nsTreeBodyFrame.cpp

Comment 8

[JP]

Attachment: Patch for nsTreeBodyFrame.h

Comment 9

[JP]

I submitted two patchs for nsTreeBodyFrame. Default text align is column align, but if text align for cell specified in style sheet, it's applied. I tested the change, and works fine.

Regards ....

Comment 10

[JP]

Attachment: XUL Example.

UL example.

Comment 11

[JP]

Attachment: CSS example.

CSS example.

Comment 12

[Stefan [:stefanh]]

ramonjp, your patch will need review and super-review. If you just attach the patch it will never get attention.

Comment 13

[[e-mail]]

While this patch looks reasonable, have I misunderstood the point of this bug? You don't seem to have much control over which cells the alignment applies to.

Comment 14

[JP]

Attachment: XUL Example 2: Cells with properties.

Comment 15

[JP]

Attachment: CSS Example 2: Several styles of cell.

Comment 16

[JP]

The cell style is defined by cell properties and css sheet. Of this form an absolute control is had on which cells are aligned and how. 

I am new here, and I do not understand the procedures. As I can ask for a review and super-review of the patch? 


Thanks in Advance...

Comment 17

[JP]

Comment on attachment 228531 [details]
XUL Example 2: Cells with properties.

The example shows like defining several styles of cell that define the alignment of the text.

Comment 18

[Bill Zaroy]

Can anyone tell me why there is no progress on this one in the last two years?
I encounter the problem when i use a custom tree view...

All I really want is to align the header different than the content of the tree.

Comment 19

[Stefan [:stefanh]]

The first thing is to confirm the bug - which I'm doing now since an unconfirmed bug will never get any attention. The next step is that someone steps forward and provides a patch...

Comment 20

[Bill Zaroy]

Attachment: Patch of nsTreeBodyFrame.cpp / .h against trunk

Comment 21

[Bill Zaroy]

Comment on attachment 335198 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk

The patch solved the problem for me and did not cause any fails in the mochitest...

I have no idea who is the appropriate person to add for a review?

Comment 22

[Stefan [:stefanh]]

You need both review and super-review. I'm guessing that the 2 neils that have commented in this bug could do the reviews. But please note comment #13

Comment 23

[Bill Zaroy]

Comment on attachment 335198 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk

actually I don't think comment #13 is still in question as the example attachments show how there is control over the alignment?

What actually seems strange to me is, that the rightmost column is right aligned out of the column... but that is not a concern of this bug as this phenomenon also occurs when defining the text-align in the whole column. This Problem is also present in the current Firefox 3 release.

Comment 24

[Neil Deakin]

Comment on attachment 335198 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk

>+  // Obtain Text Alignment for cell.
>+  PRInt32 aCellTextAlign = textContext->GetStyleText()->mTextAlign;
>+

Just 'cellTextAlign' here.

...

>+  // Obtain Text Alignment for cell.
>+  PRInt32 aCellTextAlign = textContext->GetStyleText()->mTextAlign;
>+

and here.

Also, a test would be nice. The existing tree tests are in toolkit/content/tests/widgets. It looks like we can use getCellAt to check if the text is within a certain area.

Comment 25

[[e-mail]]

Comment on attachment 335198 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk

>+    if(aCellTextAlign == NS_STYLE_TEXT_ALIGN_DEFAULT)
Additional nit: space between if and (

Comment 26

[Bill Zaroy]

Attachment: Patch of nsTreeBodyFrame.cpp / .h against trunk + test

It the first test I have written so far...

Comment 27

[Neil Deakin]

I'm not clear what you're trying to calculate against in the test. Wouldn't we just want to check that getCellAt for three points somewhere in the column (left, center and right) return 'text' or 'cell' as appropriate?

Comment 28

[Bill Zaroy]

I try to check whether the cell alignment overwrites the column alignment. As the column alignment has to be one of the three (left,center,right), is is not possible to check all those in one column. So for the sake of symmetry I did it for all the possible cases.

The reason for checking the empty cases (a position in the cell NOT containing text) is a problem with FF2, which says everything in the cell is text - and thereby passing the test.

Comment 29

[[e-mail]]

Comment on attachment 336069 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk + test

IIRC adding tests doesn't specifically require sr.

Comment 30

[Bill Zaroy]

As I said it is the first test I have written so far. Is there anything I should change?

Comment 31

[Neil Deakin]

I don't understand what the test is doing. It should just select three points, one at the left of the cell text area, one in the middle, and one in the right, and call getCellAt to see if those points return 'text' for the type. The other points should return 'cell'.

The offsetX computations seem needlessly complex. Just pick values in the middle where you think they should be. This would also avoid the last column hack because you pick text and values that is large enough.

Comment 32

[Bill Zaroy]

Attachment: Patch of nsTreeBodyFrame.cpp / .h against trunk + new test

Ok. So i changed the whole thing to check for the three points in every cell and tried to comment it in a way it should be easier to understand.

About the hack... well it actually is no hack, it is a bug in the rendering of the last cell in a row of a tree. I understand that it makes sense to extend the width of the last column to use the whole space including the one under the columnpicker, but i think that should not be done by extending the width of a fixed width column. I think it is a bit hard to follow me without a picture so i will try to make one and attach it.

Comment 33

[Bill Zaroy]

Attachment: Shows the problem with the last column

This is actually not relevant for the patch as is describes a bug already in the ff3 browser. (for example when the whole column is right aligned)

Comment 34

[Neil Deakin]

By 'hack', I meant you don't need to select a point right at the far edge, you can select a point 20 pixels or so away.

Comment 35

[Neil Deakin]

What I mean by that is that you currently are calling getCoordsForCellItem(..., "text", ...) to get the area of the text and then checking if the point is within that (which it seems like it should be naturally). What you should instead be doing is getting the "cell" and then using three points, one that is some number of pixels from the left, one in the middle, and one at the right minus some number of pixels.

Comment 36

[Bill Zaroy]

Attachment: Patch of nsTreeBodyFrame.cpp / .h against trunk + test version 3

Now it checks for the "cell" Coordinates.

In the moment getCoordsForCellItem(..., "text", ...) interestingly always returns the coordinates of left aligned text (maybe the next bug)?

Comment 37

[Neil Deakin]

OK, this looks much easier to understand. The only changes I would make are to use a longer text string than "x" in case of systems/other platforms with small default fonts, and to use a y coordinate in the middle of the cell to avoid any extra borders/margins that might appear for cells on some platforms or in the future.

Comment 38

[Bill Zaroy]

Attachment: Patch of nsTreeBodyFrame.cpp / .h against trunk + test version 4

Comment 39

[Neil Deakin]

Comment on attachment 337967 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk + test version 4

OK, that looks good.

Comment 40

[Neil Deakin]

Comment on attachment 337967 [details] [diff] [review]
Patch of nsTreeBodyFrame.cpp / .h against trunk + test version 4

Actually, I meant to add an extra comment here that you also need to add the test to toolkit/content/tests/widgets/Makefile.in

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.