163822 - input type image missing in Form fields list

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Adam Hauner]

Inputs with type submit or reset are listed on Forms tabs, but inputs with type
image is missing. 

Repro:
1. open testcase
2. open Page Info | Forms

Actual:
There are only two rows of Fields with reset and submit type, but input with
image type is missing.

Expected:
Input with type=image is correct form element and should be listed here.

Comment 1

[Adam Hauner]

Attachment: testcase

Comment 2

[Jay Knight]

Confirmed on 2002082104.

Comment 3

[Daniel Brooks [:db48x]]

this isn't actually a bug in page info, the .elements property of the form isn't
including the input when it's type equals image. I'm pretty certain this used to
work, could it be a recent regression?

-> HTML DOM

Comment 4

[Johnny Stenback (:jst)]

Daniel, this is a page info bug and a page info bug only. The DOM is supposed to
work that way, that's how it works in Mozilla, NS4x, and IE. We can't change
that even if we wanted to, lots of sites would break. If page info wants to list
<input type=image> elements it must do so w/o relying on form.elements.

Comment 5

[Daniel Brooks [:db48x]]

really? that is messed up. well, so much for always displaying the form 
elements in document order (though I suppose you'll tell me there was never 
actually any gaurantee of that either.) So, lets see...

Comment 6

[Daniel Brooks [:db48x]]

Attachment: this should work, but doesn't

NS_ERROR_UNEXPECTED, I know, I didn't expect it to fail either! sheesh.

anyway, gotta run, look at it when I get home.

Comment 7

[Daniel Brooks [:db48x]]

Attachment: this works :)

now for r= etc

Comment 8

[Daniel Brooks [:db48x]]

Attachment: remove the extra concat call timeless found

Comment 9

[Daniel Brooks [:db48x]]

sicking, could you please review this? it uses the DOMTreeWalker.

Comment 10

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

Comment on attachment 97213 [details] [diff] [review]
remove the extra concat call timeless found

