57206 - set/remove valign and align for td as necessary

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[salsa_43]

I created a a table in the editor, then tried to get a valign="middle" but it
had no effect, when I view the html source the td tag no longer has a valign
section.

Comment 1

[rubydoo123]

assigning to brade for starters, the toggle between html source and normal is
eating attributes

Comment 2

[salsa_43]

It seems there are two problems.

1) Using the table cell properties "dialog" (is that the right term?) it is not
possible to set the valign to middle.

I examined the html generated without using the html source view and the valign
tag was there but was still set to "Top"

2) After attempting to set the valign to middle switching from normal view to
html source view loses the valign tag completely.  (possibly because the
previous step did something bad?)

Comment 3

[Kathleen :Brade]

ok, I may or may not be right about this...

1) the default for a cell valign is "middle" (according to page 121 of my HTML4 
spec).  Typically Composer doesn't emit default values (it would just add clutter 
to most people's HTML source).  So, if you picked "middle" for vertical alignment 
in the properties dialog, then you shouldn't see any valign attribute.

2) I think the root of this problem is that Composer adds <tr> tags with a 
valign="Top".  When the <td> doesn't have a vertical alignment, the layout engine 
uses the vertical alignment specified on the <tr>.

I think that the correct fix for this would be for Composer to not specify an 
attribute on the <tr> tag.  

If we have a good reason to do this, then the right fix would be to change the 
logic in the table cell tab's JS to check the <tr> alignment settings and not 
remove the attribute if the <tr> and <td> settings didn't match.

Probably we really need to do both of the above things (to accommodate changes to 
existing files as well as newly created tables).

An easy/safe fix in the shortrun would be to always write out the attribute tag 
for vertical alignment.


Beppe--I don't understand your comment about html source view eating attributes.  
Please explain.  Are you just seeing the valign attribute disappearing because it 
is the default?

Workaround--in html source mode, remove the valign tag from your <tr> tags (or 
use your own favorite editor).  This will allow the table cell property dialog to 
make alignment settings as expected.

Comment 4

[Gervase Markham [:gerv]]

Another one for the Composer table alignment relnote pile...

Gerv

Comment 5

[Kathleen :Brade]

