133654 - PARAM value attribute not recognized in XHTML document

Status: VERIFIED FIXED

(Core :: XML, defect, P2)

Description

[Chris Petersen]

Build: 2002-03-26-08
Platform: All
Expected Results: Param element's value attribute should be displayed by applet
element. Applet should appear with a blue background with scrolling phrase
displaying "scrolling banner text".

What I got: Param value attribute isn't passing this information to the applet.
Applet appears with a white background and scrolling phrase stating "Message not
Found".


Steps to reproduce:

1) First, go to HTML version of the document:

http://mozilla.org/quality/browser/standards/html/applet_align_top.html

2)  Applet should appear with a blue background with scrolling phrase displaying
"scrolling banner text".

3) Go to the xhtml version:
http://mozilla.org/quality/browser/standards/xhtml/transitional/applet_align_top.xml

4) Applet appears with a white background and scrolling phrase stating "Message
not Found".

Comment 1

[Chris Petersen]

The applet in the xhtml document is rendered correctly in NS 6.2.1.

Comment 2

[Judson Valeski]

minusing for now. please re-nominate if we've miss-perceived the frequency and
end user impact.

Comment 3

[Chris Petersen]

Removed minus from nsbeta1. Needs to be resolved. This is currently working in
NS 6.2.3.

Comment 4

[Johnny Stenback (:jst)]

Um, this is not working with todays trunk build, nor can I see how it ever
worked in any recent builds (i.e. I really doubt it works on the mozilla1.0
branch either, but I don't have a build handy right now where I could test).

Untested fix coming up.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Proposed fix, don't look for "PARAM" elements in XHTML, take the namespace into account. Tons for cleanup too...

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 84464 [details] [diff] [review]
Proposed fix, don't look for "PARAM" elements in XHTML, take the namespace into account. Tons for cleanup too...

>Index: layout/html/base/src/nsObjectFrame.cpp
>===================================================================
>-  mydomElement->GetElementsByTagName(NS_LITERAL_STRING("PARAM"), getter_AddRefs(allParams));

Doh! I think I need to search LXR for all use of all DOM methods and see if
they
should be fixed to do what you did here...

>-              if (parent.get() == mydomNode.get()) {
>+            if (parent == mydomElement) {

This is the only part that worries me. In the case of interface pointers,
shouldn't we compare the same interface pointers (the old code was
comparing nsIDOMNodes, your patch compares nsIDOMNode to nsIDOMElement)?

Comment 7

[Rakesh Mishra]

*** Bug 141525 has been marked as a duplicate of this bug. ***

Comment 8

[Christine Wozniak]

Adding custrtm- to comment fields.  No impact on customization.

Comment 9

[Johnny Stenback (:jst)]

Oh, hmm, I wasn't cc:d here so I didn't see that question earlier... In theory,
yeah, we should be comparing interface pointers of the same type, that's the
right thing to do, and I'm fine with you putting that back when checking in. But
my patch will do the right thing in mozilla since no DOM element implementations
use a different interface pointer for nsIDOMElement and nsIDOMNode. And since
speed doesn't really matter here, we might as well do the right thing and be
safe in the extremely unlikely event that someone would change the interface
inheritance model for HTML param elements...

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fixes my issue

Ok, these are the only changes to jst's patch.

+	     nsCOMPtr<nsIDOMNode> mydomNode = do_QueryInterface(mydomElement);
+	     NS_ENSURE_TRUE(mydomNode, NS_ERROR_NO_INTERFACE);
!	     if (parent.get() == mydomNode.get()) {

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 88518 [details] [diff] [review]
Fixes my issue

r=heikki

Comment 12

[Jaime Rodriguez, Jr.]

removing adt1.0.1 until this has sr=, and has been verified on the trunk.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 88518 [details] [diff] [review]
Fixes my issue

r=peterv. Changing heikki's r to an sr.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 88518 [details] [diff] [review]
Fixes my issue

>+            nsCOMPtr<nsIDOMNode> mydomNode = do_QueryInterface(mydomElement);
>+            NS_ENSURE_TRUE(mydomNode, NS_ERROR_NO_INTERFACE);
>+            if (parent.get() == mydomNode.get()) {

Please loose those .get()'s when checking this in, and the NS_ENSURE_TRUE
should be replaced with an NS_ASSERTION (if anything at all), that really
should never happen, but even if it does the code would do the right thing even
w/o the NS_ENSURE_TRUE(), we'll save a few instructions... sr=jst for this part
of the change.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on trunk with last of jst's nits, will attach as well.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: final

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 88672 [details] [diff] [review]
final

Carrying reviews forward.

Comment 18

[Jaime Rodriguez, Jr.]

Resolving as fixed per Comment #15 From Heikki Toivonen.

chris - can you pls verify this on the trunk? thanks!

Comment 19

[Chris Petersen]

Excellent.... Verified on the June 24th trunk builds:

OS X (2002-06-24-08)
Linux (2002-06-24-08)
Windows ME (2002-06-24-08)

Comment 20

[Chris Petersen]

Any reason why this isn't checked into the branch yet ? I would like to see this
get resolved soon.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This has not been checked in to the branch because it does not have the needed
approvals. Cc members of ADT & drivers in the hopes that we'd get them...

Comment 22

[Chris Petersen]

It would nice to get approval since this is considered a regression. Works
properly in NS 6.2.

Comment 23

[John Morrison]

> Cc members of ADT & drivers in the hopes that we'd get them...

Heh. 'jrgm' is neither ADT nor driver. Maybe you were looking for 'jaimejr' :-)

Comment 24

[Jaime Rodriguez, Jr.]

resolving as verified per Comment #19 From Chris Petersen.

Comment 25

[Jaime Rodriguez, Jr.]

This had not been approved because it had not been resolved/verified. Marked it
as so, and will pursue getting the approvals. thanks!

Comment 26

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Shh! You exposed my secret plot. You could have pretented (I really need spell
checker for text areas, my mind is gone...) to be Jaime and given approval ;)

Comment 27

[Håkan Waara]

*** Bug 141395 has been marked as a duplicate of this bug. ***

Comment 28

[Jaime Rodriguez, Jr.]

adt1.0.1- per the ADT. let's get it in the next release.