Open Bug 684221 Opened 11 years ago Updated 4 years ago

Test and fix HTMLTableCellElement attributes reflection

Categories

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

defect

Tracking

()

People

(Reporter: mounir, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 673820 do not pass try because some tests are failing because of a bad implementation of the attributes reflection.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #557828 - Flags: review?(Olli.Pettay)
Attachment #557828 - Flags: feedback?(Ms2ger)
Sent to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=77915d8cac69
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [needs review]
Comment on attachment 557828 [details] [diff] [review]
Patch v1

colspan and rowspan aren't actually supposed to be handled the same.
Attachment #557828 - Flags: feedback?(Ms2ger) → feedback-
Attached patch Patch v1.1Splinter Review
Thanks Ms2ger, I misread the specs. This should fix it.
Attachment #557828 - Attachment is obsolete: true
Attachment #557828 - Flags: review?(Olli.Pettay)
Attachment #558193 - Flags: review?(Olli.Pettay)
Attachment #558193 - Flags: feedback?(Ms2ger)
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.
Attachment #558193 - Flags: feedback?(Ms2ger) → feedback+
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?
Attachment #558193 - Flags: review?(Olli.Pettay) → review-
I will unlikely work on this any time soon so I am unassigning myself. Feel free to take it over.
Assignee: mounir → nobody
Whiteboard: [needs review]
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.
Priority: -- → P5
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.