211636 - nsHTMLTableElement MapAttributesIntoRule is broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsHTMLTableElement's MapAttributesIntoRule is seriously broken.  It's making
lots of checks of the rule data's style context -- attribute mapping functions
shouldn't look at the rule data's style context, since that means what they do
doesn't apply to any element that has the unique set of attributes.

Furthermore, I have no idea why nsHTMLTableElement's mapping function is
concerned with the possibility that the table element might have a 'table-cell'
display type.

(nsHTMLSharedLeafElement has the same problem for spacer elements.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's going on with nsHTMLTableElement is that nsHTMLTableCellElement overrides
WalkContentStyleRules in a bizarre way.  It really needs to at least pass a
parameter in rather than testing the display value.  It also needs to stop
assuming (for XHTML) that there's a table-row-group.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The table element really should be maintaining two separate style rules -- one
for the table, and one for its cells -- what it currently does is invalid.)

Comment 3

[Hixie (not reading bugmail)]

If you're going to rewrite this, maybe you can try to fit in some sort of
support for bug 915. :-) (Probably not completely compliant support, but I think
the people asking for bug 915 would settle for IE-level support...)

Comment 4

[Bernd]

> The table element really should be maintaining two separate style rules

Is there another element in our tree which does maintain two separate style rules correctly?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not in the tree, but there's an example in my patch for bug 915:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ef3c9df8cda4/column-align-attribute

Comment 6

[Bernd]

Attachment: patch

Comment 7

[Bernd]

Attachment: patch

This fixes the table part, includes the reftests for the dependent bugs. The technically difficult stuff is copied form dbarons patch referenced in comment 5. I wouldn't be able otherwise to write by myself such code.
Its a sort of a finish for my journey into content land, all other table bugs that I am aware off are confined to layout/tables.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 424508 [details] [diff] [review]
patch

>+void
>+nsHTMLTableCellElement::BuildInheritedAttributes()

So this is copied a little too literally from the other patch.  In
particular, you'd be better off caching the nsMappedAttributes object on
the table rather than the table cell.  So I'd add a function on
nsHTMLTableElement called something like GetAttributesMappedForCell()
that returns an nsMappedAttributes*.

You also need to handle the cellpadding attribute changing or being
removed, which this patch doesn't currently handle.  That should be
relatively straightforward if it's on the table, though.  For examples,
see nsHTMLLegendElement::SetAttr and UnsetAttr; you'd just need to
either set a magic dirty value or just rebuild mAttributesForCell right
there.

>+  if (sheet) {

This should be if (sheet && table).  (Though it'll need to change for
the work addressing the previous comment, but you still do want to
null-check the table.)

You should add a crashtest for this; an XHTML file with a <td> and no
containing <table> should do fine.  (That crashes without this fix,
right?)

>+        // Reset the stylesheet of modifiableMapped so that it doesn't
>+        // spend time trying to remove itself from the hash. There is no
>+        // risk that modifiableMapped is in the hash since it will
>+        // always have come from GetModifiableMapped, which never
>+        // returns maps that are in the hash (such hashes are by nature
>+        // not modifiable).

I'd rewrite this to say:
        // Reset the stylesheet of modifiableMapped so that it doesn't
        // spend time trying to remove itself from the hash.  There is no
        // risk that modifiableMapped is in the hash since we created
        // it ourselves and it didn't come from the stylesheet (in which
        // case it would not have been modifiable).

nsHTMLTableElement.cpp:

You should also move the calls to
  nsGenericHTMLElement::MapCommonAttributesInto(aAttributes, aData);
  nsGenericHTMLElement::MapBackgroundAttributesInto(aAttributes, aData)
from inside the
  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Visibility))
  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Background))
tests to the end of the function (like it is for the other
MapAttributesIntoRule functions).  This will mean that tables will
support the 'contenteditable' attribute like all other HTML elements.
(And mean that if people add new things to those functions, like
contenteditable was added, table will pick them up.) It might be worth
adding a test for contenteditable.






Do you have tests that the cellpadding attribute on a table still sets
the 'padding' CSS property on a TD even when the TD has a 'display'
property other than 'table-cell'?  (And also perhaps what happens to the
various styles set if a TABLE is styled as 'display:table-cell'?)
That's the thing I know that this is changing.  (Do you know of other
things?  There are a lot of tests there...)


