Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsHTMLTableElement MapAttributesIntoRule is broken

RESOLVED FIXED in mozilla9



CSS Parsing and Computation
14 years ago
6 years ago


(Reporter: dbaron, Assigned: Bernd)


Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(4 attachments, 3 obsolete attachments)



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

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

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


14 years ago
Blocks: 68061


11 years ago
Blocks: 175190


10 years ago
Assignee: dbaron → nobody
QA Contact: ian → style-system
Blocks: 391136


8 years ago
Blocks: 484825


8 years ago
Blocks: 114100


8 years ago
Blocks: 186317


8 years ago
Blocks: 260406

Comment 4

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

8 years ago
Not in the tree, but there's an example in my patch for bug 915:

Comment 6

8 years ago
Created attachment 424482 [details] [diff] [review]

Comment 7

8 years ago
Created attachment 424508 [details] [diff] [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?


8 years ago
Attachment #424508 - Flags: review? → review?(dbaron)

Comment 8

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

Comment 9

7 years ago
Created attachment 465130 [details] [diff] [review]
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.
Attachment #424508 - Attachment is obsolete: true

Comment 10

7 years ago
Created attachment 470261 [details] [diff] [review]
test cases

Comment 11

7 years ago
Created attachment 470263 [details] [diff] [review]
revised patch
Attachment #465130 - Attachment is obsolete: true


7 years ago
Attachment #470261 - Flags: review?(dbaron)


7 years ago
Attachment #470263 - Flags: review?(dbaron)

Comment 12

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

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

Comment 14

7 years ago
Created attachment 475407 [details] [diff] [review]
testcases for checkin

Comment 15

7 years ago
Created attachment 475408 [details] [diff] [review]
patch for checkin

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.

Comment 17

6 years ago
I landed on inbound and will handle comment 16 in a followup.
Whiteboard: [inbound]
Assignee: nobody → bernd_mozilla
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Comment 19

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

Comment 20

6 years ago
the shared leaf frame has long gone (2004) see bug 236873, so this is indeed fixed.
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Summary: nsHTMLSharedLeafElement MapAttributesIntoRule is broken → nsHTMLTableElement MapAttributesIntoRule is broken


6 years ago
Assignee: nobody → bernd_mozilla


6 years ago
Blocks: 682460


6 years ago
No longer blocks: 682460
Depends on: 682460
Depends on: 689204
Blocks: 702043


6 years ago
Depends on: 715271
You need to log in before you can comment on or make changes to this bug.