phax / ph-oton

The complete ph websuite
Apache License 2.0
3 stars 5 forks source link

HCTFoot wrong argument in super constructor call #19

Closed NikovacsDev closed 4 months ago

NikovacsDev commented 4 months ago

The current super call in the constructor is called with true:

  public HCTFoot ()
  {
    super (EHTMLElement.TFOOT, true);
  }

I guess that's a bug because this sets class attribute m_bHeaderOrFooter to true but that should only happen for Headers right?

phax commented 4 months ago

@NikovacsDev thanks for reaching out to me. As the variable is called "m_bHeaderOrFooter" it is okay, to pass true. In the end this is only needed to make sure to create <th> elements inside <thead> and <tfoot> and <td> elements inside <tbody>. hth

NikovacsDev commented 4 months ago

But when I call the addRow() from HCTFoot it calls the addRow() from AbstractHCTablePart, inside of that it creates a new HCRow(true) because the m_bHeaderOrFooter is true. This creates a new HCRow with the attribute m_bHeader = true; Now when I later add a cell to that row with addCell() it calls the following code:

  public final IHCCell <?> addCell ()
  {
    final AbstractHCCell <?> ret = m_bHeader ? new HCTH () : new HCTD ();
    ret.internalSetParentRow (this);
    addChild (ret);
    return ret;
  }

In the end this would lead to the following HTML:

<tfoot>
   <tr>
         <th></th> 
   </tr>
</tfoot>
phax commented 4 months ago

Yes, that is correct. That is based on a previous version of the spec, where tfoot should always contain only th elements. If this is no longer the case, we need an API extension to make sure, tfoot tr elements can handle both td and th elements. Do you know if this applies to thead and tbody elements as well?

phax commented 4 months ago

I just noticed, that the th had no support for the scope attribute, so added that anyway

NikovacsDev commented 4 months ago

As far as I know it should be possible to have both cases, but it should not automatically make all the cells of the row th or td elements. Because you could have a table where the headlines are on the horizontal axis or on the vertical axis. I hope that makes sense, if not we could make a short call about that.

Example for vertical:

<table>
    <thead>
    <tr>
        <th>Heading</th>
        <td>Text</td>
    </tr>
    </thead>
    <tbody>
    <tr>
        <th>Heading</th>
        <td>Text</td>
    </tr>
    </tbody>
    <tfoot>
    <tr>
        <th>Heading</th>
        <td>Text</td>
    </tr>
    </tfoot>
</table>

Example for horizontal:

<table>
    <thead>
    <tr>
        <th>Heading</th>
        <th>Heading 2</th>
    </tr>
    </thead>
    <tbody>
    <tr>
        <td>Text</td>
        <td>Text</td>
    </tr>
    </tbody>
    <tfoot>
    <tr>
        <td>Text</td>
        <td>Text</td>
    </tr>
    </tfoot>
</table>
phax commented 4 months ago

I added an overload HCRow.addCell(boolean bHeader) to be able to create TH or TD elements for each row. The provided values in the constructor are now solely "default values" for backwards compatibility.