Other than that, this looks good, but marking review- because of the
first issue, for which I'd like to look at the revised patch.

Sorry for taking so long to get to this.

Comment 9

[Bernd]

Attachment: revised patch

The patch makes the changes that David did ask for, however the test case writing is not done yet although I found already bug 586400 when doing it.

I am insecure about the changes in SetAttr and UnsetAttr

The plan for the testcase writing is

tests should cover html quirks, strict mode, xhtml, html5

cellpadding  having it on the table
adding cellpadding to the table, modifing an existing cellpadding, removing a  cellpadding
inter action will padding on the cell
sparse tables without tbody and row
restyled table elements (display block for instance)
for the properties where the display check got removed checking that it works under normal conditions and with modified display types.

Comment 10

[Bernd]

Attachment: test cases

Comment 11

[Bernd]

Attachment: revised patch

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 470263 [details] [diff] [review]
revised patch

> nsHTMLTableCellElement::nsHTMLTableCellElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>   : nsGenericHTMLElement(aNodeInfo)
>+
> {
> }

Please don't introduce this newline.  (I'm not crazy about the other
new ones above it, but they're ok either way.)

>+  if (node  && node->IsHTML() && node->NodeInfo()->Equals(nsGkAtoms::table)) {

Could you change the two spaces after the first "node" to one space?


>+    nsMappedAttributes* tableInheritedAttributes;
>+    tableInheritedAttributes = table->GetAttributesMappedForCell();

Could you write this as a single statement with a break after the =, like:

+    nsMappedAttributes* tableInheritedAttributes =
+      table->GetAttributesMappedForCell();


Also, please don't add the new blank lines at the end of the file.

nsHTMLTableElement.cpp:

>-#include "nsIDOMHTMLTableElement.h"
>+
> #include "nsIDOMHTMLTableCaptionElem.h"
> #include "nsIDOMHTMLTableSectionElem.h"
>+#include "nsHTMLTableElement.h"

It's best to put the include of nsHTMLTableElement.h first.  Including
the .h associated with a specific .cpp first in that .cpp ensures that
that .h file compiles without any specific headers included before it.


In nsHTMLTableElement::BuildInheritedAttributes, there's a problem that
you null-check |sheet|, and fail to set mTableInheritedAttributes when
it's null.  This would lead to returning a garbage pointer (0x1).  I
think the safest fix is to change
nsHTMLTableElement::GetAttributesMappedForCell to check 
  if (mTableInheritedAttributes != TABLE_ATTRS_DIRTY)
before returning mTableInheritedAttributes.

In nsHTMLTableElement::SetAttr and UnsetAttr, could you call the
variable isCellPadding rather than hasCellPadding?

In nsHTMLTableElement, you also need to override BindFromTree and
UnbindFromTree.  This is important since the mapped attributes have a
pointer to the document's attribute style sheet, so if you change
documents you need to clear that pointer.  These overrides should simply:
 * call ReleaseInheritedAttributes
 * call the base class method.

nsHTMLTableElement.h:

+// Sentinel value of 0x1 indicates that this is dirty and needs to be

Could you:
 * write TABLE_ATTRS_DIRTY instead of 0x1
 * indent this comment 2 spaces


r=dbaron with those things fixed

Thanks for fixing this old bug.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 470261 [details] [diff] [review]
test cases

I'm just going to rubber-stamp these tests unless there's something in particular you wanted me to look at.

Comment 14

[Bernd]

Attachment: testcases for checkin

Comment 15

[Bernd]

Attachment: patch for checkin

this addresses the review comments

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 475408 [details] [diff] [review]
patch for checkin

>+  if (node && node->IsHTML() && node->NodeInfo()->Equals(nsGkAtoms::table)) {

For future reference, node->IsHTML(nsGkAtoms::table) is preferred.

Comment 17

[Bernd]

I landed http://hg.mozilla.org/integration/mozilla-inbound/rev/4738b38a2f3c on inbound and will handle comment 16 in a followup.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/4738b38a2f3c

Comment 19

[Bernd]

reopening, the shared leaf frame is not fixed, I renamed the bug title to reflect this.

Comment 20

[Bernd]

the shared leaf frame has long gone (2004) see bug 236873, so this is indeed fixed.