166752 - [FIX]A simple page with two forms - PageInfo reports three,

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Richard R. Kramer]

On page with two forms the first form is duplicated as reported by View Page
Info. The serious part is that javascript code such as
document.forms[1].whatever fails. The DOM inspector display looks OK.

Comment 1

[Vladimir Ermakov]

Wierd, looks like there is 3 forms document.forms[0],[1],[2]

Comment 2

[Andrew Schultz]

also happens on linux trunk build 20020903
regression between 2002062713 and 2002062904

Comment 3

[Alexandru Savulov]

hmmm, if i add an input to the first form, everything works corectly. this is
DOM not form submission.

Comment 4

[Richard R. Kramer]

If that additional input is a button, the problem remains.

Comment 5

[Johnny Stenback (:jst)]

bz, looks like the document.forms content list is out of whack, it puts the
first form in the list twice, thus document.forms.length is 3, and not 2. Ideas?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: fix

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[John Keiser (jkeiser)]

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

Straightforward enough.  r=jkeiser

Comment 9

[Boris Zbarsky [:bzbarsky]]

taking

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Looks fine to me too, but...  

>+       * We want to append instead of invalidating if the first one of
>+       * the things that got appended comes after ourLastContent


... "the first one of the things" sounds a bit odd.  Nix "one"?


>+      nsCOMPtr<nsIDOM3Node> ourLastDOM3Node(do_QueryInterface(ourLastContent));
>+      nsCOMPtr<nsIContent> firstAppendedContent;
>+      aContainer->ChildAt(aNewIndexInContainer,
>+                          *getter_AddRefs(firstAppendedContent));
>+      nsCOMPtr<nsIDOMNode> newNode(do_QueryInterface(firstAppendedContent));
>+      if (ourLastDOM3Node && newNode) {

Getting the DOM3Node is somewhat expensive.  Since you don't actually use
ourLastDom3Node just yet, wouldn't it be better to delay getting that until
after we know we have a newNode?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: sure thing. More like this?

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 98100 [details] [diff] [review]
sure thing. More like this?

bz's making a new patch that moves the newNode code into the if.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Argh. I am being dense today

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 98103 [details] [diff] [review]
Argh.  I am being dense today

r=caillon.  :-)

Comment 15

[Peter Van der Beken [:peterv]]

+       * We want to append instead of invalidating if the first thing
+       * that that got appended comes after ourLastContent.

invalidating should be invalidate
that that

Maybe the reviewers are dense today ;-).

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Maybe.  I saw the append/invalidating thing but my brain didn't flag it as wrong
since it's not really that wrong.  And it's late, 'that that' is just one of
those optical illusions  :-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: fix double "that", keep the correct gerund.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 98105 [details] [diff] [review]
fix double "that", keep the correct gerund.

sr=jst

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 98105 [details] [diff] [review]
fix double "that", keep the correct gerund.

a=asa (on behalf of drivers) for checkin to 1.2a

Comment 20

[Boris Zbarsky [:bzbarsky]]

fix is in.