136532 - unnamed form has '[object HTMLSelectElement]' target

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Adam Hauner]

2002040903/Win2K

Repro:
1. open http://www.index.hu/ 
2. open Page Info | Forms
3. select first line with form action http://index.hu/kereso

Actual:
Target for this form is [object HTMLSelectElement]

Expected:
None ...

Note: Form hasn't method.

Comment 1

[Daniel Brooks [:db48x]]

/me sighs

probably related to that other bug, I'll try the same workaround

Comment 2

[Daniel Brooks [:db48x]]

Attachment: patch

just needs r=

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 79069 [details] [diff] [review]
patch

How does this deal with forms which got .target set from a script?

I think what you want to do is to get .target, and then if that's of type
"object" you want to use getAttribute...

Comment 4

[Daniel Brooks [:db48x]]

so you're saying that if some js on the page changes theform.target, when page
info does theform.getAttribute("target") it won't get the same thing the js code
assigned to it? I find that hard to believe, because that would suck.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's correct.  getAttribute gets the contents of the attribute, which need not
be the same as the property value.

On the bright side, if the page sets ,target, then you get .target you better
get what the page set.  :)

Comment 6

[Adam Hauner]

WFM on original page with 2002071508/trunk/W2K, maybe mark-up changed.

Comment 7

[Daniel Brooks [:db48x]]

I can no longer reproduce either. 
http://db48x.dyndns.org/pageInfo/tests/136532.html

Comment 8

[Adam Hauner]

Original repro is showing problem again using 2002111208/trunk/W2K
-> REOPEN

Comment 9

[Jonathan Wakely]

I see this bug on Linux with Mozilla 1.3.

I was about to file a similar bug, along with a brief analysis of it, when i
found this and bug 160329 which are similar enough that I thought I'd add a
comment here before filing a new one.

Apologies if the following is already known, it isn't clear from the comments ...

http://www.index.hu/ contains something like the following markup:

    <form action="...">
      <select name="target">
    </form>

Since the <form> has no explicit target, but the <select> has name "target", the
<form>'s target gets set to the name of the type of the <select> (somehow)

The problem can also be seen on the title attributes of <img> elements when an
element has name="title" or id="title". I have a page that demonstrates this at
http://www.compsoc.man.ac.uk/~cow/mozilla/id-attr_bug.html
However, the problem with images can only be seen on the Element Properties 
dialog, and _not_ on the Page Info dialog (unlike the form target of this bug).

The general form seems to be that if "attr" in the following code matches the
name of a missing attribute of <form>, <node1> or <node2> then the attribute
value is "[object HTMLNodeElement]"

<form>
  <node1 id="attr"/>
  <node2/>
</form>

But only for some combinations of elements and attributes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This would be fixed by just using the right getter.  Caillon, weren't you gonna
do this?

Comment 11

[Jonathan Wakely]

Should the img.title bug I described be filed separately, if it involves using
the right getter in a different place?

Comment 12

[Boris Zbarsky [:bzbarsky]]

No.  All of page info should just get converted at once.

Comment 13

[David G King]

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

Comment 14

[David G King]

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

Comment 15

[David G King]

FYI, the attached patch should now refer to
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/pageInfo.js#612

Comment 16

[David G King]

