Closed Bug 211636 Opened 18 years ago Closed 9 years ago

nsHTMLTableElement MapAttributesIntoRule is broken


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: dbaron, Assigned: bernd_mozilla)




(4 files, 3 obsolete files)

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.)
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.
(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.)
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...)
Blocks: 68061
Blocks: 175190
Assignee: dbaron → nobody
QA Contact: ian → style-system
Blocks: 391136
Blocks: 484825
Blocks: 186317
Blocks: 260406
> 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?
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #424482 - Attachment is obsolete: true
Attachment #424508 - Flags: review?
Attachment #424508 - Flags: review? → review?(dbaron)
Comment on attachment 424508 [details] [diff] [review]


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

>+  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,

>+        // 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).


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.
Attachment #424508 - Flags: review?(dbaron) → review-
Attached patch revised patch (obsolete) — Splinter Review
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.
Attachment #424508 - Attachment is obsolete: true
Attached patch test casesSplinter Review
Attached patch revised patchSplinter Review
Attachment #465130 - Attachment is obsolete: true
Attachment #470261 - Flags: review?(dbaron)
Attachment #470263 - Flags: review?(dbaron)
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.


>-#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.


+// 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.
Attachment #470263 - Flags: review?(dbaron) → review+
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.
Attachment #470261 - Flags: review?(dbaron) → review+
this addresses the review comments
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.
I landed on inbound and will handle comment 16 in a followup.
Whiteboard: [inbound]
Assignee: nobody → bernd_mozilla
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
reopening, the shared leaf frame is not fixed, I renamed the bug title to reflect this.
Assignee: bernd_mozilla → nobody
Resolution: FIXED → ---
Summary: nsHTML{Table,SharedLeaf}Element's MapAttributesIntoRule are broken → nsHTMLSharedLeafElement MapAttributesIntoRule is broken
the shared leaf frame has long gone (2004) see bug 236873, so this is indeed fixed.
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Summary: nsHTMLSharedLeafElement MapAttributesIntoRule is broken → nsHTMLTableElement MapAttributesIntoRule is broken
Assignee: nobody → bernd_mozilla
Blocks: 682460
No longer blocks: 682460
Depends on: 682460
Depends on: 689204
Blocks: 702043
Depends on: 715271
You need to log in before you can comment on or make changes to this bug.