226752 - 'Page info' dialog shows form fields belonging to incorrect forms

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Baron Schwartz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031112 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031112 Firebird/0.7+

In a page with multiple forms, the page info dialog doesn't show the fields
correctly; when selecting one form with a mouse, it shows fields that belong to
others and doesn't show fields that belong to it.

Reproducible: Always

Steps to Reproduce:
1. Go to https://adwords.google.com/select/
2. Right-click and view source.
3. Find the string <form action="main#a" for reference.
4. Right-click on the browser again and view page info.
5. Select the Forms tab.  You should see three forms.
6. Select the unnamed form whose method is POST.
7. Select the named form 'langform' whose method is GET.

Actual Results:  
After step 6, the lower half of the dialog displays 5 fields: login.userid,
login.password, login, hl, and start.  After step 7, the lower half displays
those fields, plus three additional fields.

Expected Results:  
It should display login.userid, login.password, cmd, login, and hl.  After step
7, it should display only hl, hl, and null.

it seems as though it's considering that a form contains all fields defined
after the opening <form> tag, without regard to the closing tags.  Thus the
first form in the document contains all fields, and each succeeding one contains
fewer and fewer.  This isn't quite right, though -- because the first form
(langform) doesn't report that it contains the submit element named 'null', and
the second form doesn't report that it contains the hidden 'cmd' element.

Comment 1

[Bill Mason]

->Browser/Page Info

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is at
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/pageInfo.js#685
-- the page info code assumes a form's controls are descendants of the <form> in
the DOM.  On invalid pages like this, that's not the case....

Comment 3

[Baron Schwartz]

Hmmm.  I was wondering if that was the problem, too (I don't know the source,
other than just reading through what you referenced).  The page is certainly not
valid HTML, but the tags are correctly nested for the first form, at least.  I
haven't looked at the rest.  Here's the first one:

<form action="main" method="get" style="margin-top: 0px" id="langform"
name="langform">
    <input type="hidden" name="cmd" value="Login">
    <tr valign="top">
        <td nowrap>
            <h4 style="margin-bottom: 4px">
                <font color="#666666" size="-1">It's All About Results&#153;</font>
            </h4>
        </td>
        <td align="right" nowrap>
            <font size="-1">Change Language:
                <select name="hl"
onchange="document.forms[&quot;langform&quot;].submit()">
                    <font size="-1"></font>
                    <option value="da"> Danish</option>
                    <!-- etc etc -->
                    <option value="sv"> Swedish</option>
                </select>
                <noscript>
                    <input type="submit" name="null" value="Go" tabindex="5">
                    </noscript> -&nbsp;
                <a href="index.html">Help</a>
            </font>
        </td>
    </tr>
</form>

I don't know if that means anything, but I'd think that a correct DOM tree would
come out of this, and then the descendants would be a safe bet.  Of course this
doesn't help when the DOM is whacked, I am just wondering if this is the whole
problem.  Or is that really what that function is doing?  It seems to be walking
the rest of the tree in document order, not walking the descendants.  Again, I
don't know this source code.

I also noticed that their ugly HTML confused me.  The first form, 'langform,'
should have elements 'cmd', 'hl', and possibly 'null' (the submit element)
depending on how the noscript tag affects things (?).  The second should have
'cmd', 'hl', 'login.userid', 'login.password', and 'login'.  Sorry about the
false reporting there.

Comment 4

[Baron Schwartz]

I stand corrected... I have created a small test case that shows this dialog
DOES work right when the page is valid.

http://www.sequent.org/test.html

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I don't know if that means anything, but I'd think that a correct DOM tree
> would come out of this

  <table> <form> <tr> </tr> </form> </table>

produces the following DOM:

  <table> <form /> <tr> </tr> </table>

since <tr> cannot be contained directly in <form>....

The treewalker fu just needs to go and get replaced by a form.elements traversal...

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Daniel Brooks [:db48x]]

Attachment: fix

use form.elements, also fix 220833 while I'm here

Comment 9

[Daniel Brooks [:db48x]]

Comment on attachment 138993 [details] [diff] [review]
fix

requesting r=/sr=

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138993 [details] [diff] [review]
fix

sr=bzbarsky

Comment 11

[jag (Peter Annema)]

Comment on attachment 138993 [details] [diff] [review]
fix

r=jag assuming this has been teste with both valid and invalid pages with
single and multiple forms. form.elements seems so easy and obvious that it
makes me wonder if there's a reason it wasn't used.

Comment 12

[Daniel Brooks [:db48x]]

It wasn't used because I never thought of it, probably because it is too easy.

Comment 13

[Daniel Brooks [:db48x]]

and now that I'm home and have tested this, I'll can say with 100% certainty
that  this does indeed fix the problem (but why wouldn't it?).

Comment 14

[Daniel Brooks [:db48x]]

checked in, marking fixed

Comment 15

[Boris Zbarsky [:bzbarsky]]

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