(In reply to comment #3)
> (From update of attachment 79069 [details] [diff] [review])
> I think what you want to do is to get .target, and then if that's of type
> "object" you want to use getAttribute...

After some quick hacking, I find that form.target and
form.getAttribute("target") isn't set when using View-PageInfo from the menu.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ignore the .getAttribute stuff I said earlier.  There's a better way now; see
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/XPCNativeWrapper.js

Comment 18

[Erik Fabert]

So basically, it's 

    var formWrapper = new XPCNativeWrapper(form,
      "name", "elements", "encoding", "target", "getElementsByTagName()");

and then use

    if (formWrapper.name)
      ft = theBundle.getFormattedString("formTitle", [formWrapper.name]);

instead of

    if (form.name)
      ft = theBundle.getFormattedString("formTitle", [form.name]);

? Ditto for encoding, target, etc, of course? Nice.

Comment 19

[Erik Fabert]

Attachment: Testcase

This testcase shows the problems Page Info has with overwritten form
properties. It also exposes the same problem in the privacy tab.

Comment 20

[Erik Fabert]

Attachment: Use XPCNativeWrapper to access form properties.

Fixes the bug with www.index.hu and the testcase. Also updates getAbsoluteURL()
and adds <input type="image"> and <button type="submit"> elements to the
"Links" tab.

Comment 21

[David G King]

The patch seems to fix the problem from one of the duped bugs. (Win32/MinGW/cygwin)

Comment 22

[Daniel Brooks [:db48x]]

Comment on attachment 155256 [details] [diff] [review]
Use XPCNativeWrapper to access form properties.

>Index: pageInfo.js
>-    if (/^image$/i.test(elem.type))
>+    switch (elem.type) // type is guaranteed to be lowercase, right?

No, it's not. Keep the case insensitive test.

> /*
>- * Takes care of XMLBase and <base>
>  * url is the possibly relative url.
>- * node is the node where the url was given (needed for XMLBase)
>- *
>- * This function is called in many places as a workaround for bug 72524
>- * Once bug 72522 is fixed this code should use the Node.baseURI attribute
>+ * node is the node where the url was given
>  *
>  * for node==null or url=="", empty string is returned
>- *
>- * This is basically just copied from http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/metadata.js,
>- * though I've modified it so that it doesn't assign to .spec
>  */
>-
>+const gioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
> function getAbsoluteURL(url, node)
> {
>-  if (!url || !node)
>+  if (!url || !node || !node.baseURI)
>     return "";
>-  var urlArr = new Array(url);
>-
>-  var doc = node.ownerDocument;
>-  if (node.nodeType == Node.ATTRIBUTE_NODE)
>-    node = node.ownerElement;
...
>+    // Shouldn't happen
>+    dump("getAbsoluteURL: url:"+url+" node: "+node+" node.baseURI:"+node.baseURI+"\n");
>   }
>-
>-  return URL.spec;
>+  return "";
> }


Don't mess with getAbsoluteURL. I'm aware that it's outdated, and I'm also
aware that in nearly all cases it's no longer needed because the properties now
return an absolute url even when the document contains a relative url. One
exception to this is <input type="image">. Just fix the bug at hand.

Comment 23

[Daniel Brooks [:db48x]]

Also, if you wanted to go through and fix every place in page info where I
access a property on a node from the document so that it uses the wrapper I
would be very appreciative. ;)

Comment 24

[Erik Fabert]

