277434 - Setting .type property on <object> element doesn't create an HTML attribute type

Status: RESOLVED FIXED

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

Description

[Martin Honnen]

When trying to set
  var object = document.createElement('object');
  object.type = 'image/x-special';
then the created object element doesn't have a type attribute, that is
  object.getAttribute('type')
yields 
  null
This happens with 1.8a trunk builds (tested with Mozilla 1.8a6 (Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20050106) as well as with
Firefox 1.0 (tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7.5) Gecko/20041107 Firefox/1.0) but not with the Mozilla 1.7.5 release.

As for the W3C DOM specification about that property type of HTMLObjectElement at
<http://www.w3.org/TR/2003/REC-DOM-Level-2-HTML-20030109/html.html#ID-91665621>
it doesn't in any way suggest that setting the property should not change the
HTML attribute thus it seems a bug that the attribute is not set when the
property is set.

I will upload a test case that contains a static <object> and then creates two
<object> elements with script, trying to set .type on the first while calling
.setAttribute('type', ...) on the second. The script then examines all <object>
elements in the page and outputs the result of ['type'] and
.getAttribute('type'). The result with Mozilla which I think is a bug is

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
image/x-special; ["type"]: image/x-special

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
null; ["type"]: image/x-special

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
image/x-special; ["type"]: image/x-special

the correct result should be 

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
image/x-special; ["type"]: image/x-special

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
image/x-special; ["type"]: image/x-special

Checking [object HTMLObjectElement]; .tagName: OBJECT; .getAttribute("type"):
image/x-special; ["type"]: image/x-special

meaning it shouldn't matter whether the object and its type are set statically
in HTML or with setting .type or with setting .setAttribute('type', ...), all
cases getAttribute('type') and the property access .type should give the same
result.

Comment 1

[Martin Honnen]

Attachment: test case, see description in bug report

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is an aviary branch regression (plugin finder).  See revision 1.73 of
nsHTMLObjectElement.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that <embed> has the same problem, by the way....

jst, is there a reason we're hijacking DOM methods for internal use and changing
their behavior in the process?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why was that patch even needed? mType seems compleatly unused. In both classes.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And the patch is by none other then jst! What were you smoking? ;-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

mType is used by SetType().

The problem is that our the frame calls SetType() on the content node and then
wants to use GetType() to get that type back.  But it doesn't want to change the
DOM (obviosuly).  So SetType got hacked to not change the attr value....

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If it desperatly needs to do this then it should use some internal interface
rather then abusing the nsIDOM ones. Maybe time to revive my
nsHTMLSharedObjectElement class...

Comment 8

[David Baron :dbaron:]

Too late for 1.8b1, plussing for 1.8b2.

Comment 9

[Asa Dotzler [:asa]]

JST, are you gonna be able to get to this pretty quickly or should we try to get
it done in 1.8b3?

Comment 10

[Asa Dotzler [:asa]]

moving out to blocking 1.8b3

Comment 11

[Johnny Stenback (:jst)]

Maybe it's just me, but it seems like this is more what we'd want than having
setting object.type set the type attribute. But I bet IE works exactly opposite
to that, so maybe we just need to revert to what we had...

The reason for the change is that we need to expose the type of the data in the
case where it's either not specified in the HTML or where what we get from the
server doesn't match what's in the HTML, and in that case it surely does not
make sense to go change the HTML type attribute... Maybe now is the time for a
'realType' property that doesn't map directly to the attribute value?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Having a new property that has the server-sent type sounds great to me.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we need to expose this in the DOM at all, if so .realType sounds good?
Otherwise we could just use nsIContent::SetProperty

Comment 14

[Johnny Stenback (:jst)]

The PFS widget needs to know the real type, so we need a new exposed property.

Comment 15

[Boris Zbarsky [:bzbarsky]]

If biesi's creating an nsIImageLoadingContent-like interface for objects, we
should just expose it there.  Note that this interface need not have classinfo
or anything if all we're using it for is PFS.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

that will of course only land for 1.9, so if people want a solution for 1.8
something else is needed in the meantime...

Comment 17

[Boris Zbarsky [:bzbarsky]]

No reason the interface, or at least the realType part of it, couldn't land for 1.8.

Comment 18

[Johnny Stenback (:jst)]

