skeemer Reporter
	Description • 19 years ago

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.

	skeemer Reporter
	Comment 1 • 19 years ago

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

	Neil Deakin
	Comment 2 • 19 years ago

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

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

	JP
	Comment 3 • 18 years ago

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?

	Stefan [:stefanh]
	Comment 4 • 18 years ago

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

	neil@parkwaycc.co.uk
	Comment 5 • 18 years ago

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.

	JP
	Comment 6 • 18 years ago

Ok, i will use trunk version.

	JP
	Comment 9 • 18 years ago

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

	JP
	Comment 10 • 18 years ago

UL example.

	JP
	Comment 11 • 18 years ago

CSS example.

	Stefan [:stefanh]
	Comment 12 • 18 years ago

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

	neil@parkwaycc.co.uk
	Comment 13 • 18 years ago

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.

	JP
	Comment 16 • 18 years ago

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

	JP
	Comment 17 • 18 years ago

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.

	Bill Zaroy
	Comment 18 • 16 years ago

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.

	Stefan [:stefanh]
	Comment 19 • 16 years ago

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

	Bill Zaroy
	Comment 21 • 16 years ago

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?

	Stefan [:stefanh]
	Comment 22 • 16 years ago

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

	Bill Zaroy
	Comment 23 • 16 years ago

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.

	Neil Deakin
	Comment 24 • 16 years ago

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.

	neil@parkwaycc.co.uk
	Comment 25 • 16 years ago

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 (

	Bill Zaroy
	Comment 26 • 16 years ago

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

	Neil Deakin
	Comment 27 • 16 years ago

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?

	Bill Zaroy
	Comment 28 • 16 years ago

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.

	neil@parkwaycc.co.uk
	Comment 29 • 16 years ago

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

IIRC adding tests doesn't specifically require sr.

	Bill Zaroy
	Comment 30 • 16 years ago

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

	Neil Deakin
	Comment 31 • 16 years ago

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.

	Bill Zaroy
	Comment 32 • 16 years ago

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.

	Bill Zaroy
	Comment 33 • 16 years ago

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)

	Neil Deakin
	Comment 34 • 16 years ago

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.

	Neil Deakin
	Comment 35 • 16 years ago

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.

	Bill Zaroy
	Comment 36 • 16 years ago

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

	Neil Deakin
	Comment 37 • 16 years ago

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.

	Neil Deakin
	Comment 39 • 16 years ago

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

OK, that looks good.

	Neil Deakin
	Comment 40 • 16 years ago

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