(In reply to comment #22)
> >+    switch (elem.type) // type is guaranteed to be lowercase, right?
> 
> No, it's not. Keep the case insensitive test.

But then I can't use a switch... do you have any objections to elem.type.
toLowerCase()? Do you want the <button> and <input type="image"> elements in the 
Links tab at all?

> Don't mess with getAbsoluteURL. I'm aware that it's outdated, and I'm also
> aware that in nearly all cases it's no longer needed because the properties 
now
> return an absolute url even when the document contains a relative url. One
> exception to this is <input type="image">. Just fix the bug at hand.

Ok, but this functions is almost identical to the one the Privacy tab uses and 
it seems no one has complained so far. I think this speeds up Page Info a bit.

(In reply to comment #23)
> Also, if you wanted to go through and fix every place in page info where I
> access a property on a node from the document so that it uses the wrapper I
> would be very appreciative. ;)

I think form elements are the only case where this is needed, other elements 
can't get their properties overwritten by their children. Using a wrapper for 
every access would just slow things down.

While I am at it, what are your thoughts about replacing getValueText() with 
node.textContent? The results are almost always identical, except for object 
elements.

 

Comment 25

[Daniel Brooks [:db48x]]

(In reply to comment #24)
> (In reply to comment #22)
> > >+    switch (elem.type) // type is guaranteed to be lowercase, right?
> > 
> > No, it's not. Keep the case insensitive test.
> 
> But then I can't use a switch... do you have any objections to elem.type.
> toLowerCase()? Do you want the <button> and <input type="image"> elements in the 
> Links tab at all?

Correct, you'll need to use toLowerCase and the switch. It's just something I
overlooked when I wrote this part.

> 
> > Don't mess with getAbsoluteURL. I'm aware that it's outdated, and I'm also
> > aware that in nearly all cases it's no longer needed because the properties 
> now
> > return an absolute url even when the document contains a relative url. One
> > exception to this is <input type="image">. Just fix the bug at hand.
> 
> Ok, but this functions is almost identical to the one the Privacy tab uses and 
> it seems no one has complained so far. I think this speeds up Page Info a bit.
> 

Right. My point is that in most cases we don't need to resolve the url at all,
since most html elements will do that for us. I'm about halfway through testing
all the different html elements that have urls in properties to see which ones
still need to be resolved. Then I'll go fix those properties in the dom (bz made
that very easy) and then remove the whole thing altogether.

> (In reply to comment #23)
> > Also, if you wanted to go through and fix every place in page info where I
> > access a property on a node from the document so that it uses the wrapper I
> > would be very appreciative. ;)
> 
> I think form elements are the only case where this is needed, other elements 
> can't get their properties overwritten by their children. Using a wrapper for 
> every access would just slow things down.

Correct, but a webpage can perform much the same trick if it wants. Not a huge
issue, but something I've been meaning to get around to fixing.

> 
> While I am at it, what are your thoughts about replacing getValueText() with 
> node.textContent? The results are almost always identical, except for object 
> elements.

Don't for now.

Comment 26

[Daniel Brooks [:db48x]]

Oh, and don't forget to make the same change to Firefox as well.

Comment 27

[Erik Fabert]

Attachment: Reduced patch, uses toLowerCase()

Comment 28

[Erik Fabert]

(In reply to comment #26)
> Oh, and don't forget to make the same change to Firefox as well.

I can't find XPCNativeWrapper in FF, should I add it? If yes, where?

Comment 29

[Daniel Brooks [:db48x]]

Assume for now that you'll be adding it to toolkit/content/

Comment 30

[jag (Peter Annema)]

This patch looks good to me, will do more thorough review after you get r=db48x.
You should really check all this page info code to make sure that anything that
accesses properties and methods goes through a wrapper.

Comment 31

[jag (Peter Annema)]

Not that you have to do that for this bug or this patch, just saying it should
be done :-)

Comment 32

[Erik Fabert]

Attachment: Same patch for Firefox

If we place XPCNativeWrapper under toolkit/content, won't Mozilla 1.8 have two
copies then?

Comment 33

[Daniel Brooks [:db48x]]

Attachment: patch to add XPCNativeWrapper.js

Comment 34

[Daniel Brooks [:db48x]]

Comment on attachment 155569 [details] [diff] [review]
Same patch for Firefox

r=db48x

Comment 35

[Daniel Brooks [:db48x]]

Comment on attachment 155344 [details] [diff] [review]
Reduced patch, uses toLowerCase()

>+        var formWrapper = new XPCNativeWrapper(elem.form, "target", "action");
>+        linkView.addRow([elem.value || gStrings.linkSubmit, getAbsoluteURL(formWrapper.action, elem), gStrings.linkSubmission, formWrapper.target]);


Naturally as soon as I reviewed it, I found a way to break it. Put an if around
it like this:

if ("form" in elem && form.elem)
{
  var formWrapper = new XPCNativeWrapper(elem.form, "target", "action");
  linkView.addRow([elem.value || gStrings.linkSubmit, formWrapper.action,
gStrings.linkSubmission, formWrapper.target]);
  }
  else
    linkView.addRow([elem.value || gStrings.linkSubmit, '',
gStrings.linkSubmission, '']);

Buttons and other form inputs aren't supposed to happen outside a form, but it
does happen. Before that wouldn't have been much of a problem, now it causes a
js exception because elem.form is null, which stops page info from working. The
Firefox code needs the same test, of course.

Comment 36

[Daniel Brooks [:db48x]]

err, s/form.elem/elem.form/ ;)

Comment 37

[Erik Fabert]

Attachment: Combined patch

Adressed the problem with form elements outside a form. I don't think we should
add those elements to the Links tab, they can't submit anything except with a
script. Also added another check for the obscure case of <button type="image">.

Comment 38

[Daniel Brooks [:db48x]]

Erik: I'm terribly sorry that I've left this bug alone so far, it could have
been checked in two months ago if I'd remembered (perhaps you should pester me
more next time :)

Anyway, I just recently finished up my patch for bug 195492, which will fix a
good number of bugs in page info all at once. It turns out that my patch for
that bug includes your fix for this one, so I can go ahead and get it reviewed
and checked in with 195492 or I can seperate it out, which ever you prefer.
Either way gets your name on the changes ;)

Bug 195492 will probably be check in within a few days.

Comment 39

[Erik Fabert]

Daniel, I've had quite forgotten about this bug, too. I don't see any need for 
the extra effort to separate the code again, feel free to get review and check 
it in together with your patch. :-)

Comment 40

[Daniel Brooks [:db48x]]

just chekcked in, marking bug 19492 and it's dependants fixed