684221 - Test and fix HTMLTableCellElement attributes reflection

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Mounir Lamouri (:mounir)]

Bug 673820 do not pass try because some tests are failing because of a bad implementation of the attributes reflection.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

[Mounir Lamouri (:mounir)]

Sent to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=77915d8cac69

Comment 3

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

Comment on attachment 557828 [details] [diff] [review]
Patch v1

colspan and rowspan aren't actually supposed to be handled the same.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Thanks Ms2ger, I misread the specs. This should fix it.

Comment 5

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

Comment on attachment 558193 [details] [diff] [review]
Patch v1.1

>--- a/content/html/content/src/nsHTMLTableCellElement.cpp
>+++ b/content/html/content/src/nsHTMLTableCellElement.cpp
>+NS_IMPL_UINT_ATTR_NON_ZERO(nsHTMLTableCellElement, ColSpan, colspan)
>+NS_IMPL_UINT_ATTR_DEFAULT_VALUE(nsHTMLTableCellElement, RowSpan, rowspan, 1)
>     if (aAttribute == nsGkAtoms::colspan) {
>+      return aResult.ParsePositiveIntValue(aValue);
>     if (aAttribute == nsGkAtoms::rowspan) {
>+      return aResult.ParseNonNegativeIntValue(aValue);

Looks correct per spec, but you'll need to add the clamping to nsTableCellFrame::Get{Col,Row}Span.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 558193 [details] [diff] [review]
Patch v1.1

>     if (aAttribute == nsGkAtoms::colspan) {
>-      PRBool res = aResult.ParseIntWithBounds(aValue, -1);
>-      if (res) {
>-        PRInt32 val = aResult.GetIntegerValue();
>-        // reset large colspan values as IE and opera do
>-        // quirks mode does not honor the special html 4 value of 0
>-        if (val > MAX_COLSPAN || val < 0 ||
>-            (0 == val && InNavQuirksMode(GetOwnerDoc()))) {
>-          aResult.SetTo(1);
>-        }
>-      }
>-      return res;
>+      return aResult.ParsePositiveIntValue(aValue);
Why is this ok?


>     }
>     if (aAttribute == nsGkAtoms::rowspan) {
>-      PRBool res = aResult.ParseIntWithBounds(aValue, -1, MAX_ROWSPAN);
>-      if (res) {
>-        PRInt32 val = aResult.GetIntegerValue();
>-        // quirks mode does not honor the special html 4 value of 0
>-        if (val < 0 || (0 == val && InNavQuirksMode(GetOwnerDoc()))) {
>-          aResult.SetTo(1);
>-        }
>-      }
>-      return res;
>+      return aResult.ParseNonNegativeIntValue(aValue);
And this?
Can layout really handle huge values?

Doesn't this regress bug 141818, Bug 27993 and Bug 30332?

What does other browsers do?

Comment 7

[Mounir Lamouri (:mounir)]

I will unlikely work on this any time soon so I am unassigning myself. Feel free to take it over.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.