set/remove valign and align for td as necessary

VERIFIED FIXED in mozilla0.9

Status

()

P3
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: salsa_43, Assigned: cmanske)

Tracking

Trunk
mozilla0.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIX IN HAND; waiting to checkin)

Attachments

(3 attachments)

(Reporter)

Description

19 years ago
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

19 years ago
assigning to brade for starters, the toggle between html source and normal is
eating attributes
Assignee: beppe → brade
Target Milestone: --- → mozilla0.9
(Reporter)

Comment 2

19 years ago
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

19 years ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: relnoteRTM
OS: Windows NT → All
Hardware: PC → All
Target Milestone: mozilla0.9 → mozilla0.6
Another one for the Composer table alignment relnote pile...

Gerv
Whiteboard: relnote-user

Comment 7

19 years ago
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

19 years ago
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

19 years ago
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

19 years ago
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

Updated

19 years ago
Whiteboard: relnote-user → relnote-user, patch attached

Comment 11

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

Comment 12

19 years ago
sr=kin@netscape.com

Spoke to brade@netscape.com and the plan is to check these patches in to get 
things working again, and then pass this bug off to cmanske@netscape.com 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

19 years ago
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?)
Assignee: brade → cmanske
Status: ASSIGNED → NEW
Summary: modifying table cell property vertical alignment to middle doesn't work → set/remove valign and align for td as necessary
Whiteboard: relnote-user, patch attached → relnote-user
(Assignee)

Comment 14

19 years ago
What is milestone "mozilla0.6" used for? I'm changing this to 'mozilla0.9'
Status: NEW → ASSIGNED
Target Milestone: mozilla0.6 → mozilla0.9
(Assignee)

Comment 15

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

Whiteboard: relnote-user → FIX IN HAND
(Assignee)

Comment 17

18 years ago
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

18 years ago
r=brade
Whiteboard: FIX IN HAND → FIX IN HAND; waiting for sr=

Comment 19

18 years ago
sr=sfraser

Updated

18 years ago
Whiteboard: FIX IN HAND; waiting for sr= → FIX IN HAND; waiting to checkin
(Assignee)

Comment 20

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

Comment 21

18 years ago
Forgot to check "fixed"
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 22

18 years ago
Verified on 9-03 build
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.