Attachment: part 1 of patch (don't set valign attribute on tr)

Comment 6

[Kathleen :Brade]

Attachment: part 2 of patch (don't allow valign tag to clear)

Comment 7

[Kathleen :Brade]

2 patches attached; these should fix most of the problem.  To really fix this
bug properly we need to be smarter about climbing up the hierarchy and determine
if the <tr> or <table> tags have vertical alignment set.  If someone does, we
can't remove valign otherwise we should remove the valign attribute.

Comment 8

[rubydoo123]

I'm not sure where you saw the discussion in the spec about the default being 
middle, looking at the spec the default should be top -- based on the ordering 
specified in the attribute list (which is normally how you would determine the 
default) unless explicitly specified in the spec. But, I reviewed section 11 of 
the 4.01 spec and saw nothing that stated the default is middle. In addtion to 
not finding that, they specify the ordering in how alignment is established:
-------------------------------------------------------------------------------
Per section 11.3.2:
Inheritance of alignment specifications
The alignment of cell contents can be specified on a cell by cell basis, or 
inherited from enclosing elements, such as the row, column or the table itself.
The order of precedence (from highest to lowest) for the attribute valign (as 
well as the other inherited attributes lang, dir, and style) is the following:

1.An attribute set on an element within a cell's data (e.g., P). 
2.An attribute set on a cell (TH and TD). 
3.An attribute set on a row or row grouping element (TR, THEAD, TFOOT, and 
TBODY). When a cell is part of a multi-row span, the attribute value is 
inherited from the cell definition at the beginning of the span. 
4.An attribute set on a column grouping element (COL and COLGROUP). When a cell 
is part of a multi-column span, the attribute value is inherited from the cell 
definition at the beginning of the span. 
5.An attribute set on the table (TABLE). 
6.The default attribute value. 

----------------------------------------------
valign is defined as:
 "valign     (top|middle|bottom|baseline) #IMPLIED"

the rule, is if the implied default value is not specified, then the first order 
 value is the default value.

For example, the scrolling attribute is defined this way:
scrolling   (yes|no|auto)  auto      -- scrollbar or none --
 and auto is expressed as the default value
--------------------------------------------------------------------------------
Kathy, as for eating attributes, toggling between source mode and normal mode, 
in the past, has resulted in certain attributes getting deleted from the 
content. I believe either Joe, Charlye or Kin worked on that issue.

Comment 9

[Kathleen :Brade]

This bug isn't about eating attributes, it's about setting valign="middle"

The problem (in the case I found) is that a table created with the current build 
of Composer will insert a valign="top" on the <tr> as well as the <td>.  However, 
we don't provide an easy way for users to remove that attribute from the <tr> (I 
think only by editing the raw html source works right now).  With the valign=
"top" on a <tr>, the clearing of the valign tag (whichever the default is) will 
always result in the table row setting being triggered.

In a similar scenario, you could envision someone editing an existing document 
with a <tr> valign attribute set to something (doesn't matter).  We currently 
blindy add or remove the valign attribute from the <td> when we really should be 
checking the <tr>.

I hope this makes more sense to others now.  The two patches I have attached will 
cause composer to never set the valign attribute on a newly created table's <tr> 
and always cause Composer to write out the valign attribute for a <td> (since we 
don't yet have logic to be smart and write it only when necessary).

Comment 10

[rubydoo123]

kathy stated: "This bug isn't about eating attributes, it's about setting 
valign="middle""

I know that - but, you asked in a previous comment:
"Beppe--I don't understand your comment about html source view eating 
attributes. Please explain."
and I just answered that question.

Why did we just blindly delete? odd behavior, good catch on that one

Comment 11

[kinmoz]

FYI: according to http://www.w3.org/TR/html4/struct/tables.html#adef-valign 
"middle" is the default value for valign.

Comment 12

[kinmoz]

sr=[e-mail]

Spoke to [e-mail] and the plan is to check these patches in to get 
things working again, and then pass this bug off to [e-mail] to 
decide or not to implement the hierarchy traversal code that determines whether 
or not to write out the align or valign attributes.

Comment 13

[Kathleen :Brade]

The patches above are checked in.

Change summary from:
  modifying table cell property vertical alignment to middle doesn't work
to:
  set/remove valign and align for td as necessary

Reassign this bug to cmanske for the following work:
  * be smart about writing the valign attribute (look up the hierarchy to see 
what parent setting is)
  * be smart about writing the align attribute as well
  * (are there any other attributes we need to be smarter about?)

Comment 14

[Charles Manske]

What is milestone "mozilla0.6" used for? I'm changing this to 'mozilla0.9'

Comment 15

[Charles Manske]

I completely agree that always setting the "valign" and "align" attributes is
the best way to solve this problem. The attributes will make copying/pasting
cells better by including the attributes.
Contrary to above statement, "Part 1" patch has not been checked in.
I think it's a good idea to do so, so I just checked it in.
I also checked in a comment in EdTableProps.js to explain the "part 2" change.
I'll attach a new diff for the "align" attribute changes.

Comment 16

[Charles Manske]

Attachment: Always set the "align" attribute on table cells

Comment 17

[Charles Manske]

The diff is hard to understand, imho, so here's the block with the new code to
set the "align" attribute (also fixes the wrong word "Vertical" in comment):
  if (dialog.CellHAlignCheckbox.checked)
  {
    // Horizontal alignment is complicated by "char" type
    var hAlign = dialog.CellHAlignList.selectedItem.data;

    if (hAlign == charStr)
    {
      //Note: Is space a valid align character?
      // Assume yes and don't use "trimString()"
      var alignChar = dialog.CellAlignCharInput.value.charAt(0);
      globalCellElement.setAttribute(charStr, alignChar);
      if (!alignChar)
      {
        ShowInputErrorMessage(GetString("NoAlignChar"));
        SetTextfieldFocus(dialog.CellAlignCharInput);
        return false;
      }
    }
    else
    {
      globalCellElement.removeAttribute(charStr);
    }

    // Always set "align" attribute,
    //  so the default "left" is effective in a cell
    //  when parent row has align set.
    globalCellElement.setAttribute("align", hAlign);
  }

Comment 18

[Kathleen :Brade]

r=brade

Comment 19

[Simon Fraser [no longer active]]

sr=sfraser

Comment 20

[Charles Manske]

We always set "align" attribute except when original value was "char" and user
didn't change the Horizontal Alignment menulist (that issues is bug 69795).

Comment 21

[Charles Manske]

Forgot to check "fixed"

Comment 22

[James Baxter]

Verified on 9-03 build