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

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Martin Honnen, Assigned: jst)

Tracking

({fixed1.8, regression})

Trunk
fixed1.8, regression
Points:
---
Bug Flags:
blocking1.8b3 -
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 170565 [details]
test case, see description in bug report
This is an aviary branch regression (plugin finder).  See revision 1.73 of
nsHTMLObjectElement.

Assignee: general → jst
Flags: blocking1.8b?
Keywords: aviary-landing
Summary: Setting .type property on dynamically create <object> element doesn't create an HTML attribute type → Setting .type property on <object> element doesn't create an HTML attribute type
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?
OS: Windows XP → All
Hardware: PC → All
Why was that patch even needed? mType seems compleatly unused. In both classes.
And the patch is by none other then jst! What were you smoking? ;-)
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....
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...
Too late for 1.8b1, plussing for 1.8b2.
Flags: blocking1.8b?
Flags: blocking1.8b2+
Flags: blocking1.8b-

Comment 9

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

13 years ago
moving out to blocking 1.8b3
Flags: blocking1.8b3+
Flags: blocking1.8b2+
Flags: blocking1.8b-
(Assignee)

Comment 11

13 years ago
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?
Having a new property that has the server-sent type sounds great to me.
Do we need to expose this in the DOM at all, if so .realType sounds good?
Otherwise we could just use nsIContent::SetProperty
(Assignee)

Comment 14

13 years ago
The PFS widget needs to know the real type, so we need a new exposed property.
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.
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...
No reason the interface, or at least the realType part of it, couldn't land for 1.8.
(Assignee)

Comment 18

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

Comment 19

13 years ago
Created attachment 187457 [details] [diff] [review]
Make setting .type change the type attr

This implements what I talked about in my previous comment.
(Assignee)

Updated

13 years ago
Attachment #187457 - Flags: superreview?(peterv)
Attachment #187457 - Flags: review?(bugmail)
(Assignee)

Updated

13 years ago
Whiteboard: needs review

Updated

13 years ago
Flags: blocking-aviary1.1+
Whiteboard: needs review → [cb] ready for review? r=bugmail? sr=peterv?
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 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 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.
Attachment #187457 - Flags: review?(bugmail) → review+
Comment on attachment 187457 [details] [diff] [review]
Make setting .type change the type attr

Yeah, switch to nsACString, also maybe rename mType to mActualType.
Attachment #187457 - Flags: superreview?(peterv) → superreview+
(Assignee)

Updated

13 years ago
Attachment #187457 - Flags: approval1.8b3?

Updated

13 years ago
Attachment #187457 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 24

13 years ago
Created attachment 187735 [details] [diff] [review]
Final patch that was checked in.
(Assignee)

Comment 25

13 years ago
Fixed checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Doesn't this still violate the DOM HTML spec?  Per that, the .type property
should reflect the type attribute on getting no matter what.
Reopening, since we're still not following DOM spec here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 28

13 years ago
Unless we hear a compelling argument for blocking, let's push this out to 1.8b4.
Flags: blocking1.8b4+
Flags: blocking1.8b3-
Flags: blocking1.8b3+
Whiteboard: [cb] ready for review? r=bugmail? sr=peterv?

Comment 29

13 years ago
would consider a non-risky patch if we can get one in time for b4 but not going
to block on this.
Flags: blocking1.8b4+ → blocking1.8b4-
(Assignee)

Comment 30

13 years ago
Created attachment 193116 [details] [diff] [review]
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.
Attachment #193116 - Flags: superreview?(bzbarsky)
Attachment #193116 - Flags: review?(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.
Attachment #193116 - Flags: superreview?(bzbarsky)
Attachment #193116 - Flags: superreview+
Attachment #193116 - Flags: review?(bzbarsky)
Attachment #193116 - Flags: review+
(Assignee)

Comment 32

13 years ago
Created attachment 193510 [details] [diff] [review]
Fix that was checked in.

Changed all that and checked this patch in.
(Assignee)

Updated

13 years ago
Attachment #193510 - Flags: superreview+
Attachment #193510 - Flags: review+
Attachment #193510 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #193510 - Flags: approval1.8b4? → approval1.8b4+

Updated

13 years ago
Flags: blocking-aviary1.5+

Comment 33

13 years ago
is this fixed for the branch? If so, can you please add the fixed1.8 keyword?
Yeah, this is all fixed
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Comment 35

12 years ago
Removing aviary-landing keyword.
Keywords: aviary-landing
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.

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.