unnamed form has '[object HTMLSelectElement]' target

RESOLVED FIXED

Status

SeaMonkey
Page Info
--
minor
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Adam Hauner, Assigned: db48x)

Tracking

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

16 years ago
/me sighs

probably related to that other bug, I'll try the same workaround
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

16 years ago
Created attachment 79069 [details] [diff] [review]
patch

just needs r=
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...
Attachment #79069 - Flags: needs-work+
(Assignee)

Comment 4

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

Comment 6

16 years ago
WFM on original page with 2002071508/trunk/W2K, maybe mark-up changed.
(Assignee)

Comment 7

16 years ago
I can no longer reproduce either. 
http://db48x.dyndns.org/pageInfo/tests/136532.html
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 8

16 years ago
Original repro is showing problem again using 2002111208/trunk/W2K
-> REOPEN
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 9

15 years ago
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.
This would be fixed by just using the right getter.  Caillon, weren't you gonna
do this?

Comment 11

15 years ago
Should the img.title bug I described be filed separately, if it involves using
the right getter in a different place?
No.  All of page info should just get converted at once.

Comment 13

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

Comment 14

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

Comment 16

14 years ago
(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.
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

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

14 years ago
Created attachment 155255 [details]
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

14 years ago
Created attachment 155256 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #155256 - Flags: superreview?(jag)
Attachment #155256 - Flags: review?(db48x)

Comment 21

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

Comment 22

14 years ago
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.
Attachment #155256 - Flags: review?(db48x) → review-
(Assignee)

Comment 23

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

14 years ago
(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.

 

Updated

14 years ago
Attachment #155256 - Flags: superreview?(jag)
(Assignee)

Comment 25

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

Comment 26

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

Comment 27

14 years ago
Created attachment 155344 [details] [diff] [review]
Reduced patch, uses toLowerCase()
Attachment #155256 - Attachment is obsolete: true

Comment 28

14 years ago
(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?

Updated

14 years ago
Attachment #155344 - Flags: review?(db48x)
(Assignee)

Comment 29

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

Comment 30

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

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

Comment 32

14 years ago
Created attachment 155569 [details] [diff] [review]
Same patch for Firefox

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

Comment 33

14 years ago
Created attachment 155618 [details] [diff] [review]
patch to add XPCNativeWrapper.js
(Assignee)

Updated

14 years ago
Attachment #155344 - Flags: review?(db48x) → review+
(Assignee)

Comment 34

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

r=db48x
Attachment #155569 - Flags: review+
(Assignee)

Comment 35

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

Comment 36

14 years ago
err, s/form.elem/elem.form/ ;)

Comment 37

14 years ago
Created attachment 155681 [details] [diff] [review]
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">.
Attachment #155344 - Attachment is obsolete: true
Attachment #155569 - Attachment is obsolete: true
(Assignee)

Comment 38

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

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

Updated

14 years ago
Depends on: 195492
Product: Browser → Seamonkey
(Assignee)

Comment 40

13 years ago
just chekcked in, marking bug 19492 and it's dependants fixed
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.