How about we make .type expose what it exposes today, but setting it also always
set the HTML attribute? For that we'd still need a new interface to tell the
element about the actual type, but it wouldn't need to be scriptable since the
caller that tells the element about the actual type is nsObjectFrame.cpp. That'd
be the lowest impact fix for this I believe...

Comment 19

[Johnny Stenback (:jst)]

Attachment: Make setting .type change the type attr

This implements what I talked about in my previous comment.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Isn't this fixing just half of the problem? Getting .type will never get the
attribute value and just be an empty string in most cases.

I'm not entierly sure what we're trying to do here. If the plugin code just
needs to store a type in the contentnode then IMHO nsIContent::SetProperty is a
good solution.

Or do we want to make this behave like for example myImg.width where the
property always returns the 'real' value which only sometimes is the same as the
attribute? If so, should setting .type really null out mType? And we should
probably then return the attribute value of mType isn't set in GetType.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187457 [details] [diff] [review]
Make setting .type change the type attr

why not use nsACString& here, given that both caller and callee would prefer
that?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 187457 [details] [diff] [review]
Make setting .type change the type attr

r=me

it's been suggested to use an nsACString for mType and SetActualType which
sounds fine to me.

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 187457 [details] [diff] [review]
Make setting .type change the type attr

Yeah, switch to nsACString, also maybe rename mType to mActualType.

Comment 24

[Johnny Stenback (:jst)]

Attachment: Final patch that was checked in.

Comment 25

[Johnny Stenback (:jst)]

Fixed checked in.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Doesn't this still violate the DOM HTML spec?  Per that, the .type property
should reflect the type attribute on getting no matter what.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Reopening, since we're still not following DOM spec here.

Comment 28

[Chris Beard]

Unless we hear a compelling argument for blocking, let's push this out to 1.8b4.

Comment 29

[Asa Dotzler [:asa]]

would consider a non-risky patch if we can get one in time for b4 but not going
to block on this.

Comment 30

[Johnny Stenback (:jst)]

Attachment: Expose .actualType through nsIPluginElement

This restores .type to map directly to the attribute and introduces a new
.actualType property that our chrome (or content too for that matter) can QI to
to get to the actual type. This way we won't pollute the plugin element
namespace except in the case when we're dealing with missing plugins.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193116 [details] [diff] [review]
Expose .actualType through nsIPluginElement

>Index: content/html/content/src/nsHTMLObjectElement.cpp

> nsHTMLObjectElement::SetActualType(const nsACString& aActualType)
> {
>   mActualType = aActualType;

This lets pages set the actual type on a broken plugin, right?	Is that
desirable?  Do we need a security check here?  Or perhaps we should make
setActualType noscript and leave actualType readonly scriptable?

> //NS_IMPL_STRING_ATTR(nsHTMLObjectElement, Type, type)

> nsHTMLObjectElement::GetType(nsAString& aType)

Why not just remove the manual SetType/GetType impls and uncomment the
NS_IMPL_STRING_ATTR?

>Index: content/html/content/src/nsHTMLSharedElement.cpp

> nsHTMLSharedElement::GetType(nsAString& aType)
> {
>-  if (!mNodeInfo->Equals(nsHTMLAtoms::embed) || mActualType.IsEmpty()) {
>+  if (!mNodeInfo->Equals(nsHTMLAtoms::embed)) {
>     GetAttr(kNameSpaceID_None, nsHTMLAtoms::type, aType);
>   } else {
>-    CopyUTF8toUTF16(mActualType, aType);
>+    aType.Truncate();

This looks like it will always return the empty string for the one case when
type matters -- <embed>.  Again, why not remove the GetType/SetType here and
just uncomment the NS_IMPL_STRING_ATTR?

r+sr=bzbarsky with those fixed.

Comment 32

[Johnny Stenback (:jst)]

Attachment: Fix that was checked in.

Changed all that and checked this patch in.

Comment 33

[Asa Dotzler [:asa]]

is this fixed for the branch? If so, can you please add the fixed1.8 keyword?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Yeah, this is all fixed

Comment 35

[Steffen Wilberg]

Removing aviary-landing keyword.

Comment 36

[Boris Zbarsky [:bzbarsky]]

It looks like the fix for this was actually wrong.  :(  In particular, it didn't switch the other consumer that was calling SetType() to using actualType.  See bug 430424.