>+function findImageControls(aNode)
>+{
>+  function ImageControlFilter() 
>+  {
>+    this.acceptNode = function(aNode)
>+    {
>+      switch (aNode.nodeName.toLowerCase())
>+      {
>+        case "input":
>+          if (aNode.type=="image")
>+            return NodeFilter.FILTER_ACCEPT;
>+          break;
>+        default:
>+          return NodeFilter.FILTER_SKIP;
>+          break;
>+      }
>+      return NodeFilter.FILTER_SKIP;   // placate the js compiler

Wouldn't this be more simply done as

if (aNode.nodeName.toLowerCase()=="input" && aNode.type=="image")
  return NodeFilter.FILTER_ACCEPT;
return NodeFilter.FILTER_SKIP;

?

Also, as a sidenote. You can actually pass createTreeWalker a function instead
of an object with an acceptNode method. So

function ImageControlFilter(aNode) {
  if (aNode.nodeName.toLowerCase()=="input" && aNode.type=="image")
    return NodeFilter.FILTER_ACCEPT;
  return NodeFilter.FILTER_SKIP;
}
var iterator = theDocument.createTreeWalker(aNode, NodeFilter.SHOW_ELEMENT,
ImageControlFilter, true);

will work fine (in fact that is how the EcmaScript bindings say it should be
done). Feel free to do either way, especially since all other filters in this
script uses the acceptNode-method way.

r=sicking

Comment 11

[Daniel Brooks [:db48x]]

yea, I don't know why I used a switch there, I'll change that. 

However, as far as passing the filter in directly (without creating a 'new'
filter), I can only get that to work on line 446 (my line numbers are going to
be a few off from what's in the patch now). on lines 472 and 592 I have to
create the new object, otherwise it just doesn't work. I have no idea why, and
it does it whether or not I use acceptNode (which I prefer.) I'd really like to
find out why. Do you have any ideas?

Comment 12

[Daniel Brooks [:db48x]]

Attachment: remove copy-and-paste crud

Comment 13

[Johnny Stenback (:jst)]

To do this right you should also take the node namespace URI into account if
you're dealing with XHTML. I think you forgot the == "input" in that last diff
btw...

Could you by any chance create a simplified testcase that shows the node
iterator code not working correctly when you pass in a function in stead of an
object with an acceptNode function on it? If so, please file a separate bug on
me with that testcase.

Comment 14

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

Comment on attachment 97241 [details] [diff] [review]
remove copy-and-paste crud

>+      if (aNode.nodeName.toLowerCase() && aNode.type == "image")

hehe, i made the *exact* same error while writing my last comment :)
You're missing the | == "input"|


>+    theList = theList.concat(iterator.currentNode);

Sorry, missed this last time. This whould be better done using
theList.push(iterator.currentNode)


Could you show me an example of code that doesn't work. I'm not sure i fully
understand how you are trying

Comment 15

[Daniel Brooks [:db48x]]

this code doesn't work (line 471 for me):
  var iterator = theDocument.createTreeWalker(aNode, NodeFilter.SHOW_ELEMENT,
ImageControlFilter, true);

whereas this does (used in place of the above):
  var filter = new ImageControlFilter;
  var iterator = theDocument.createTreeWalker(aNode, NodeFilter.SHOW_ELEMENT,
filter, true);

however, on line 446, I use the following:
  var iterator = theDocument.createTreeWalker(node, NodeFilter.SHOW_ELEMENT,
FormControlFilter, true);

I'm not sure if it's some difference between FormControlFilter and
ImageControlFilter (a typo maybe) or what.

also, does push() put stuff at the end of the list like concat does? if so, I'll
use push a couple other places too.

Comment 16

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

That's not the way to do it, what you're doing is giving the
createTreeWalker-function a class that contains an acceptNode method. What you
want to do is to give it a function that *is* the acceptNode function.

So instead of

 function ImageControlFilter() 
 {
   this.acceptNode = function(aNode)
   {
     if (aNode.nodeName.toLowerCase() && aNode.type == "image")
       return NodeFilter.FILTER_ACCEPT;
     return NodeFilter.FILTER_SKIP;
   }
 }

 var filter = new ImageControlFilter;
 var iterator = theDocument.createTreeWalker(aNode, NodeFilter.SHOW_ELEMENT,
filter, true);


you just do


 function myFilter(aNode)
 {
   if (aNode.nodeName.toLowerCase() && aNode.type == "image")
     return NodeFilter.FILTER_ACCEPT;
   return NodeFilter.FILTER_SKIP;
 }

 var iterator = theDocument.createTreeWalker(aNode, NodeFilter.SHOW_ELEMENT,
myFilter, true);

Comment 17

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

and yes, myArray.push(foo) appends 'foo' to the end of myArray

Comment 18

[Daniel Brooks [:db48x]]

I understand that, but I'm giveing the the treewalker the same thing in both
cases, a class with an acceptnode function. I'd just like to know why it works
the first time but fails the second two.

Comment 19

[Daniel Brooks [:db48x]]

ok, so I figured it out.

in the one that worked, I was passing the node in: FormControlFilter(node), but
in the others I wasn't: ImageControlFilter(). Anyway, now it works, and I might
as well condense it down to just a single function, instead of an object with a
member. Anyway, I'm glad I won't have to let that drive me crazy anymore. :P

Comment 20

[Daniel Brooks [:db48x]]

Attachment: yay

addresses all issues brought up by sicking

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 97339 [details] [diff] [review]
yay

>Index: pageInfo.js
>===================================================================
>+    var formfields = new Array();

Prefer

  = [];

to 

  = new Array();

Comment 22

[Daniel Brooks [:db48x]]

Attachment: /me sighs. ok.

I guess I'm just old fashioned and think new is cool. ;)

Comment 23

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

Comment on attachment 97340 [details] [diff] [review]
/me sighs. ok.

>-      theList = theList.concat(inputList[i]);
>+      theList = theList.push(inputList[i]);

This should just be

theList.push(inputList[i]);

(no |theList =|). This needs to be fixed in a few places.

Also, the patch still says new Array instead of []? Did you agree to do that
instead?

If so, r=sicking

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

AIUI, [] is more performant than new Array which is why I suggested that be done.

Comment 25

[Daniel Brooks [:db48x]]

doh, it's just the wrong patch, that's all.

Comment 26

[Daniel Brooks [:db48x]]

Attachment: correct file

Comment 27

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

Comment on attachment 97360 [details] [diff] [review]
correct file

r=me

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 97360 [details] [diff] [review]
correct file

This still doesn't work correctly with non-HTML elements, do we care?

I.e. if I create an XML document that has an element in any namespace (or no
namespace) w/o using a prefix and I name the element "input" and add a
type="image" attribute we'll think it's an image input even if it's not.

This is easy enough to fix so I'd suggest we'd do that (i.e. check that element
instanceOf HTMLElement before accepting anything in the filter).

Comment 29

[Daniel Brooks [:db48x]]

now that bug 177047 is in, this is fixed. jst: 177047 also corrected page info
to only look at html and xhtml nodes, including this case.