97519 - Alignment of table is shown as the alignment of the cell text even if they are different

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[TucsonTester2]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.3+) Gecko/20010828
Netscape6/6.1b1
BuildID:    20010828

When text is in a table, the text alignment is not always properly showing.  If
you center a line that includes a table using the center alignment in the
toolbar then the text in the cells won't show the proper alignment.

Reproducible: Always
Steps to Reproduce:
1.Open Composer
2.Make a 2x2 100pixel table using the table button on the toolbar
3.Click to the left of the table to place the cursor and then click on the
center align button in the toolbar
4.Place the cursor in one of the table cells

Actual Results:  If you look at the alignment button on the toolbar you will see
that it shows the cell is center aligned.  If you type in text you will notice
that the text is left aligned.  This also can be done if the table is right
aligned using the toolbar.  Left alignment works fine even if the text is
aligned differently in the cell.

Expected Results:  The composer window should reflect the proper settings I have
for a cell.  I should be able to see that the text will be left aligned when I
type, instead of center aligned.

Comment 1

[Kathleen :Brade]

-->cmanske

Comment 2

[Charles Manske]

First, placing the caret to the left of the table, then clicking on Center 
button should not (and does not in current build) center the table. 
If you select the table first, then use Center button, the table will be 
centered. And in that case, you are correct that when you click inside a cell,
which has default left-aligned text, the toolbar button shows centered.
Note that if we fix bug 96444, which will put the align attribute on the <table>
tag instead of wrapping table in <div align="center">, this problem doesn't 
occur.
But it seems that nsHTMLEditRules::GetAlignment() should stop at TD, TH or other
table elements when it does "check up the ladder for divs with alignment",
and check the "align" attribute of the TD/TH instead.
I don't think we should morph this bug, but we do need a general bug on the 
issue: 'Use selected elements "align" attribute instead of "div" for elements
that support it (e.g. HR, TABLE, TD...)'. But then there's the complication
of "align" being deprecated for HR. See the closely-related bug 78703.

Comment 3

[Joe Francis]

alignment is a mess in general.  There is no general solution that has all the 
desired qualities: simplicity, valid html, backwards compatibilty, non-intrusive 
(whereby non-intrusive I mean that it does the minimum amount of structural 
change to your document).  The approach I chose meets the first three but not the 
fourth, and also suffers from correctness problems.  Fixing the correctness 
problems will make it also lack simplicity.

Basically, HTML sux as a document format for editting.

Comment 4

[Joe Francis]

097

Comment 5

[Joe Francis]

Charlie, Kathy:
I need some elp here figuring out what the bug is.  Reading this report again I
don't understand what it means.  Can one of you figure out what it is I'm
supposed to fix?  If so, please tell me.  A test case would be wonderful!

Comment 6

[TucsonTester2]

If you center a table using the toolbar and put the cursor in one of the cells 
the toolbar will show that the text is center aligned in the cell.  Instead the 
toolbar should show that the text is left aligned.

Comment 7

[Charles Manske]

What is happening is that GetAlignment() goes up the parent node chaing untill it
sees the <div align="Center"> element. But if you're in a cell, the default 
alignment is center. The immediate fix for this would be to stop the parent 
search at the TD, like you did to solve other problems
This is related to a request that the GetAlignment() and SetAlignment()
(bug 54912 and bug 78703) should be smarter and detect when the selected element 
is a Table, Cell, HLine (or other elements that have their own "align"
attribute) and change that instead of using the "div" element.
I also had a bug on this issue (bug 96444) that I've just duped to 54912.
Other related alignment issues are in bug 74051 and 87671.
I bet there's more dups or at least dependencies that should be made between 
these bugs.

Comment 8

[Charles Manske]

Last explanation is confused! Should say:
"What is happening is that GetAlignment() goes up the parent node chain until it
sees the <div align="Center"> element. But if you're in a cell, the default 
alignment for the cell contents is *left*..."

Comment 9

[Kathleen :Brade]

Attachment: test case

Comment 10

[Kathleen :Brade]

Daniel has been working on css-ization of Composer and table dialog in particular.
Daniel--do you have any input on this?  Will your stuff help/fix this bug?

Comment 11

[Kathleen :Brade]

It seems like alignment should be set on the <td> or <table> instead of with a 
<div>.  Does that change this bug at all or should we file a different bug on 
that issue?

Comment 12

[Charles Manske]

Kathy: that is what bug 54912 is about.

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Kathy: yes, my code helps a lot because it emulates the notion of inline-block.
       Basically, I climb in the hierarchy of the selection until I find the
       first block-level element. In the example given by the reporter, the
       table is at the same time a block (a table *is* a block) and an inline,
       in the flow of the line. But of course, it'll work only in CSS mode...

All: I think I can have an easy fix for this bug in both standard mode and the
     future css mode. I'll propose a patch for the existing mode first and will
     let bug 77705 handle the css case.

Joe : feel free to reassign this bug to me if you want/need to. I can play with
      that. You can also mark 1 day for the EDITORBASE tagging.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Getting ownership of this bug since I have a patch for it

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0 ; ready for reviews

Comment 16

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Patch v1.0 attached. Tested successfully on win2k and rh7 with various test
cases. Joe, can you r= please ? Kin, can you sr= please (easy fix) ? Thanks.

Comment 17

[Joe Francis]

r=jfrancis

Comment 18

[kinmoz]

Comment on attachment 50684 [details] [diff] [review]
patch v1.0 ; ready for reviews

sr=[e-mail]

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in trunk

Comment 20

[James Baxter]

Verified in 10-01 Trunk build