1314026 - colspan and rowspan setters should not stealth-cast unsigned ints to ints

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The call to SetHTMLIntAttr casts to int.  So if you do:

  td.colSpan = 2147483648;

you get "-2147483648" as the attribute value.  This is quite odd.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a SetHTMLUIntAttr function that does the right thing with setters for unsigned long IDL attributes that reflect a content attribute

Tests will appear at https://github.com/w3c/web-platform-tests/pull/4115 in a bit

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805954 [details] [diff] [review]
Add a SetHTMLUIntAttr function that does the right thing with setters for unsigned long IDL attributes that reflect a content attribute

Review of attachment 8805954 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsGenericHTMLElement.h
@@ +979,5 @@
>  
>      SetHTMLAttr(aName, value, aError);
>    }
>  
> +  void SetHTMLUIntAttr(nsIAtom* aName, uint32_t aValue, mozilla::ErrorResult& aError)

This is fine, but we could remove some duplication if we use a template:

template <typename T>
void SetHTMLIntAttr(nsIAtom* aName, T aValue, mozilla::ErrorResult& aError)
{
  nsAutoString value;
  value.AppendInt(aValue);
  SetHTMLAttr(aName, value, aError);
}

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's true.  I wasn't quite sure whether it was better to do that or to make the caller's desire for int/uint be explicit....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Thoughts on the implicit vs explicit thing?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> Thoughts on the implicit vs explicit thing?

I don't feel super strong either way.  I think since we have AppendInt() that works for both signed and unsigned in our string API that there is precedent for not including the full type in the method name.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  Thought about it some more, and calling the int overload with an unsigned value that doesn't fit in int makes no sense, and calling the uint overload with a negative-valued int would also be nonsense.  So I went ahead with the template approach.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/933e7a735a80
Add a version of SetHTMLIntAttr that does the right thing with an unsigned argument, for use from setters for unsigned long IDL attributes that reflect a content attribute.  r=bkelly

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/933e7a735a80